Mac: block ability to stat arbitrary files in the Sandbox

Mac: block ability to stat arbitrary files in the Sandbox.

This change removes the ability to stat any file on disk and instead only allows stating files to which we have read access.

The complication with removing the ability to stat an arbitrary path is that without extra work you get into a situation where you can stat a leaf directory, but not it's parent. e.g. stat("/foo/bar") succeeds while stat("/foo") fails with errno == EPERM.

The only place we currently run into this is the utility process where the file system is off limits except for one directory.

This causes problems in 2 places:
1) DirectoryExists() works it's way from / down to the leaf directory stating each directory as it goes.
2) The extension installation code calls realpath() which fails if it can't stat parent directories.

The fix for the above is to explictly allow stating parent directories.  We achieve this in the sandbox code by adding a function which generates the appropriate sandbox syntax.

This CL also contains unit tests for the above functionality and re-enables it [bug 56765, the underlying issue appears to be unrelated to the test and previously fixed].

BUG=42989, 56765
TEST=Chrome should continue to render web pages correctly, installing extensions and themes should continue to work on OS X.

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

[jeremy, 2010-10-21 15:19:48]

Local testing shows this appears not to break anything major, but I'd like to
release this early to allow sufficient bake time on the Dev channel before it
goes to stable.

[Mark Mentovai, 2010-10-21 15:31:27]

LGTM

[jeremy, 2010-10-25 17:01:46]

Updated, could you take another look?

[Mark Mentovai, 2010-10-25 18:00:20]

http://codereview.chromium.org/4044002/diff/9001/10003
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/4044002/diff/9001/10003#newcode250
chrome/common/sandbox_mac.mm:250: // The main complication in this function is
that we whitelist directories
Don’t say we in comments. 257 also. Clean it up.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode256
chrome/common/sandbox_mac.mm:256: // The solution to this is to allow stating
the parent directories themselves
statting

http://codereview.chromium.org/4044002/diff/9001/10003#newcode289
chrome/common/sandbox_mac.mm:289: subdir_escaped.c_str());
Odd indent.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode295
chrome/common/sandbox_mac.mm:295: // Finally append the leaf directory.  Unlike
it's parents (which we only
we

http://codereview.chromium.org/4044002/diff/9001/10003#newcode310
chrome/common/sandbox_mac.mm:310: allowed_dir_escaped.c_str());
Odd indent. Suggest wrapping the line at base::SysUTF8ToNSString.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode312
chrome/common/sandbox_mac.mm:312: [sandbox_command
stringByAppendingFormat:@"(regex #\"%@\") )",
This regex won’t end with the ($|/) thing, will it?

Shouldn’t it?

http://codereview.chromium.org/4044002/diff/9001/10004
File chrome/common/sandbox_mac_diraccess_unittest.mm (right):

http://codereview.chromium.org/4044002/diff/9001/10004#newcode228
chrome/common/sandbox_mac_diraccess_unittest.mm:228: FilePath denied_file2 = 
allowed_parent_dir.Append(tricky_filename);
Also test that we can’t access any siblings of allowed_parent_dir that have
allowed_parent_dir as a substring. For example, if we permit access to and
within /tmp/permitted, we should still reject access attempts to
/tmp/permitted_just_kidding.

http://codereview.chromium.org/4044002/diff/9001/10005
File chrome/renderer/renderer.sb (right):

http://codereview.chromium.org/4044002/diff/9001/10005#newcode10
chrome/renderer/renderer.sb:10: (allow file-read-data file-read-metadata (regex
#"^/System/Library/Fonts($|/)"))  ; 10.5.6
I saw you use file-read-* elsewhere. Does that work here? Are there other things
that can be read beyond -data and -metadata? Should we standardize on one or the
other?

[jeremy, 2010-10-25 18:56:35]

All fixed, ready for another look.

http://codereview.chromium.org/4044002/diff/9001/10003
File chrome/common/sandbox_mac.mm (right):

http://codereview.chromium.org/4044002/diff/9001/10003#newcode250
chrome/common/sandbox_mac.mm:250: // The main complication in this function is
that we whitelist directories
On 2010/10/25 18:00:20, Mark Mentovai wrote:
> Don’t say we in comments. 257 also. Clean it up.

Done.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode256
chrome/common/sandbox_mac.mm:256: // The solution to this is to allow stating
the parent directories themselves
On 2010/10/25 18:00:20, Mark Mentovai wrote:
> statting

Done.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode289
chrome/common/sandbox_mac.mm:289: subdir_escaped.c_str());
On 2010/10/25 18:00:20, Mark Mentovai wrote:
> Odd indent.

Done.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode295
chrome/common/sandbox_mac.mm:295: // Finally append the leaf directory.  Unlike
it's parents (which we only
On 2010/10/25 18:00:20, Mark Mentovai wrote:
> we

Done.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode310
chrome/common/sandbox_mac.mm:310: allowed_dir_escaped.c_str());
On 2010/10/25 18:00:20, Mark Mentovai wrote:
> Odd indent. Suggest wrapping the line at base::SysUTF8ToNSString.

Done.

http://codereview.chromium.org/4044002/diff/9001/10003#newcode312
chrome/common/sandbox_mac.mm:312: [sandbox_command
stringByAppendingFormat:@"(regex #\"%@\") )",
QuoteStringForRegex() does this implicitly.

http://codereview.chromium.org/4044002/diff/9001/10004
File chrome/common/sandbox_mac_diraccess_unittest.mm (right):

http://codereview.chromium.org/4044002/diff/9001/10004#newcode228
chrome/common/sandbox_mac_diraccess_unittest.mm:228: FilePath denied_file2 = 
allowed_parent_dir.Append(tricky_filename);
On 2010/10/25 18:00:20, Mark Mentovai wrote:
> Also test that we can’t access any siblings of allowed_parent_dir that have
> allowed_parent_dir as a substring. For example, if we permit access to and
> within /tmp/permitted, we should still reject access attempts to
> /tmp/permitted_just_kidding.

Done.

http://codereview.chromium.org/4044002/diff/9001/10005
File chrome/renderer/renderer.sb (right):

http://codereview.chromium.org/4044002/diff/9001/10005#newcode10
chrome/renderer/renderer.sb:10: (allow file-read-data file-read-metadata (regex
#"^/System/Library/Fonts($|/)"))  ; 10.5.6
file-read-data, file-read-metadata & file-read-xattr are the available options.

Enabling all of them seems better, esp. since the * syntax seems more
future-proof.

[Mark Mentovai, 2010-10-25 20:07:47]

LGTM