Add namespace sandbox class.

Add namespace sandbox class.

BUG=312380
Committed: https://crrev.com/8f235daf6d29e05f8a1949864da493e910a3ddd1
Cr-Commit-Position: refs/heads/master@{#314284}

[rickyz (no longer on Chrome), 2015-01-27 07:51:56]

rickyz@chromium.org changed reviewers:
+ jln@chromium.org

[rickyz (no longer on Chrome), 2015-01-27 07:51:57]

https://codereview.chromium.org/881733002/diff/1/sandbox/linux/services/names...
File sandbox/linux/services/namespace_sandbox.cc (right):

https://codereview.chromium.org/881733002/diff/1/sandbox/linux/services/names...
sandbox/linux/services/namespace_sandbox.cc:60:
SetEnvironForNamespaceType(environ, kSandboxUSERNSEnvironmentVarName,
If environmental variables are a little ugly, we could probably compare
/proc/ppid/ns/* and /proc/self/ns/* before chrooting instead (and then set some
global variables). What do you think?

[jln (very slow on Chromium), 2015-01-28 02:34:39]

I had to rush this review a little, so I haven't fully made up my mind, but I
thought I would send this early anyways.

We need to be very careful about ownership here, it's a bit confusing. The
base:: API doesn't make that very easy (we should consider changing it if we
have good ideas!).

One suggestion is to change the way this class work to become more of a
NameSpaceSandboxLauncher.

For LaunchOption, we could either claim ownership (take a scoped_fd) or hold its
own copy of LaunchOption (which is cheap to copy, it's a struct [but are we sure
it's always safe to copy?]).

One could construct a NameSpaceSandboxLauncher and call a LaunchProcess()
wrapper on it.

The typical LaunchProcess options could either be passed in our LaunchProcess()
wrapper or as NameSpaceSandboxLauncher constructor arguments.

Let me know what you think. These are just a few ideas. The general problem is
to think carefully about ownership. Another thing to think carefully about is
what "state" your class is in. You can track and update the state (Setup... has
been called, but Prepare hasn't been) and have a bunch of DCHECK), but ideally
the API can be simple enough that it's not needed.

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
File sandbox/linux/services/namespace_sandbox.cc (right):

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.cc:57: base::EnvironmentMap* environ =
&options->environ;
DCHECK environ?

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.cc:58:
SetEnvironForNamespaceType(environ, kSandboxUSERNSEnvironmentVarName,
For loop?

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.cc:76: read_fd_.reset();
This should destruct the ReadFromPipeDelegate instead, no?

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
File sandbox/linux/services/namespace_sandbox.h (right):

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.h:42: void
SetupLaunchOptions(base::LaunchOptions* options,
Maybe mention again that CanCreateProcessInNewUserNS must be true?

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.h:42: void
SetupLaunchOptions(base::LaunchOptions* options,
We should at least claim ownership of options. That's because we own the
PreExecDelegate, and both should be tied.

- You could take a scoped_ptr<LaunchOptions> here, which would guarantee that
the caller is doing the right thing, and it would make it clear that we're
taking ownership.
- We could simply take a LaunchOptions instead of a pointer and hold that copy a
a NamespaceSandbox member.

In this case, we should provide our own LaunchProcess() wrapper here, which
could also help us remove the explicit PrepareSandboxedProcess() step.

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.h:58: class ReadFromPipeDelegate :
public base::LaunchOptions::PreExecDelegate {
It's an edge case, but I would split to its own file for clarity.

https://chromiumcodereview.appspot.com/881733002/diff/20001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.h:66: int fd_;
ReadFromPipeDelegate should own this file descriptor. Let's make this a
ScopedFD.

[rickyz (no longer on Chrome), 2015-01-29 00:15:44]

Patchset #3 (id:40001) has been deleted

[rickyz (no longer on Chrome), 2015-01-29 00:57:48]

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_sandbox.cc (right):

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.cc:57: base::EnvironmentMap* environ =
&options->environ;
On 2015/01/28 02:34:39, jln wrote:
> DCHECK environ?
Don't think this is needed since environ isn't a pointer (seems LaunchOptions is
a little inconsistent about using pointers or embedding classes).

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.cc:58:
SetEnvironForNamespaceType(environ, kSandboxUSERNSEnvironmentVarName,
On 2015/01/28 02:34:39, jln wrote:
> For loop?

Done.

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.cc:76: read_fd_.reset();
On 2015/01/28 02:34:38, jln wrote:
> This should destruct the ReadFromPipeDelegate instead, no?
See the other comment about read_fd_ ownership - the summary is that I didn't
want ReadFromPipeDelegate to own it because ReadFromPipeDelegate is only
meaningful in the child, and ReadFromPipeDelegate won't ever be destroyed in the
child.

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_sandbox.h (right):

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.h:42: void
SetupLaunchOptions(base::LaunchOptions* options,
On 2015/01/28 02:34:39, jln wrote:
> We should at least claim ownership of options. That's because we own the
> PreExecDelegate, and both should be tied.
> 
> - You could take a scoped_ptr<LaunchOptions> here, which would guarantee that
> the caller is doing the right thing, and it would make it clear that we're
> taking ownership.
> - We could simply take a LaunchOptions instead of a pointer and hold that copy
a
> a NamespaceSandbox member.
> 
> In this case, we should provide our own LaunchProcess() wrapper here, which
> could also help us remove the explicit PrepareSandboxedProcess() step.

For now, I went with embedding the LaunchOptions in NamespaceSandbox. As things
are now, passing a scoped_ptr is a little deceptive because LaunchOptions may
contain pointers to outside objects. This API still isn't ideal - I currently
expose a mutable_launch_options where users may still set pointer fields (though
lifetime requirements are documented at least) because the nacl zygote code will
eventually need it.

Eventually, it might be worth changing LaunchOptions to avoid having so many
pointers to lists and such, but then pre_exec_delegate would still be
problematic, blech. Will think more about what kind of change I'd make to
LaunchOptions, but here's kind of a compromise for now.

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.h:58: class ReadFromPipeDelegate :
public base::LaunchOptions::PreExecDelegate {
On 2015/01/28 02:34:39, jln wrote:
> It's an edge case, but I would split to its own file for clarity.

This became unnecessary after the last refactoring.

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.h:66: int fd_;
On 2015/01/28 02:34:39, jln wrote:
> ReadFromPipeDelegate should own this file descriptor. Let's make this a
> ScopedFD.
I didn't want to make the delegate own the fd because ownership across fork
isn't so obvious. If this were a ScopedFd, then we would still need to
explicitly close the fd in the child because the delegate will never be
destroyed (since we'll execve).

[rickyz (no longer on Chrome), 2015-01-29 01:02:48]

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_sandbox.h (right):

https://codereview.chromium.org/881733002/diff/20001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.h:42: void
SetupLaunchOptions(base::LaunchOptions* options,
On 2015/01/28 02:34:39, jln wrote:
> Maybe mention again that CanCreateProcessInNewUserNS must be true?

Oops, missed this one, added a comment to Launch.

[jln (very slow on Chromium), 2015-01-30 20:12:28]

This looks pretty good. I think the ScopedFDs don't need to be part of the class
and that we can make better use of them as "scopers".

Outside of that this is fine, unless you think we can get a more simple API with
Launch() taking an already built LaunchOptions.

(sorry for the long review delay).

https://chromiumcodereview.appspot.com/881733002/diff/80001/sandbox/linux/ser...
File sandbox/linux/services/namespace_sandbox.cc (right):

https://chromiumcodereview.appspot.com/881733002/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.cc:113: write_fd_.reset();
Having to "remember" to do that is problematic.

read_fd_ and write_fd_ are mostly used in this function.

I would not make them class members and instead just have the ScopedFD in this
method.

Then you can have WriteUidGidMapAndUnblockProcess() take a ScopedFD (that you
can Pass()) and I believe that everything will look right, with "automatic"
closing of the ScopedFD.

https://chromiumcodereview.appspot.com/881733002/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.cc:127:
PCHECK(HANDLE_EINTR(write(write_fd_.get(), &kPipeValue, 1)) == 1);
If the child process gets killed or disappears for any reason, you'll get
SIGPIP-ed.

You could use send with MSG_NOSIGNAL. Or you could simply close the pipe to
unblock the child, but then the child won't immediately know if the parent died
or signaled it (which is probably no big deal).

https://chromiumcodereview.appspot.com/881733002/diff/80001/sandbox/linux/ser...
File sandbox/linux/services/namespace_sandbox.h (right):

https://chromiumcodereview.appspot.com/881733002/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.h:55: base::LaunchOptions*
mutable_launch_options() { return &launch_options_; }
I don't have a good memory of what the future callers will look like.

But is it not possible to have callers:

1. Build their LaunchOptions
2. pass it to NameSpaceSandbox::Launch() which would then patch-up options as
needed?

https://chromiumcodereview.appspot.com/881733002/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_sandbox.h:77: };
DISABLE_COPY_AND_ASSIGN

[rickyz (no longer on Chrome), 2015-01-31 02:33:54]

This change also fixes a bug where I forgot to check the return value of
WriteToIdMapFile. The NestedNamespaceSandbox test was invalid - since /proc/pid
becomes invalid from inside the PID namespace, you cannot nest the namespace
sandbox.

https://codereview.chromium.org/881733002/diff/80001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_sandbox.cc (right):

https://codereview.chromium.org/881733002/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.cc:113: write_fd_.reset();
On 2015/01/30 20:12:28, jln wrote:
> Having to "remember" to do that is problematic.
> 
> read_fd_ and write_fd_ are mostly used in this function.
> 
> I would not make them class members and instead just have the ScopedFD in this
> method.
> 
> Then you can have WriteUidGidMapAndUnblockProcess() take a ScopedFD (that you
> can Pass()) and I believe that everything will look right, with "automatic"
> closing of the ScopedFD.

Ah, that's much nicer, done

https://codereview.chromium.org/881733002/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.cc:127:
PCHECK(HANDLE_EINTR(write(write_fd_.get(), &kPipeValue, 1)) == 1);
On 2015/01/30 20:12:28, jln wrote:
> If the child process gets killed or disappears for any reason, you'll get
> SIGPIP-ed.
> 
> You could use send with MSG_NOSIGNAL. Or you could simply close the pipe to
> unblock the child, but then the child won't immediately know if the parent
died
> or signaled it (which is probably no big deal).

Done.

https://codereview.chromium.org/881733002/diff/80001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_sandbox.h (right):

https://codereview.chromium.org/881733002/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.h:55: base::LaunchOptions*
mutable_launch_options() { return &launch_options_; }
On 2015/01/30 20:12:28, jln wrote:
> I don't have a good memory of what the future callers will look like.
> 
> But is it not possible to have callers:
> 
> 1. Build their LaunchOptions
> 2. pass it to NameSpaceSandbox::Launch() which would then patch-up options as
> needed?

I wasn't crazy about the patching up part, because in order to truly own the
LaunchOptions, we would have to go every pointer field of LaunchOptions and
maintain our own copy of it. Right now, I think the only other ones on Linux are
maximize_rlimits and pre_exec_delegate (which we'd disallow), but we'd need to
add handling for any pointer fields added in the future. I think what we should
eventually do is switch all these pointer fields just embedding the objects
(with the exception of pre_exec_delegate). Then we could just make Launch take a
LaunchOptions, make a copy, and modify it.

https://codereview.chromium.org/881733002/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_sandbox.h:77: };
On 2015/01/30 20:12:28, jln wrote:
> DISABLE_COPY_AND_ASSIGN

Done.

[jln (very slow on Chromium), 2015-02-02 18:54:45]

lgtm as this is a good step forward.

But I think we need the nesting. Didn't it work before? I thought I had made
sure of that, but I might remember incorrectly.

IIRC, the Child could alter its own mapping via /proc/self (we'd need to make
WriteToIdMapFile async safe).

https://chromiumcodereview.appspot.com/881733002/diff/100001/sandbox/linux/se...
File sandbox/linux/services/namespace_sandbox.h (right):

https://chromiumcodereview.appspot.com/881733002/diff/100001/sandbox/linux/se...
sandbox/linux/services/namespace_sandbox.h:40: // Warning: After a process has
entered a new PID namespace, /proc/pid still
Wasn't it nestable before? I don't remember, but didn't the child modify its own
uid map via /proc/self?

This will be problematic since NaCl is in a nested PID NS.

https://chromiumcodereview.appspot.com/881733002/diff/100001/sandbox/linux/se...
File sandbox/linux/services/namespace_utils_unittest.cc (right):

https://chromiumcodereview.appspot.com/881733002/diff/100001/sandbox/linux/se...
sandbox/linux/services/namespace_utils_unittest.cc:47: RAW_CHECK(getuid() !=
uid);
These can be CHECKs (just need to _exit and not exit()).

Using RAW_CHECK is a little scary here: it looks like a bug given that
WriteToIdMapFile is not async signal safe.

[rickyz (no longer on Chrome), 2015-02-02 21:35:31]

This is now writing the uid/gid in the pre_exec_delgate instead. Mind giving
this another quick look to those extra changes?

https://codereview.chromium.org/881733002/diff/100001/sandbox/linux/services/...
File sandbox/linux/services/namespace_sandbox.h (right):

https://codereview.chromium.org/881733002/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_sandbox.h:40: // Warning: After a process has
entered a new PID namespace, /proc/pid still
On 2015/02/02 18:54:45, jln wrote:
> Wasn't it nestable before? I don't remember, but didn't the child modify its
own
> uid map via /proc/self?
> 
> This will be problematic since NaCl is in a nested PID NS.

Changed it to write the uid/gid map in the pre_exec delegate as we discussed, so
nested namespace sandboxes are now supported.

https://codereview.chromium.org/881733002/diff/100001/sandbox/linux/services/...
File sandbox/linux/services/namespace_utils_unittest.cc (right):

https://codereview.chromium.org/881733002/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:47: RAW_CHECK(getuid() !=
uid);
On 2015/02/02 18:54:45, jln wrote:
> These can be CHECKs (just need to _exit and not exit()).
> 
> Using RAW_CHECK is a little scary here: it looks like a bug given that
> WriteToIdMapFile is not async signal safe.

Looks like WriteToIdMapFile not being async-signal-safe was a bug - that's fixed
now.

[rickyz (no longer on Chrome), 2015-02-02 22:00:33]

Patchset #6 (id:120001) has been deleted

[rickyz (no longer on Chrome), 2015-02-02 22:02:58]

Apologies, that last patch set was missing a bunch of cleanups enabled by moving
the uid/gid map writing.

[jln (very slow on Chromium), 2015-02-03 01:14:30]

https://chromiumcodereview.appspot.com/881733002/diff/100001/sandbox/linux/se...
File sandbox/linux/services/namespace_utils_unittest.cc (right):

https://chromiumcodereview.appspot.com/881733002/diff/100001/sandbox/linux/se...
sandbox/linux/services/namespace_utils_unittest.cc:47: RAW_CHECK(getuid() !=
uid);
On 2015/02/02 21:35:31, rickyz wrote:
> On 2015/02/02 18:54:45, jln wrote:
> > These can be CHECKs (just need to _exit and not exit()).
> > 
> > Using RAW_CHECK is a little scary here: it looks like a bug given that
> > WriteToIdMapFile is not async signal safe.
> 
> Looks like WriteToIdMapFile not being async-signal-safe was a bug - that's
fixed
> now.

I don't think this code here needs to be async safe though.

https://chromiumcodereview.appspot.com/881733002/diff/140001/sandbox/linux/se...
File sandbox/linux/services/namespace_sandbox.cc (right):

https://chromiumcodereview.appspot.com/881733002/diff/140001/sandbox/linux/se...
sandbox/linux/services/namespace_sandbox.cc:30: explicit
WriteUidGidMapDelegate() : uid_(getuid()), gid_(getgid()) {}
You can remove "explicit"

https://chromiumcodereview.appspot.com/881733002/diff/140001/sandbox/linux/se...
File sandbox/linux/services/namespace_sandbox.h (right):

https://chromiumcodereview.appspot.com/881733002/diff/140001/sandbox/linux/se...
sandbox/linux/services/namespace_sandbox.h:51: base::LaunchOptions*
mutable_launch_options() { return &launch_options_; }
Get rid of that and take LaunchOptions as a Launch() argument?

https://chromiumcodereview.appspot.com/881733002/diff/140001/sandbox/linux/se...
File sandbox/linux/services/namespace_utils.cc (right):

https://chromiumcodereview.appspot.com/881733002/diff/140001/sandbox/linux/se...
sandbox/linux/services/namespace_utils.cc:34: base::ScopedFD
fd(HANDLE_EINTR(open(map_file, O_WRONLY)));
I would rather not assume ScopedFD to be async-safe unfortunately. Thankfully, I
don't think this will make this function much more complex to avoid it.

https://chromiumcodereview.appspot.com/881733002/diff/140001/sandbox/linux/se...
sandbox/linux/services/namespace_utils.cc:42: // This function needs to be
async-signal-safe, as it is called in between
Add this as a comment to the function instead.

[rickyz (no longer on Chrome), 2015-02-03 01:27:22]

https://codereview.chromium.org/881733002/diff/140001/sandbox/linux/services/...
File sandbox/linux/services/namespace_sandbox.cc (right):

https://codereview.chromium.org/881733002/diff/140001/sandbox/linux/services/...
sandbox/linux/services/namespace_sandbox.cc:30: explicit
WriteUidGidMapDelegate() : uid_(getuid()), gid_(getgid()) {}
On 2015/02/03 01:14:30, jln wrote:
> You can remove "explicit"

Done.

https://codereview.chromium.org/881733002/diff/140001/sandbox/linux/services/...
File sandbox/linux/services/namespace_sandbox.h (right):

https://codereview.chromium.org/881733002/diff/140001/sandbox/linux/services/...
sandbox/linux/services/namespace_sandbox.h:51: base::LaunchOptions*
mutable_launch_options() { return &launch_options_; }
On 2015/02/03 01:14:30, jln wrote:
> Get rid of that and take LaunchOptions as a Launch() argument?

Done.

https://codereview.chromium.org/881733002/diff/140001/sandbox/linux/services/...
File sandbox/linux/services/namespace_utils.cc (right):

https://codereview.chromium.org/881733002/diff/140001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.cc:34: base::ScopedFD
fd(HANDLE_EINTR(open(map_file, O_WRONLY)));
On 2015/02/03 01:14:30, jln wrote:
> I would rather not assume ScopedFD to be async-safe unfortunately. Thankfully,
I
> don't think this will make this function much more complex to avoid it.

Done.

https://codereview.chromium.org/881733002/diff/140001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.cc:42: // This function needs to be
async-signal-safe, as it is called in between
On 2015/02/03 01:14:30, jln wrote:
> Add this as a comment to the function instead.

Done.

[jln (very slow on Chromium), 2015-02-03 01:27:46]

lgtm

https://chromiumcodereview.appspot.com/881733002/diff/130011/sandbox/linux/se...
File sandbox/linux/services/namespace_sandbox.h (right):

https://chromiumcodereview.appspot.com/881733002/diff/130011/sandbox/linux/se...
sandbox/linux/services/namespace_sandbox.h:30: //   
Credentials::DropFileSystemAccess().
Add the call to DropAllCapabilities() as well.

https://chromiumcodereview.appspot.com/881733002/diff/130011/sandbox/linux/se...
sandbox/linux/services/namespace_sandbox.h:40: const base::LaunchOptions&
options);
Nit: indent.

Could also be worth renaming to LaunchProcess for clarity.

[rickyz (no longer on Chrome), 2015-02-03 01:35:39]

https://chromiumcodereview.appspot.com/881733002/diff/130011/sandbox/linux/se...
File sandbox/linux/services/namespace_sandbox.h (right):

https://chromiumcodereview.appspot.com/881733002/diff/130011/sandbox/linux/se...
sandbox/linux/services/namespace_sandbox.h:30: //   
Credentials::DropFileSystemAccess().
On 2015/02/03 01:27:46, jln wrote:
> Add the call to DropAllCapabilities() as well.

Done.

https://chromiumcodereview.appspot.com/881733002/diff/130011/sandbox/linux/se...
sandbox/linux/services/namespace_sandbox.h:40: const base::LaunchOptions&
options);
On 2015/02/03 01:27:46, jln wrote:
> Nit: indent.
> 
> Could also be worth renaming to LaunchProcess for clarity.

Done.

[rickyz (no longer on Chrome), 2015-02-03 01:35:45]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-02-03 01:37:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/881733002/210001

[commit-bot: I haz the power, 2015-02-03 03:38:13]

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

[commit-bot: I haz the power, 2015-02-03 03:38:14]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (None)

[rickyz (no longer on Chrome), 2015-02-03 07:25:14]

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-02-03 07:25:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/881733002/210001

[commit-bot: I haz the power, 2015-02-03 07:26:46]

Committed patchset #10 (id:210001)

[commit-bot: I haz the power, 2015-02-03 07:27:47]

Patchset 10 (id:??) landed as
https://crrev.com/8f235daf6d29e05f8a1949864da493e910a3ddd1
Cr-Commit-Position: refs/heads/master@{#314284}