Refactor configuration of sandboxes - first steps

Refactor configuration of sandboxes - first steps

See
https://docs.google.com/document/d/1H-hCsIcMsAEP0fWHimbuiNA-Hc9eXEmR94eb-2RQAhA/edit?usp=sharing
for background.

This moves all process type dependent decisions on how to create
Linux processes (not how to sandbox them once created, not Android)
into the launch delegates and makes the arguments to the
ChildProcessLauncher constructor and
BrowserChildProcessHostImpl::Launch OS independent.

BUG=none

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

[aberent, 2014-02-26 15:08:34]

mseaborn@chromium.org: Please review changes in nacl component

jam@chromium.org: Please review changes in content.

jln@chromium.org: Security review please.

[jam, 2014-02-26 19:47:51]

https://codereview.chromium.org/177863002/diff/180001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:152: #elif defined(OS_POSIX)
why have two implementations in this file instead of just one with ifdefs? the
latter is how things are usually done. especially if one of the motivations here
is to have more code sharing between different platforms

https://codereview.chromium.org/177863002/diff/180001/content/browser/child_p...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/child_p...
content/browser/child_process_launcher.cc:173: bool launch_elevated = delegate
-> LaunchElevated();
nit: no space

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:182: ChildProcessHost* /*host*/):
nit: here and in other places, within content code doesn't comment out names for
unused parameters. i think consistency would be good

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:183: cmd_line_(cmd_line) {}
nit: indentation

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:284: class
GpuSandboxedProcessLauncherDelegate
ditto, we should have one implementation of this file per cc file

https://codereview.chromium.org/177863002/diff/180001/content/browser/plugin_...
File content/browser/plugin_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/plugin_...
content/browser/plugin_process_host.cc:98: class
PluginSandboxedProcessLauncherDelegate
ditto

https://codereview.chromium.org/177863002/diff/180001/content/browser/ppapi_p...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/ppapi_p...
content/browser/ppapi_plugin_process_host.cc:73: // NOTE: changes to this class
need to be reviewed by the security team.
ditto

https://codereview.chromium.org/177863002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:306: // NOTE: changes
to this class need to be reviewed by the security team.
ditto

https://codereview.chromium.org/177863002/diff/180001/content/browser/utility...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/utility...
content/browser/utility_process_host_impl.cc:62: class
UtilitySandboxedProcessLauncherDelegate :
ditto

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:82: // NOTE: changes to this
class need to be reviewed by the security team.
ditto

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:113: }
this method body looks like a copy of the code that's still there but which only
changes flags. why not keep the logic of use_zygote there, and pass it as an
argument to this class?

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:30: }
nit: keep on one line as before, that's the convention for all the headers in
content/public

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:35: virtual bool
LaunchElevated();
nit: LaunchElevated could be confused in that content is telling the delegate to
launch something itself. ShouldLaunchElevated would be clearer (i.e. like
ShouldSandbox below)

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:58: virtual bool
UseZygote();
nit: ditto re naming, this should be ShouldUseZygote

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:64: virtual int
IpcFd() = 0;
nit: GetIPCFD

[jam, 2014-02-26 22:45:13]

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:40: virtual void
ShouldSandbox(bool* in_sandbox) {}
oh i forgot to mention, I had added this method this way instead of bool
ShouldSandbox() to avoid a cc file. since you're adding one now, if you don't
mind please change this method to a more natural form

[aberent, 2014-02-28 08:51:06]

https://codereview.chromium.org/177863002/diff/180001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:152: #elif defined(OS_POSIX)
On 2014/02/26 19:47:52, jam wrote:
> why have two implementations in this file instead of just one with ifdefs? the
> latter is how things are usually done. especially if one of the motivations
here
> is to have more code sharing between different platforms

Done. I have tried various versions of this, and whatever one does it seems to
get a bit messy because of the need for different initializers in the different
implementations of the constructor. I would welcome your opinion on the way I do
it in the next patch set.

https://codereview.chromium.org/177863002/diff/180001/content/browser/child_p...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/child_p...
content/browser/child_process_launcher.cc:173: bool launch_elevated = delegate
-> LaunchElevated();
On 2014/02/26 19:47:52, jam wrote:
> nit: no space

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:182: ChildProcessHost* /*host*/):
On 2014/02/26 19:47:52, jam wrote:
> nit: here and in other places, within content code doesn't comment out names
for
> unused parameters. i think consistency would be good

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:183: cmd_line_(cmd_line) {}
On 2014/02/26 19:47:52, jam wrote:
> nit: indentation

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/gpu/gpu...
content/browser/gpu/gpu_process_host.cc:284: class
GpuSandboxedProcessLauncherDelegate
On 2014/02/26 19:47:52, jam wrote:
> ditto, we should have one implementation of this file per cc file

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/plugin_...
File content/browser/plugin_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/plugin_...
content/browser/plugin_process_host.cc:98: class
PluginSandboxedProcessLauncherDelegate
On 2014/02/26 19:47:52, jam wrote:
> ditto

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/ppapi_p...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/ppapi_p...
content/browser/ppapi_plugin_process_host.cc:73: // NOTE: changes to this class
need to be reviewed by the security team.
On 2014/02/26 19:47:52, jam wrote:
> ditto

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:306: // NOTE: changes
to this class need to be reviewed by the security team.
On 2014/02/26 19:47:52, jam wrote:
> ditto

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/utility...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/utility...
content/browser/utility_process_host_impl.cc:62: class
UtilitySandboxedProcessLauncherDelegate :
On 2014/02/26 19:47:52, jam wrote:
> ditto

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:82: // NOTE: changes to this
class need to be reviewed by the security team.
On 2014/02/26 19:47:52, jam wrote:
> ditto

Done.

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:113: }
On 2014/02/26 19:47:52, jam wrote:
> this method body looks like a copy of the code that's still there but which
only
> changes flags. why not keep the logic of use_zygote there, and pass it as an
> argument to this class?
Done. BTW I am slightly puzzled as to why there isn't equivalent code to this
for the other process types. The documentation of these flags suggest that they
apply to all child processes. If this should apply to all processes then it
seems it could be moved down into ChildProcessLauncher::LaunchInternal. This is,
however, probably a matter for a different CL.

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:30: }
On 2014/02/26 19:47:52, jam wrote:
> nit: keep on one line as before, that's the convention for all the headers in
> content/public

Done.

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:35: virtual bool
LaunchElevated();
On 2014/02/26 19:47:52, jam wrote:
> nit: LaunchElevated could be confused in that content is telling the delegate
to
> launch something itself. ShouldLaunchElevated would be clearer (i.e. like
> ShouldSandbox below)

Done.

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:40: virtual void
ShouldSandbox(bool* in_sandbox) {}
On 2014/02/26 22:45:14, jam wrote:
> oh i forgot to mention, I had added this method this way instead of bool
> ShouldSandbox() to avoid a cc file. since you're adding one now, if you don't
> mind please change this method to a more natural form

Done.

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:58: virtual bool
UseZygote();
On 2014/02/26 19:47:52, jam wrote:
> nit: ditto re naming, this should be ShouldUseZygote

Done.

https://codereview.chromium.org/177863002/diff/180001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:64: virtual int
IpcFd() = 0;
On 2014/02/26 19:47:52, jam wrote:
> nit: GetIPCFD

Done.

[aberent, 2014-02-28 08:52:57]

cpu@chromium.org: Please review changes in sandbox_win.cc as owner.

[jam, 2014-02-28 18:07:41]

lgtm

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
File content/browser/worker_host/worker_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/180001/content/browser/worker_...
content/browser/worker_host/worker_process_host.cc:113: }
On 2014/02/28 08:51:07, aberent wrote:
> On 2014/02/26 19:47:52, jam wrote:
> > this method body looks like a copy of the code that's still there but which
> only
> > changes flags. why not keep the logic of use_zygote there, and pass it as an
> > argument to this class?
> Done. BTW I am slightly puzzled as to why there isn't equivalent code to this
> for the other process types. The documentation of these flags suggest that
they
> apply to all child processes. If this should apply to all processes then it
> seems it could be moved down into ChildProcessLauncher::LaunchInternal. This
is,
> however, probably a matter for a different CL.

it's probably an oversight..

https://codereview.chromium.org/177863002/diff/190001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/190001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:171: 
nit: no blank line

https://codereview.chromium.org/177863002/diff/190001/content/browser/ppapi_p...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/190001/content/browser/ppapi_p...
content/browser/ppapi_plugin_process_host.cc:45: : is_broker_(is_broker) {}
nit: since is_broker_ is used for both ifdefs, i'd take this out of the ifdefs
since common parts shouldn't be duplicated.

https://codereview.chromium.org/177863002/diff/190001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/190001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:317: #endif  // OS_WIN
nit: blank line before private

https://codereview.chromium.org/177863002/diff/190001/content/browser/utility...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/190001/content/browser/utility...
content/browser/utility_process_host_impl.cc:45: : exposed_dir_(exposed_dir),
nit: bring out the shared part out of the ifdefs

[aberent, 2014-02-28 21:17:27]

https://codereview.chromium.org/177863002/diff/190001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/190001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:171: 
On 2014/02/28 18:07:43, jam wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/177863002/diff/190001/content/browser/ppapi_p...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/190001/content/browser/ppapi_p...
content/browser/ppapi_plugin_process_host.cc:45: : is_broker_(is_broker) {}
On 2014/02/28 18:07:43, jam wrote:
> nit: since is_broker_ is used for both ifdefs, i'd take this out of the ifdefs
> since common parts shouldn't be duplicated.

Done. Had to put in some slightly strange line breaks to do it. Final layout is
as recommended by clang_format.

https://codereview.chromium.org/177863002/diff/190001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/190001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:317: #endif  // OS_WIN
On 2014/02/28 18:07:43, jam wrote:
> nit: blank line before private

Done.

https://codereview.chromium.org/177863002/diff/190001/content/browser/utility...
File content/browser/utility_process_host_impl.cc (right):

https://codereview.chromium.org/177863002/diff/190001/content/browser/utility...
content/browser/utility_process_host_impl.cc:45: : exposed_dir_(exposed_dir),
On 2014/02/28 18:07:43, jam wrote:
> nit: bring out the shared part out of the ifdefs

Done.

[jam, 2014-02-28 21:50:37]

thanks, nice cleanup!

[Mark Seaborn, 2014-02-28 22:12:23]

Please fill out "BUG=" in the commit message, if only with "none".  LGTM for
components/nacl.

https://codereview.chromium.org/177863002/diff/210001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/210001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:190: 
Nit: don't put an empty line at the end of the class decl

https://codereview.chromium.org/177863002/diff/210001/content/public/common/s...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/177863002/diff/210001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:62: // Return the
File descriptor for the IPC channel.
Nit: lower case "file"?

[jam, 2014-03-03 20:31:41]

On 2014/02/28 22:12:23, Mark Seaborn wrote:
> Please fill out "BUG=" in the commit message, if only with "none".  LGTM for
> components/nacl.

"bug=" or "bug=none" are just noise. see
http://dev.chromium.org/developers/contributing-code "If there's an associated
bug file, you should also add a BUG=bug_number line so they can find the
associated bug."

[Mark Seaborn, 2014-03-03 20:38:04]

On 3 March 2014 12:31, <jam@chromium.org> wrote:

> On 2014/02/28 22:12:23, Mark Seaborn wrote:
>
>> Please fill out "BUG=" in the commit message, if only with "none".  LGTM
>> for
>> components/nacl.
>>
>
> "bug=" or "bug=none" are just noise. see
> http://dev.chromium.org/developers/contributing-code "If there's an
> associated
> bug file, you should also add a BUG=bug_number line so they can find the
> associated bug."


Then leave out "BUG=" from the commit message entirely.  "git cl upload"
adds "BUG=" in an attempt to prompt developers to fill this out, but this
apparently doesn't work a lot of the time.

Cheers,
Mark

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

[aberent, 2014-03-05 10:24:32]

jln@, cpu@ - Any idea when you will be able to review this? Subject to me fixing
Mark's nits it is otherwise ready to land.

[cpu_(ooo_6.6-7.5), 2014-03-07 18:45:54]

lgtm

[jln (very slow on Chromium), 2014-03-07 20:18:42]

lgtm as well.

Note that the "Zygote = Setuid sandbox" equivalency needs to go away at some
point though. There is --disable-setuid-sandbox, and --no-sandbox which still
use the Zygote but not the setuid sandbox.

Also be aware that Linux / Chrome OS moving towards a new sandboxing model
(crbug.com/312380), so a lot of this code is changing now. Please talk to me
before touching anything deep in content/common/sandbox_linux/ or
content/zygote.

A lot of the headaches are around the fact that we need to support many Linux /
Chrome OS kernel versions with vastly different sandboxing features.

[aberent, 2014-03-12 21:58:58]

https://codereview.chromium.org/177863002/diff/210001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/177863002/diff/210001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:190: 
On 2014/02/28 22:12:24, Mark Seaborn wrote:
> Nit: don't put an empty line at the end of the class decl

Done.

https://codereview.chromium.org/177863002/diff/210001/content/public/common/s...
File content/public/common/sandboxed_process_launcher_delegate.h (right):

https://codereview.chromium.org/177863002/diff/210001/content/public/common/s...
content/public/common/sandboxed_process_launcher_delegate.h:62: // Return the
File descriptor for the IPC channel.
On 2014/02/28 22:12:24, Mark Seaborn wrote:
> Nit: lower case "file"?

Done.

[aberent, 2014-03-12 22:00:32]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-03-12 22:02:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/177863002/230001

[commit-bot: I haz the power, 2014-03-13 04:22:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 04:22:58]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, check_deps2git, chromeos_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
dbus_unittests, device_unittests, events_unittests, google_apis_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
keyboard_unittests, media_unittests, net_unittests, ppapi_unittests,
printing_unittests, sandbox_linux_unittests, sql_unittests, sync_unit_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[aberent, 2014-03-13 10:08:46]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-03-13 10:08:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/177863002/230001

[commit-bot: I haz the power, 2014-03-13 10:10:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 10:10:15]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[aberent, 2014-03-13 10:36:03]

The CQ bit was checked by aberent@chromium.org

[commit-bot: I haz the power, 2014-03-13 10:36:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/177863002/230001

[commit-bot: I haz the power, 2014-03-13 11:35:17]

Change committed as 256802