mac: Make the (10.6-only) sandbox hole for the components build smaller

mac: Make the (10.6-only) sandbox hole for the components build smaller

Part 2/2 of the changes requested by jeremy at
https://chromiumcodereview.appspot.com/10389047

Previously, the components build added (allow file-read-metadata) to
the sandbox on 10.6. With this patch, only all parent directories of the
bundle executable get this exception.

BUG=127465
TEST=component build still runs

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

[Nico, 2012-06-05 17:54:20]

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
File content/common/sandbox_mac.h (right):

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
content/common/sandbox_mac.h:155: static FilePath GetCanonicalSandboxPath(const
FilePath& path);
This is a behavior-preserving refactoring to make the calling sites a bit
shorter.

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
File content/common/sandbox_mac.mm (right):

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
content/common/sandbox_mac.mm:110: NSString* Sandbox::AllowMetadataForPath(const
FilePath& allowed_path) {
This was extracted almost unchanged from below. (I called out the one change.)

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
content/common/sandbox_mac.mm:114: for (FilePath path = allowed_path;
Note that this now starts at |allowed_path|, not at |allowed_path.DirName()|
like it did previously. Since the final component gets full read permissions at
the old calling site, giving it file-read-metadata permissions in addition isn't
any less secure.

[Nico, 2012-06-06 21:27:57]

ping

[jeremy, 2012-06-07 10:00:13]

LGTM

[commit-bot: I haz the power, 2012-06-07 14:35:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10539009/1

[commit-bot: I haz the power, 2012-06-07 14:35:08]

Presubmit check for 10539009-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common

[Avi (use Gerrit), 2012-06-07 14:50:01]

LGTM with super important comment nits :)

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
File content/common/sandbox_mac.h (right):

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
content/common/sandbox_mac.h:128: // Returns a (allow file-read-metadata) rule
for |allowed_dir| and all its
incorrect variable name

also, wouldn't that be "an (allow file-read-metadata) rule"?

[Nico, 2012-06-07 14:59:56]

ty!

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
File content/common/sandbox_mac.h (right):

https://chromiumcodereview.appspot.com/10539009/diff/1/content/common/sandbox...
content/common/sandbox_mac.h:128: // Returns a (allow file-read-metadata) rule
for |allowed_dir| and all its
On 2012/06/07 14:50:01, Avi wrote:
> incorrect variable name
> 
> also, wouldn't that be "an (allow file-read-metadata) rule"?

Done.

[commit-bot: I haz the power, 2012-06-07 15:00:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10539009/4002

[commit-bot: I haz the power, 2012-06-07 15:16:17]

Try job failure for 10539009-4002 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-07 15:25:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10539009/2002

[commit-bot: I haz the power, 2012-06-07 16:38:23]

Change committed as 141020

[Jiang Jiang, 2014-08-13 10:48:08]

https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac...
content/common/sandbox_mac.mm:114: for (FilePath path = allowed_path;
This seems wrong as the original code is:

for (FilePath path = allow_dir_canonical.DirName())

missing .DirName() here will result the comparison below:

path.value() != last_path.value()

to become false in the first iteration of the loop and subpaths won't contain
anything.

[Nico, 2014-08-13 15:43:17]

https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac...
content/common/sandbox_mac.mm:114: for (FilePath path = allowed_path;
On 2014/08/13 10:48:08, Jiang wrote:
> This seems wrong as the original code is:
> 
> for (FilePath path = allow_dir_canonical.DirName())
> 
> missing .DirName() here will result the comparison below:
> 
> path.value() != last_path.value()
> 
> to become false in the first iteration of the loop
> and subpaths won't contain
> anything.

Hm, that seems right. I guess noone uses the components build on 10.6 anymore,
then? Should we just rip this out?

[Jiang Jiang, 2014-08-13 15:47:40]

On 2014/08/13 15:43:17, Nico (very away) wrote:
>
https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac.mm
> File content/common/sandbox_mac.mm (right):
> 
>
https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac...
> content/common/sandbox_mac.mm:114: for (FilePath path = allowed_path;
> On 2014/08/13 10:48:08, Jiang wrote:
> > This seems wrong as the original code is:
> > 
> > for (FilePath path = allow_dir_canonical.DirName())
> > 
> > missing .DirName() here will result the comparison below:
> > 
> > path.value() != last_path.value()
> > 
> > to become false in the first iteration of the loop
> > and subpaths won't contain
> > anything.
> 
> Hm, that seems right. I guess noone uses the components build on 10.6 anymore,
> then? Should we just rip this out?

AllowMetadataForPath() is also used by BuildAllowDirectoryAccessSandboxString()
though, can that usage also be removed?

[Nico, 2014-08-13 15:53:02]

On Wed, Aug 13, 2014 at 8:47 AM, <jiangj@opera.com> wrote:

> On 2014/08/13 15:43:17, Nico (very away) wrote:
>
> https://codereview.chromium.org/10539009/diff/2002/
> content/common/sandbox_mac.mm
>
>> File content/common/sandbox_mac.mm (right):
>>
>
>
> https://codereview.chromium.org/10539009/diff/2002/
> content/common/sandbox_mac.mm#newcode114
>
>> content/common/sandbox_mac.mm:114: for (FilePath path = allowed_path;
>> On 2014/08/13 10:48:08, Jiang wrote:
>> > This seems wrong as the original code is:
>> >
>> > for (FilePath path = allow_dir_canonical.DirName())
>> >
>> > missing .DirName() here will result the comparison below:
>> >
>> > path.value() != last_path.value()
>> >
>> > to become false in the first iteration of the loop
>> > and subpaths won't contain
>> > anything.
>>
>
>  Hm, that seems right. I guess noone uses the components build on 10.6
>> anymore,
>> then? Should we just rip this out?
>>
>
> AllowMetadataForPath() is also used by BuildAllowDirectoryAccessSandb
> oxString()
> though, can that usage also be removed?
>

Ooh, the function currently builds the string `(allow file-read-metadata )`
– I wonder if that just generally allows reading of file metadata? Do
things still work if you remove the call to AllowMetadataForPath()? If not,
that means this CL added a fairly large metadata hole.


>
> https://codereview.chromium.org/10539009/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jiang Jiang, 2014-08-13 15:53:37]

On 2014/08/13 15:47:40, Jiang wrote:
> On 2014/08/13 15:43:17, Nico (very away) wrote:
> >
>
https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac.mm
> > File content/common/sandbox_mac.mm (right):
> > 
> >
>
https://codereview.chromium.org/10539009/diff/2002/content/common/sandbox_mac...
> > content/common/sandbox_mac.mm:114: for (FilePath path = allowed_path;
> > On 2014/08/13 10:48:08, Jiang wrote:
> > > This seems wrong as the original code is:
> > > 
> > > for (FilePath path = allow_dir_canonical.DirName())
> > > 
> > > missing .DirName() here will result the comparison below:
> > > 
> > > path.value() != last_path.value()
> > > 
> > > to become false in the first iteration of the loop
> > > and subpaths won't contain
> > > anything.
> > 
> > Hm, that seems right. I guess noone uses the components build on 10.6
anymore,
> > then? Should we just rip this out?
> 
> AllowMetadataForPath() is also used by
BuildAllowDirectoryAccessSandboxString()
> though, can that usage also be removed?

Looks like it's still used by utility process, and that usage is still in
chrome:

https://code.google.com/p/chromium/codesearch#search/&q=SetExposedDir&sq=pack...

[Jiang Jiang, 2014-08-13 15:56:06]

On 2014/08/13 15:53:02, Nico (very away) wrote:
> On Wed, Aug 13, 2014 at 8:47 AM, <mailto:jiangj@opera.com> wrote:
> 
> > On 2014/08/13 15:43:17, Nico (very away) wrote:
> >
> > https://codereview.chromium.org/10539009/diff/2002/
> > content/common/sandbox_mac.mm
> >
> >> File content/common/sandbox_mac.mm (right):
> >>
> >
> >
> > https://codereview.chromium.org/10539009/diff/2002/
> > content/common/sandbox_mac.mm#newcode114
> >
> >> content/common/sandbox_mac.mm:114: for (FilePath path = allowed_path;
> >> On 2014/08/13 10:48:08, Jiang wrote:
> >> > This seems wrong as the original code is:
> >> >
> >> > for (FilePath path = allow_dir_canonical.DirName())
> >> >
> >> > missing .DirName() here will result the comparison below:
> >> >
> >> > path.value() != last_path.value()
> >> >
> >> > to become false in the first iteration of the loop
> >> > and subpaths won't contain
> >> > anything.
> >>
> >
> >  Hm, that seems right. I guess noone uses the components build on 10.6
> >> anymore,
> >> then? Should we just rip this out?
> >>
> >
> > AllowMetadataForPath() is also used by BuildAllowDirectoryAccessSandb
> > oxString()
> > though, can that usage also be removed?
> >
> 
> Ooh, the function currently builds the string `(allow file-read-metadata )`
> – I wonder if that just generally allows reading of file metadata? Do
> things still work if you remove the call to AllowMetadataForPath()? If not,
> that means this CL added a fairly large metadata hole.

That generally allows reading all the file metadata in the entire filesystem,
yes.

(I discovered this during debugging of Opera's Core Audio based decoding code
which requires additional meta-data read access for on 10.6, and it only works
with component build because component builds has this hole that accidentally
granted all metadata access.)

[Jiang Jiang, 2014-08-13 16:00:35]

https://codereview.chromium.org/472513002/ posted to correct the loop logic.