Initialize the bootstrap sandbox in the browser process.

Initialize the bootstrap sandbox in the browser process.

This wires up the ChildProcessLauncher and SandboxedProcessLauncherDelegate.
No restrictive policies are currently registered or enforced, i.e. renderers
should not be locked down. However, to keep NPAPI plugins working, an allow-by-
default policy exists.

BUG=367863
R=jam@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276026

[Mark Mentovai, 2014-05-30 21:01:12]

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
content/public/common/sandboxed_process_launcher_delegate.h:66: // Gets the Mac
SandboxType to enforce on the process. Return -1 for no
What’s a SandboxType? It’s just a registered policy ID, right? You should say
more about this in the comment, because the return value is just “int” and
SandboxType isn’t actually a thing. Implementers will need to know what to do
here.

Maybe the type returned here should be a typedef, like SandboxPolicyID? You
wouldn’t have to say as much in this comment here then.

[Robert Sesek, 2014-05-30 21:05:11]

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
content/public/common/sandboxed_process_launcher_delegate.h:66: // Gets the Mac
SandboxType to enforce on the process. Return -1 for no
On 2014/05/30 21:01:13, Mark Mentovai wrote:
> What’s a SandboxType? It’s just a registered policy ID, right? You should say
> more about this in the comment, because the return value is just “int” and
> SandboxType isn’t actually a thing. Implementers will need to know what to do
> here.

Yes, it is:

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

[Mark Mentovai, 2014-05-30 21:07:43]

Then why is this returning an int instead of a SandboxType?

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

[jam, 2014-05-30 21:11:55]

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
content/public/common/sandboxed_process_launcher_delegate.h:66: // Gets the Mac
SandboxType to enforce on the process. Return -1 for no
On 2014/05/30 21:05:11, rsesek wrote:
> On 2014/05/30 21:01:13, Mark Mentovai wrote:
> > What’s a SandboxType? It’s just a registered policy ID, right? You should
say
> > more about this in the comment, because the return value is just “int” and
> > SandboxType isn’t actually a thing. Implementers will need to know what to
do
> > here.
> 
> Yes, it is:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...

can this be SandboxType instead of "int" then?

[Robert Sesek, 2014-05-30 21:17:35]

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/303293002/diff/70001/content/public/common/sa...
content/public/common/sandboxed_process_launcher_delegate.h:66: // Gets the Mac
SandboxType to enforce on the process. Return -1 for no
On 2014/05/30 21:11:55, jam wrote:
> On 2014/05/30 21:05:11, rsesek wrote:
> > On 2014/05/30 21:01:13, Mark Mentovai wrote:
> > > What’s a SandboxType? It’s just a registered policy ID, right? You should
> say
> > > more about this in the comment, because the return value is just “int” and
> > > SandboxType isn’t actually a thing. Implementers will need to know what to
> do
> > > here.
> > 
> > Yes, it is:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...
> 
> can this be SandboxType instead of "int" then?

Yeah, if I add an INVALID enum value. Done.

[Mark Mentovai, 2014-05-30 21:20:38]

LGTM

[Avi (use Gerrit), 2014-05-30 22:10:12]

Drive-by nit.

https://codereview.chromium.org/303293002/diff/110001/content/browser/bootstr...
File content/browser/bootstrap_sandbox_mac.h (right):

https://codereview.chromium.org/303293002/diff/110001/content/browser/bootstr...
content/browser/bootstrap_sandbox_mac.h:14: // Returns the singleton instance of
the BootstrapSandox. The returned object
s/BootstrapSandox/BootstrapSandbox/

[jam, 2014-05-30 23:05:29]

lgtm

[Robert Sesek, 2014-06-05 20:24:37]

PTAL. Patch set 4 broke NPAPI plugins. I needed to rebase against trunk to pick
up some dependent changes, but the diffs to patch set 4 are still quite
readable.

NPAPI plugins broke because they cannot be sandboxed *at all*, so this requires
a small workaround. BootstrapSandboxPolicy is now a BrowserChildProcessObserver
to get process death notifications, to clean up the sandbox mappings. I also
added a function ShouldEnableBootstrapSandbox() to make it easy to disable the
sandbox in a pinch.

Details on the NPAPI workaround:
Because NPAPI plugins need unfettered access to the real bootstrap port owned by
launchd, I've created a bootstrap sandbox policy for NPAPI plugins. This policy
allows plugins to look up the original bootstrap port, and then in
plugin_main_mac.mm, reset the bootstrap port to the one retrieved from the
sandbox.

[Mark Mentovai, 2014-06-05 20:32:28]

LGTM % usage-describing nit

https://codereview.chromium.org/303293002/diff/150001/content/common/sandbox_...
File content/common/sandbox_init_mac.cc (right):

https://codereview.chromium.org/303293002/diff/150001/content/common/sandbox_...
content/common/sandbox_init_mac.cc:80:
"org.chromium.sandbox.real_bootstrap_server";
You’re never ever ever gonna bootstrap_register this with the real bootstrap
server, are you? At the very least, the .h should have a comment saying so. But
you might also want to rename this variable to make it clear that it’s not a
valid thing to look up with the real bootstrap server, it’s a pseudo-name.

[Robert Sesek, 2014-06-05 20:44:27]

https://codereview.chromium.org/303293002/diff/150001/content/common/sandbox_...
File content/common/sandbox_init_mac.cc (right):

https://codereview.chromium.org/303293002/diff/150001/content/common/sandbox_...
content/common/sandbox_init_mac.cc:80:
"org.chromium.sandbox.real_bootstrap_server";
On 2014/06/05 20:32:28, Mark Mentovai wrote:
> You’re never ever ever gonna bootstrap_register this with the real bootstrap
> server, are you? At the very least, the .h should have a comment saying so.
But
> you might also want to rename this variable to make it clear that it’s not a
> valid thing to look up with the real bootstrap server, it’s a pseudo-name.

Done. Renamed to kBootstrapPortNameForNPAPIPlugins.

[Mark Mentovai, 2014-06-05 20:48:46]

Good. LGTM.

[jam, 2014-06-09 19:59:38]

slgtm

[Robert Sesek, 2014-06-09 20:37:27]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2014-06-09 20:38:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsesek@chromium.org/303293002/170001

[commit-bot: I haz the power, 2014-06-10 09:57:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-10 14:50:51]

Change committed as 276026