Define a bootstrap sandbox policy for renderer processes and enable it.

Define a bootstrap sandbox policy for renderer processes and enable it.

BUG=367863
R=avi@chromium.org, mark@chromium.org

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

[Avi (use Gerrit), 2014-06-19 23:59:48]

lgtm

https://codereview.chromium.org/341073005/diff/40001/content/browser/bootstra...
File content/browser/bootstrap_sandbox_mac.cc (right):

https://codereview.chromium.org/341073005/diff/40001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:130: void
BootstrapSandboxPolicy::AddBaselinePolicy(
This is eventually going to be called by other policy registration functions?

https://codereview.chromium.org/341073005/diff/40001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:145: base::mac::IsOSMavericks();
IsOSMavericksOrEarlier?

[Robert Sesek, 2014-06-20 14:47:20]

https://codereview.chromium.org/341073005/diff/40001/content/browser/bootstra...
File content/browser/bootstrap_sandbox_mac.cc (right):

https://codereview.chromium.org/341073005/diff/40001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:130: void
BootstrapSandboxPolicy::AddBaselinePolicy(
On 2014/06/19 23:59:47, Avi wrote:
> This is eventually going to be called by other policy registration functions?

Yes. Utility and PPAPI types will both have policies.

https://codereview.chromium.org/341073005/diff/40001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:145: base::mac::IsOSMavericks();
On 2014/06/19 23:59:47, Avi wrote:
> IsOSMavericksOrEarlier?

Done. Wasn't available when I needed it :).

[Mark Mentovai, 2014-06-20 16:05:57]

LGTM

https://codereview.chromium.org/341073005/diff/60001/content/browser/bootstra...
File content/browser/bootstrap_sandbox_mac.cc (right):

https://codereview.chromium.org/341073005/diff/60001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:134: // Allow connecting to the
MachBroker to get the new child's task port.
Because this function is concerned with setting up the sandbox policy for a
child process, this comment is sort of misleading. It makes it sound like the
child needs to connect to the MachBroker to get its own task port. In reality,
it needs to talk to the MachBroker so that the browser can get the child’s task
port.

https://codereview.chromium.org/341073005/diff/60001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:135:
rules[MachBroker::GetMachPortName()] = sandbox::Rule(sandbox::POLICY_ALLOW);
For the future, it might be nice to have something like
sandbox::POLICY_ALLOW_ONCE, which would allow just one lookup, and deny future
ones from the process. This rule would be a good place for that policy.

[Robert Sesek, 2014-06-20 17:32:19]

https://codereview.chromium.org/341073005/diff/60001/content/browser/bootstra...
File content/browser/bootstrap_sandbox_mac.cc (right):

https://codereview.chromium.org/341073005/diff/60001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:134: // Allow connecting to the
MachBroker to get the new child's task port.
On 2014/06/20 16:05:57, Mark Mentovai wrote:
> Because this function is concerned with setting up the sandbox policy for a
> child process, this comment is sort of misleading. It makes it sound like the
> child needs to connect to the MachBroker to get its own task port. In reality,
> it needs to talk to the MachBroker so that the browser can get the child’s
task
> port.

Done.

https://codereview.chromium.org/341073005/diff/60001/content/browser/bootstra...
content/browser/bootstrap_sandbox_mac.cc:135:
rules[MachBroker::GetMachPortName()] = sandbox::Rule(sandbox::POLICY_ALLOW);
On 2014/06/20 16:05:57, Mark Mentovai wrote:
> For the future, it might be nice to have something like
> sandbox::POLICY_ALLOW_ONCE, which would allow just one lookup, and deny future
> ones from the process. This rule would be a good place for that policy.

That's an interesting idea. Would require a separate book keeping table in the
sandbox, though.

[Robert Sesek, 2014-06-20 17:32:25]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2014-06-20 17:42:22]

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

[Mark Mentovai, 2014-06-20 18:09:41]

Another nice thing to do would be to make a send-once right from the send right
and move that to the caller, but Mach doesn’t let you do that. You could provide
a proxying server to manage the send right and return a new send-once right to
the caller, but I that might be getting to the point of overkill.

[Robert Sesek, 2014-06-20 21:43:13]

Committed patchset #4 manually as r278831 (presubmit successful).

[jackhou1, 2014-06-23 03:37:10]

A revert of this CL has been created in
https://codereview.chromium.org/345373002/ by jackhou@chromium.org.

The reason for reverting is: I suspect this is breaking
ESCDoesNotLeaveFullscreenDOM. It's the only CL in that build that touches Mac,
(none of them seem related to fullscreen behavior).

http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%20Tests%20%28....