Add regex escaping code to Mac sandbox implementation and re-enable the utility process on OS X.

Add regex escaping code to Mac sandbox implementation and re-enable the utility process on OS X.

Other changes:
* An error initializing the sandbox on OS X is now treated as fatal.
* Improved error reporting for sandbox-related failures.

BUG=26492, 23837
TEST=Installing extensions and themes should still work on OS X.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33682

[jeremy, 2009-11-25 15:23:30]

Anything I should be escaping that I'm not?

[TVL, 2009-11-25 15:38:07]

http://codereview.chromium.org/434077/diff/2009/2015
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/434077/diff/2009/2015#newcode32
chrome/common/sandbox_mac.mm:32: static bool EscapeSingleChar(const CHAR c,
std::string* dst) {
anon namespace outside sandbox instead of static within?

http://codereview.chromium.org/434077/diff/2009/2015#newcode52
chrome/common/sandbox_mac.mm:52: case '"':
does single quote matter?

http://codereview.chromium.org/434077/diff/2009/2015#newcode102
chrome/common/sandbox_mac.mm:102: if (c < 128) {  // EscapeSingleChar only
handles ASCII.
would this be more readable if it was simply a switch on c?  with cases for
everything in EscapeSingleChar and regex_special_chars?  then the default would
just need one check for asc to do the numeric vs. raw form.  this way it bounces
through a bunch of gates and a second function which seems a little more
complex.

http://codereview.chromium.org/434077/diff/2009/2016
File chrome/common/sandbox_mac_unittest.mm (right):

http://codereview.chromium.org/434077/diff/2009/2016#newcode32
chrome/common/sandbox_mac_unittest.mm:32: std::string in_utf8("^.$|()[]*+?{}");
send in an escape (\) also. (will be a little confusing since you have to escape
it for CPP, but \\ should become \\\\ (meaning 1 becomes 2)

[Mark Mentovai, 2009-11-25 15:45:32]

http://codereview.chromium.org/434077/diff/2009/2014
File chrome/chrome.gyp (right):

http://codereview.chromium.org/434077/diff/2009/2014#newcode4800
chrome/chrome.gyp:4800: 'common/sandbox_mac_unittest.mm',
Keep this list sorted.

http://codereview.chromium.org/434077/diff/2009/2015
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/434077/diff/2009/2015#newcode31
chrome/common/sandbox_mac.mm:31: template<typename CHAR>
Unnecessary to templatize, since we only care about char.

http://codereview.chromium.org/434077/diff/2009/2015#newcode32
chrome/common/sandbox_mac.mm:32: static bool EscapeSingleChar(const CHAR c,
std::string* dst) {
It's unusual to declare a primitive argument const in our code.  It's OK that
you've done so here, I'm just bringing it up in case you didn't intend this.

http://codereview.chromium.org/434077/diff/2009/2015#newcode32
chrome/common/sandbox_mac.mm:32: static bool EscapeSingleChar(const CHAR c,
std::string* dst) {
TVL wrote:
> anon namespace outside sandbox instead of static within?

It can also be an anonymous namespace within a named namespace.  I also think
static is fine.  Your call.

http://codereview.chromium.org/434077/diff/2009/2015#newcode35
chrome/common/sandbox_mac.mm:35: dst->append("\\b");
You should be putting these "\\b"s and things into a char*, and then doing the
dst->append below, right before returning true.  You shouldn't be duplicating
these dst->appends.

http://codereview.chromium.org/434077/diff/2009/2015#newcode65
chrome/common/sandbox_mac.mm:65: // |dst| is filled in on output with an escaped
version having the following
Since this returns void and sets dst, I think it'd be better if it just returned
std::string.

http://codereview.chromium.org/434077/diff/2009/2015#newcode76
chrome/common/sandbox_mac.mm:76: char regex_special_chars[] = {
const?

http://codereview.chromium.org/434077/diff/2009/2015#newcode78
chrome/common/sandbox_mac.mm:78: '^',
Have you tested things using paths that have these characters in them to ensure
that they do the right thing?

Specifically, the unit test you wrote is good, but it doesn't do anything to
bring Apple's sandbox RE parsing into the mix.  You need to either have an
automated test for that or at least have tested all of these cases manually.

http://codereview.chromium.org/434077/diff/2009/2015#newcode82
chrome/common/sandbox_mac.mm:82: '(',
This one in particular is cause for concern.  In some "basic" RE flavors, the
'(' character doesn't gain its special meaning unless it's backslash-escaped.

http://codereview.chromium.org/434077/diff/2009/2015#newcode244
chrome/common/sandbox_mac.mm:244: string16 allowed_dir_as_utf16 =
UTF8ToUTF16(allowed_dir.value());
8 to 16...

http://codereview.chromium.org/434077/diff/2009/2015#newcode246
chrome/common/sandbox_mac.mm:246: RegexQuote(allowed_dir_as_utf16,
&allowed_dir_escaped);
...to 8...

http://codereview.chromium.org/434077/diff/2009/2015#newcode247
chrome/common/sandbox_mac.mm:247: NSString* allowed_dir_escaped_ns =
base::SysUTF8ToNSString(
...to NS.

Why are we doing the 8 to 16 hop?  Can't we do all of this in UTF8-land?  Isn't
there a U8_NEXT you could use in your RegexQuote function?

[jeremy, 2009-11-30 15:20:12]

I've added tests to bring the sandbox regex parser into the mix.  Turns out that
only characters codes between 32 & 125 are accepted in regexes.  I've tried all
manner of escaping with no luck.

I'd be grateful if you could take a cursory look and see if you see any
fundamental flaw in the current snapshot.

Note: This CL is not final, I'll clean up style etc when I have everything fully
in place but I wanted to checkpoint and get some feedback at this stage since I
want to make sure we get this right.

http://codereview.chromium.org/434077/diff/2009/2014
File chrome/chrome.gyp (right):

http://codereview.chromium.org/434077/diff/2009/2014#newcode4800
chrome/chrome.gyp:4800: 'common/sandbox_mac_unittest.mm',
On 2009/11/25 15:45:32, Mark Mentovai wrote:
> Keep this list sorted.

Done.

http://codereview.chromium.org/434077/diff/2009/2015
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/434077/diff/2009/2015#newcode31
chrome/common/sandbox_mac.mm:31: template<typename CHAR>
This function is taken from base/json/string_escape.cc - JsonSingleEscapeChar()
.  The reason I didn't use that version is because you can't use icu in base
anymore apparently and the regex escaping code is tailored for the Mac sandbox
implementation.  Do you think this would fit better somewhere else?

http://codereview.chromium.org/434077/diff/2009/2015#newcode76
chrome/common/sandbox_mac.mm:76: char regex_special_chars[] = {
On 2009/11/25 15:45:32, Mark Mentovai wrote:
> const?

Done.

http://codereview.chromium.org/434077/diff/2009/2015#newcode102
chrome/common/sandbox_mac.mm:102: if (c < 128) {  // EscapeSingleChar only
handles ASCII.
I thought of that but I like this better since it's easier to see which parts
are escaped due to regex vs plain string escaping.

[Mark Mentovai, 2009-11-30 15:30:08]

If EnableSandbox returns false, does stuff just proceed with no
sandbox, or does it cause death?

Mark

Jeremy Moskovich wrote:
> I've added tests to bring the sandbox regex parser into the mix.  Turns out
> that
> only characters codes between 32 & 125 are accepted in regexes.  I've tried
> all
> manner of escaping with no luck.
>
> I'd be grateful if you could take a cursory look and see if you see any
> fundamental flaw in the current snapshot.
>
> Note: This CL is not final, I'll clean up style etc when I have everything
> fully
> in place but I wanted to checkpoint and get some feedback at this stage
> since I
> want to make sure we get this right.
>
>
>
>
> http://codereview.chromium.org/434077/diff/2009/2014
> File chrome/chrome.gyp (right):
>
> http://codereview.chromium.org/434077/diff/2009/2014#newcode4800
> chrome/chrome.gyp:4800: 'common/sandbox_mac_unittest.mm',
> On 2009/11/25 15:45:32, Mark Mentovai wrote:
>>
>> Keep this list sorted.
>
> Done.
>
> http://codereview.chromium.org/434077/diff/2009/2015
> File chrome/common/sandbox_mac.mm (right):
>
> http://codereview.chromium.org/434077/diff/2009/2015#newcode31
> chrome/common/sandbox_mac.mm:31: template<typename CHAR>
> This function is taken from base/json/string_escape.cc -
> JsonSingleEscapeChar() .  The reason I didn't use that version is
> because you can't use icu in base anymore apparently and the regex
> escaping code is tailored for the Mac sandbox implementation.  Do you
> think this would fit better somewhere else?
>
> http://codereview.chromium.org/434077/diff/2009/2015#newcode76
> chrome/common/sandbox_mac.mm:76: char regex_special_chars[] = {
> On 2009/11/25 15:45:32, Mark Mentovai wrote:
>>
>> const?
>
> Done.
>
> http://codereview.chromium.org/434077/diff/2009/2015#newcode102
> chrome/common/sandbox_mac.mm:102: if (c < 128) {  // EscapeSingleChar
> only handles ASCII.
> I thought of that but I like this better since it's easier to see which
> parts are escaped due to regex vs plain string escaping.
>
> http://codereview.chromium.org/434077
>

[TVL, 2009-11-30 15:38:12]

On 2009/11/30 15:20:12, jeremy wrote:
> I've added tests to bring the sandbox regex parser into the mix.  Turns out
that
> only characters codes between 32 & 125 are accepted in regexes.  I've tried
all
> manner of escaping with no luck.

What does that mean for the home dir/temp dir then?  Can either run into this?

[TVL, 2009-11-30 15:40:31]

On 2009/11/30 15:38:12, TVL wrote:
> On 2009/11/30 15:20:12, jeremy wrote:
> > I've added tests to bring the sandbox regex parser into the mix.  Turns out
> that
> > only characters codes between 32 & 125 are accepted in regexes.  I've tried
> all
> > manner of escaping with no luck.
> 
> What does that mean for the home dir/temp dir then?  Can either run into this?

Specifically, how do we get the temp dir, could an env variable impact this
depending on how we were launched?  (if yes, there may well be other security
implications given what we use the directory for)

[jeremy, 2009-11-30 17:12:27]

We currently only use the regex code with a temp directory path, the place
where we use the home directory is 10.6 only and uses different non-regex
sandbox syntax.

I'll do some searching and see if there's any way the temp directory path
can be manipulated to contain characters outside this range.

Best regards,
Jeremy

On Mon, Nov 30, 2009 at 5:38 PM, <thomasvl@chromium.org> wrote:

> On 2009/11/30 15:20:12, jeremy wrote:
>
>> I've added tests to bring the sandbox regex parser into the mix.  Turns
>> out
>>
> that
>
>> only characters codes between 32 & 125 are accepted in regexes.  I've
>> tried
>>
> all
>
>> manner of escaping with no luck.
>>
>
> What does that mean for the home dir/temp dir then?  Can either run into
> this?
>
>
>
> http://codereview.chromium.org/434077
>

[jeremy, 2009-12-01 13:59:27]

OK this is ready for a full review now, thanks for the feedback - to answer some
previous questions:

* As of this revision, a failure in EnableSandbox() is considered fatal.
* I've split the escaping code into a function that escapes string literals and
one for regexes.  The regex code is currently only used with temporary directory
paths.  I've run some experiments with NSTemporaryDirectory() and have been
unable to get it to return a path other than in /private/var .

[TVL, 2009-12-01 14:37:33]

mark had a fair number of comments in here before, so I'll let him cover the
meat of the changes, here are a few minor things.

http://codereview.chromium.org/434077/diff/6017/5013
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/434077/diff/6017/5013#newcode270
chrome/common/sandbox_mac.mm:270: LOG(ERROR) << "Failed to find the sandbox
profile on disk"
space before quote?

http://codereview.chromium.org/434077/diff/6017/5013#newcode287
chrome/common/sandbox_mac.mm:287: // The sandbox only understands "real" paths. 
This resolving steps is
s/steps/step/?

http://codereview.chromium.org/434077/diff/6017/5013#newcode341
chrome/common/sandbox_mac.mm:341: if (error == -1 || !success) {
looks like the error == -1 isn't needed since that is captures in success.

[jeremy, 2009-12-01 14:51:44]

tvl: thanks, all fixed.

[Mark Mentovai, 2009-12-01 15:07:32]

http://codereview.chromium.org/434077/diff/6017/5007
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/434077/diff/6017/5007#newcode553
chrome/app/chrome_dll_main.cc:553: // Die if Sandbox can't be enabled.
Die if the sandbox

(insert "the" and use a lowercase "s" for something that's not a proper name.)

http://codereview.chromium.org/434077/diff/6017/5007#newcode555
chrome/app/chrome_dll_main.cc:555: << process_type;
Line up << with the << on the previous line.

http://codereview.chromium.org/434077/diff/6017/5013
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/434077/diff/6017/5013#newcode17
chrome/common/sandbox_mac.mm:17: #include "base/file_util.h"
Sort.  f < j.

http://codereview.chromium.org/434077/diff/6017/5013#newcode32
chrome/common/sandbox_mac.mm:32: template<typename CHAR>
Again:

This should not be templatized.  The first argument should be |char|.

I understand that you took the code from somewhere else, but since you're taking
the code and not sharing it, there's no reason to keep it templatized.  It
doesn't need to be templatized for use in this file.

http://codereview.chromium.org/434077/diff/6017/5013#newcode77
chrome/common/sandbox_mac.mm:77: DCHECK(c>=0);
DCHECK_GE(c, 0).

http://codereview.chromium.org/434077/diff/6017/5013#newcode78
chrome/common/sandbox_mac.mm:78: if (c<0)
if (c < 0)

(space around operators.)

http://codereview.chromium.org/434077/diff/6017/5013#newcode89
chrome/common/sandbox_mac.mm:89: unsigned int as_uint = static_cast<unsigned
int>(c);
Put a comment here saying that anything that's not printable ASCII gets the \u
treatment.

It's important to say the word "printable" in your comment so that the
discrepancy between < 128 and > 126 makes sense.

http://codereview.chromium.org/434077/diff/6017/5013#newcode113
chrome/common/sandbox_mac.mm:113: bool QuoteStringForRegex(const std::string&
str_utf8, std::string* dst) {
You should file an Apple bug on the limitations of the regular expressions used
for sandbox strings.

You should reference the Apple bug somewhere in this file, although if you need
to check this in first, that's fine.  You can add the bug reference in a
follow-up.

http://codereview.chromium.org/434077/diff/6017/5013#newcode143
chrome/common/sandbox_mac.mm:143: DCHECK(c>=0);
DCHECK_GE

http://codereview.chromium.org/434077/diff/6017/5013#newcode144
chrome/common/sandbox_mac.mm:144: if (c<0)
Spaces around the < operator.

http://codereview.chromium.org/434077/diff/6017/5013#newcode148
chrome/common/sandbox_mac.mm:148: // 32 >= c <= 125
Uh-oh.

First of all, your inequality is wrong.  You want <= on both sides of c.

Second of all, 125?  Shouldn't that be 126?  126 is '~', which is certainly a
printable ASCII character.

http://codereview.chromium.org/434077/diff/6017/5013#newcode160
chrome/common/sandbox_mac.mm:160: // If we got here we know that the character
in question is strictly
This comment is redundant in this function.

http://codereview.chromium.org/434077/diff/6017/5013#newcode273
chrome/common/sandbox_mac.mm:273: << strerror(errno);
Don't do strerror(errno) yourself.  Use PLOG instead of LOG.  That appends
strerror(errno) for you.

http://codereview.chromium.org/434077/diff/6017/5013#newcode293
chrome/common/sandbox_mac.mm:293: LOG(ERROR) << "Failed to resolve absolute path
error: "
Use PLOG, not strerror.

http://codereview.chromium.org/434077/diff/6017/5013#newcode307
chrome/common/sandbox_mac.mm:307:
stringByReplacingOccurrencesOfString:@"DIR_TO_ALLOW_ACCESS"
utility.sb does:

(allow file-read* file-write* (regex #"^DIR_TO_ALLOW_ACCESS"))

That anchors the front at the beginning but doesn't anchor the end.  If
DIR_TO_ALLOW_ACCESS is /tmp/m, then this sandbox config will still allow access
to /tmp/myPrivateFiles.  That's bad.

I think that the ^ should be moved out of utility.sb and into the
QuoteStringForRegex function.

The end of the string should be anchored with a trailing slash.

If something in the sandbox needs to access the directory by name without a
trailing slash, you should anchor the end of the regex with:

(/|$)

(meaning "accept a trailing slash here and implicitly anything that follows, or
accept a string that ends right here.")

You should add tests for this to your sandbox unit test.

http://codereview.chromium.org/434077/diff/6017/5013#newcode342
chrome/common/sandbox_mac.mm:342: LOG(ERROR) << "Failed to Initialize Sandbox: "
Use LOG_IF instead of if (junk) LOG.

http://codereview.chromium.org/434077/diff/6017/5013#newcode343
chrome/common/sandbox_mac.mm:343: << error
Line up the == -1s.

http://codereview.chromium.org/434077/diff/6017/5014
File chrome/common/sandbox_mac_unittest.mm (right):

http://codereview.chromium.org/434077/diff/6017/5014#newcode26
chrome/common/sandbox_mac_unittest.mm:26: const char* kSandboxAccessPathKey =
"sandbox_dir";
static or anonymous namespace.

[jeremy, 2009-12-01 15:59:14]

New snapshot uploaded.

On Tue, Dec 1, 2009 at 5:07 PM, <mark@chromium.org> wrote:

>
> http://codereview.chromium.org/434077/diff/6017/5007
> File chrome/app/chrome_dll_main.cc (right):
>
> http://codereview.chromium.org/434077/diff/6017/5007#newcode553
> chrome/app/chrome_dll_main.cc:553: // Die if Sandbox can't be enabled.
> Die if the sandbox
>
> (insert "the" and use a lowercase "s" for something that's not a proper
> name.)
>

Done.


>
> http://codereview.chromium.org/434077/diff/6017/5007#newcode555
> chrome/app/chrome_dll_main.cc:555: << process_type;
> Line up << with the << on the previous line.


Done

>
>
> http://codereview.chromium.org/434077/diff/6017/5013
> File chrome/common/sandbox_mac.mm (right):
>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode17
> chrome/common/sandbox_mac.mm:17: #include "base/file_util.h"
> Sort.  f < j.
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode32
> chrome/common/sandbox_mac.mm:32: template<typename CHAR>
> Again:
>
> This should not be templatized.  The first argument should be |char|.
>
> I understand that you took the code from somewhere else, but since
> you're taking the code and not sharing it, there's no reason to keep it
> templatized.  It doesn't need to be templatized for use in this file.
>

Done.


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode77
> chrome/common/sandbox_mac.mm:77: DCHECK(c>=0);
> DCHECK_GE(c, 0).
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode78
> chrome/common/sandbox_mac.mm:78: if (c<0)
> if (c < 0)
>
> (space around operators.)
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode89
> chrome/common/sandbox_mac.mm:89: unsigned int as_uint =
> static_cast<unsigned int>(c);
> Put a comment here saying that anything that's not printable ASCII gets
> the \u treatment.
>
> It's important to say the word "printable" in your comment so that the
> discrepancy between < 128 and > 126 makes sense.
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode113
> chrome/common/sandbox_mac.mm:113: bool QuoteStringForRegex(const
> std::string& str_utf8, std::string* dst) {
> You should file an Apple bug on the limitations of the regular
> expressions used for sandbox strings.
>
> You should reference the Apple bug somewhere in this file, although if
> you need to check this in first, that's fine.  You can add the bug
> reference in a follow-up.
>

OK, I'll follow up with that.


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode143
> chrome/common/sandbox_mac.mm:143: DCHECK(c>=0);
> DCHECK_GE
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode144
> chrome/common/sandbox_mac.mm:144: if (c<0)
> Spaces around the < operator.
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode148
> chrome/common/sandbox_mac.mm:148: // 32 >= c <= 125
> Uh-oh.
>
> First of all, your inequality is wrong.  You want <= on both sides of c.
>
> Second of all, 125?  Shouldn't that be 126?  126 is '~', which is
> certainly a printable ASCII character.
>

Fixed the comment, the code is correct.  The ~ character isn't legal in
regexes.


> http://codereview.chromium.org/434077/diff/6017/5013#newcode160
> chrome/common/sandbox_mac.mm:160: // If we got here we know that the
> character in question is strictly
> This comment is redundant in this function.
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode273
> chrome/common/sandbox_mac.mm:273: << strerror(errno);
> Don't do strerror(errno) yourself.  Use PLOG instead of LOG.  That
> appends strerror(errno) for you.
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode293
> chrome/common/sandbox_mac.mm:293: LOG(ERROR) << "Failed to resolve
> absolute path error: "
> Use PLOG, not strerror.
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode307
> chrome/common/sandbox_mac.mm:307:
> stringByReplacingOccurrencesOfString:@"DIR_TO_ALLOW_ACCESS"
> utility.sb does:
>
> (allow file-read* file-write* (regex #"^DIR_TO_ALLOW_ACCESS"))
>
> That anchors the front at the beginning but doesn't anchor the end.  If
> DIR_TO_ALLOW_ACCESS is /tmp/m, then this sandbox config will still allow
> access to /tmp/myPrivateFiles.  That's bad.
>
> I think that the ^ should be moved out of utility.sb and into the
> QuoteStringForRegex function.
>
> The end of the string should be anchored with a trailing slash.
>
> If something in the sandbox needs to access the directory by name
> without a trailing slash, you should anchor the end of the regex with:
>
> (/|$)
>
> (meaning "accept a trailing slash here and implicitly anything that
> follows, or accept a string that ends right here.")
>
> You should add tests for this to your sandbox unit test.
>

Good catch on the trailing '/'.  I've added the ^/ appending to the code and
unit tests for robustness.
We can't anchor at the end since one of the things the utility process does
is to unpack zip files with directory structures and this sandbox rule is
matched per file so such a pattern won't work.


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode342
> chrome/common/sandbox_mac.mm:342: LOG(ERROR) << "Failed to Initialize
> Sandbox: "
> Use LOG_IF instead of if (junk) LOG.
>

Done


>
> http://codereview.chromium.org/434077/diff/6017/5013#newcode343
> chrome/common/sandbox_mac.mm:343: << error
> Line up the == -1s.
>
> http://codereview.chromium.org/434077/diff/6017/5014
> File chrome/common/sandbox_mac_unittest.mm (right):
>
> http://codereview.chromium.org/434077/diff/6017/5014#newcode26
> chrome/common/sandbox_mac_unittest.mm:26: const char*
> kSandboxAccessPathKey = "sandbox_dir";
> static or anonymous namespace.


Done


>
>
> http://codereview.chromium.org/434077
>

[Mark Mentovai, 2009-12-01 16:09:13]

Jeremy wrote:
>> (/|$)
>>
>> (meaning "accept a trailing slash here and implicitly anything that
>> follows, or accept a string that ends right here.")
>>
>> You should add tests for this to your sandbox unit test.
>
> Good catch on the trailing '/'.  I've added the ^/ appending to the code and
> unit tests for robustness.
> We can't anchor at the end since one of the things the utility process does
> is to unpack zip files with directory structures and this sandbox rule is
> matched per file so such a pattern won't work.

Reread what I wrote.  Appending (/|$) should handle this case.

Mark

[jeremy, 2009-12-01 18:06:53]

I'm not sure I understand what we gain from ending the regex with /|$ rather
than just / ?

Since the regular expression is matched against every file access then if
for example the pattern is #"^/foo/bar/"

then:
attempts to access /foo/bar123 will fail.
attemtps to access /foo/bar will succeed.
attempts to access /foo/bar/baz  will succeed.
attempts to access /foo/bar/baz/bla will succeed.

Why do we need the extra |$ ?  Given that the use case here is always
enabling access to a directory?

Best regards,
Jeremy

On Tue, Dec 1, 2009 at 6:08 PM, Mark Mentovai <mark@chromium.org> wrote:

> Jeremy wrote:
> >> (/|$)
> >>
> >> (meaning "accept a trailing slash here and implicitly anything that
> >> follows, or accept a string that ends right here.")
> >>
> >> You should add tests for this to your sandbox unit test.
> >
> > Good catch on the trailing '/'.  I've added the ^/ appending to the code
> and
> > unit tests for robustness.
> > We can't anchor at the end since one of the things the utility process
> does
> > is to unpack zip files with directory structures and this sandbox rule is
> > matched per file so such a pattern won't work.
>
> Reread what I wrote.  Appending (/|$) should handle this case.
>
> Mark
>

[Mark Mentovai, 2009-12-01 18:08:44]

Jeremy Moskovich wrote:
> I'm not sure I understand what we gain from ending the regex with /|$ rather
> than just / ?
> Since the regular expression is matched against every file access then if
> for example the pattern is #"^/foo/bar/"
> then:
> attempts to access /foo/bar123 will fail.
> attemtps to access /foo/bar will succeed.

No they won't.

> attempts to access /foo/bar/baz  will succeed.
> attempts to access /foo/bar/baz/bla will succeed.
> Why do we need the extra |$ ?  Given that the use case here is always
> enabling access to a directory?

[jeremy, 2009-12-01 18:26:42]

I stand corrected, will update the patch.

On Tue, Dec 1, 2009 at 8:08 PM, Mark Mentovai <mark@chromium.org> wrote:

> Jeremy Moskovich wrote:
> > I'm not sure I understand what we gain from ending the regex with /|$
> rather
> > than just / ?
> > Since the regular expression is matched against every file access then if
> > for example the pattern is #"^/foo/bar/"
> > then:
> > attempts to access /foo/bar123 will fail.
> > attemtps to access /foo/bar will succeed.
>
> No they won't.
>
> > attempts to access /foo/bar/baz  will succeed.
> > attempts to access /foo/bar/baz/bla will succeed.
> > Why do we need the extra |$ ?  Given that the use case here is always
> > enabling access to a directory?
>

[jeremy, 2009-12-03 10:12:56]

New snapshot uploaded.
(/|$) appended + updated unit tests to check this case.

[jeremy, 2009-12-03 10:28:58]

Turns out that for some uses of the utility process, no directory access is
required.

I've updated utility.sb & sandbox_mac.mm to reflect this.

[Mark Mentovai, 2009-12-03 14:11:25]

lg with a couple of minor changes.

http://codereview.chromium.org/434077/diff/14011/13009
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/434077/diff/14011/13009#newcode31
chrome/common/sandbox_mac.mm:31: bool EscapeSingleChar(char c, std::string* dst)
{
I think you should only have one dst->append in this function, right before
return true.

Outside of the switch, you could have |const char* append = NULL;|, and then for
each case that wants to append something, |append = "\\b";| (or whatever).

http://codereview.chromium.org/434077/diff/14011/13009#newcode72
chrome/common/sandbox_mac.mm:72: while (position < length) {
Before entering the loop, clear dst.

http://codereview.chromium.org/434077/diff/14011/13009#newcode137
chrome/common/sandbox_mac.mm:137: dst->push_back('^');
Don't push_back.  Assign.  You want to clear out dst before you add anything to
it.

http://codereview.chromium.org/434077/diff/14011/13009#newcode165
chrome/common/sandbox_mac.mm:165: // Make sure last element of path is
interpreted as a directory, leaving this
as a directory.  Leaving this

(period and a new sentence.)

http://codereview.chromium.org/434077/diff/14011/13009#newcode271
chrome/common/sandbox_mac.mm:271: <<
base::SysNSStringToUTF8(sandbox_profile_path);
Align <<s.

http://codereview.chromium.org/434077/diff/14011/13009#newcode291
chrome/common/sandbox_mac.mm:291: PLOG(ERROR) << "Failed to resolve absolute
path error: ";
PLOG will take care of appending the error and any other stuff it needs to make
sense out of the string.  This just needs to be:

      PLOG(ERROR) << "Failed to resolve absolute path";

http://codereview.chromium.org/434077/diff/14011/13009#newcode297
chrome/common/sandbox_mac.mm:297: &allowed_dir_escaped)) {
&allowed_dir_excaped should line up with allowed_dir_absolute.

http://codereview.chromium.org/434077/diff/14011/13009#newcode342
chrome/common/sandbox_mac.mm:342: LOG_IF(ERROR, !success) << "Failed to
Initialize Sandbox: "
initialize and sandbox should be lowercase.