Move a couple of utility functions to a new namespace_utils class.

Move a couple of utility functions to a new namespace_utils class.

The implementations are minorly different - uses /proc/self/ns/* files
to detect namespaces support, and always make sure to write uid_map in a
single write.

BUG=312380
Committed: https://crrev.com/41fb1457a6a221b5417608154bbdc7d433520f0d
Cr-Commit-Position: refs/heads/master@{#313221}

[rickyz (no longer on Chrome), 2015-01-14 00:12:00]

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

[rickyz (no longer on Chrome), 2015-01-14 00:39:56]

Patchset #2 (id:20001) has been deleted

[jln (very slow on Chromium), 2015-01-14 00:46:38]

A few early comments.

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
File sandbox/linux/services/namespace_utils.h (right):

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
sandbox/linux/services/namespace_utils.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
Not (c) after "Copyright" anymore

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
sandbox/linux/services/namespace_utils.h:13: 
Please, add a little bit of documentation about what this class is for. (cf
style guide)

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
sandbox/linux/services/namespace_utils.h:30: };
DISALLOW_IMPLICIT_CONSTRUCTORS

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_utils.cc (right):

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:29: }  // namespace
Nit: vertical space

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:47: bool
NamespaceUtils::SupportsLinuxNamespace(int type) {
This implementation will have some false negatives. For instance, CLONE_NEWPID
was supported before /proc/self/ns/pid existed.

Could you track down which kernel added /proc/self/ns/pid to see if it's a
problem?

I think it was roughly added with unprivileged CLONE_NEWUSER. Let's document
that cleanly in the interface.

[rickyz (no longer on Chrome), 2015-01-23 00:23:38]

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_utils.cc (right):

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:47: bool
NamespaceUtils::SupportsLinuxNamespace(int type) {
On 2015/01/14 00:46:38, jln wrote:
> This implementation will have some false negatives. For instance, CLONE_NEWPID
> was supported before /proc/self/ns/pid existed.
> 
> Could you track down which kernel added /proc/self/ns/pid to see if it's a
> problem?
> 
> I think it was roughly added with unprivileged CLONE_NEWUSER. Let's document
> that cleanly in the interface.
Darn, looks like this was added in 3.8. In view of this limitation, I think I'll
leave things as is in credentials.h and drop this CL for now and try to do this
a little more cleanly after the initial namespace sandbox stuff is in.

[jln (very slow on Chromium), 2015-01-23 00:27:09]

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_utils.cc (right):

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:47: bool
NamespaceUtils::SupportsLinuxNamespace(int type) {
On 2015/01/23 00:23:38, rickyz wrote:
> On 2015/01/14 00:46:38, jln wrote:
> > This implementation will have some false negatives. For instance,
CLONE_NEWPID
> > was supported before /proc/self/ns/pid existed.
> > 
> > Could you track down which kernel added /proc/self/ns/pid to see if it's a
> > problem?
> > 
> > I think it was roughly added with unprivileged CLONE_NEWUSER. Let's document
> > that cleanly in the interface.
> Darn, looks like this was added in 3.8. In view of this limitation, I think
I'll
> leave things as is in credentials.h and drop this CL for now and try to do
this
> a little more cleanly after the initial namespace sandbox stuff is in.

As you prefer, but it's also ok to keep as long as your rename the API
adequately.

Since we consider NEWUSER to be usefully supported only around 3.10 we can
consider this API to tell us what flags are compatible with NEWUSER. The current
name however seems to suggest kernel support for things like privileged
CLONE_NEWPID, which it doesn't do.

[rickyz (no longer on Chrome), 2015-01-23 01:19:23]

Patchset #3 (id:60001) has been deleted

[rickyz (no longer on Chrome), 2015-01-23 01:20:19]

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
File sandbox/linux/services/namespace_utils.h (right):

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
sandbox/linux/services/namespace_utils.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2015/01/14 00:46:37, jln wrote:
> Not (c) after "Copyright" anymore

Done.

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
sandbox/linux/services/namespace_utils.h:13: 
On 2015/01/14 00:46:37, jln wrote:
> Please, add a little bit of documentation about what this class is for. (cf
> style guide)

Not the best comment, since the class is a little sparse - hope that's OK

https://codereview.chromium.org/849893004/diff/1/sandbox/linux/services/names...
sandbox/linux/services/namespace_utils.h:30: };
On 2015/01/14 00:46:37, jln wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
File sandbox/linux/services/namespace_utils.cc (right):

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:29: }  // namespace
On 2015/01/14 00:46:38, jln wrote:
> Nit: vertical space

Sorry, where should vertical space be added/removed here?

https://codereview.chromium.org/849893004/diff/40001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:47: bool
NamespaceUtils::SupportsLinuxNamespace(int type) {
On 2015/01/23 00:27:09, jln wrote:
> On 2015/01/23 00:23:38, rickyz wrote:
> > On 2015/01/14 00:46:38, jln wrote:
> > > This implementation will have some false negatives. For instance,
> CLONE_NEWPID
> > > was supported before /proc/self/ns/pid existed.
> > > 
> > > Could you track down which kernel added /proc/self/ns/pid to see if it's a
> > > problem?
> > > 
> > > I think it was roughly added with unprivileged CLONE_NEWUSER. Let's
document
> > > that cleanly in the interface.
> > Darn, looks like this was added in 3.8. In view of this limitation, I think
> I'll
> > leave things as is in credentials.h and drop this CL for now and try to do
> this
> > a little more cleanly after the initial namespace sandbox stuff is in.
> 
> As you prefer, but it's also ok to keep as long as your rename the API
> adequately.
> 
> Since we consider NEWUSER to be usefully supported only around 3.10 we can
> consider this API to tell us what flags are compatible with NEWUSER. The
current
> name however seems to suggest kernel support for things like privileged
> CLONE_NEWPID, which it doesn't do.

Yeah - on second thought, I prefer your suggestion, and having this makes the
namespace sandbox code easier - thanks!

[jln (very slow on Chromium), 2015-01-23 02:48:24]

My high level remark is that I think you need to check for the current process
being able to CLONE_NEWUSER one way or another.

You could keep both APIs. If that's too confusing you could check for kernel
support, but also return a bitmask of supported flags (gathered from looking at
/proc/self/ns/).

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

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
sandbox/linux/services/credentials.cc:50: struct FILECloser {
Remove

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
File sandbox/linux/services/credentials.h (left):

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
sandbox/linux/services/credentials.h:44: static bool SupportsNewUserNS();
I wonder if we should keep this as "CanCreateProcessInNewUserNS"?

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

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
sandbox/linux/services/credentials_unittest.cc:152: CHECK_EQ(-1,
unshare(CLONE_NEWUSER));
If we decide to not keep SupportsNewUserNS() under a new name, let's still make
this a separate function!

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

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_utils.cc:33: bool
NamespaceUtils::WriteToIdMapFile(const char* map_file, generic_id_t id) {
Nit: add "// static" before the type.

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_utils.cc:48: bool
NamespaceUtils::SupportsUnprivilegedNamespace(int type) {
Note: this works for kernel support, but it still doesn't tell you if switching
to a user NS will work.

The most common case where this won't work is if Chrome is run from within a
chroot().

This could be problematic, so I wonder if we should keep the previous function
(with the probe process) under a name such as CanCreateProcessInNewUserNS().

And maybe rename this one to KernelSupports*

WDYT?

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

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_utils_unittest.cc:39:
EXPECT_EQ(supports_user_ns, fork_succeeded);
This will break from inside a chroot. fork_succeeds implies supports_user_ns
though.

https://chromiumcodereview.appspot.com/849893004/diff/80001/sandbox/linux/ser...
sandbox/linux/services/namespace_utils_unittest.cc:48: ASSERT_NE(-1, pid);
Same remark, this will fail inside a chroot().

[rickyz (no longer on Chrome), 2015-01-23 23:59:37]

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/c...
File sandbox/linux/services/credentials.cc (right):

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/c...
sandbox/linux/services/credentials.cc:50: struct FILECloser {
On 2015/01/23 02:48:23, jln wrote:
> Remove

Done.

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/c...
File sandbox/linux/services/credentials.h (left):

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/c...
sandbox/linux/services/credentials.h:44: static bool SupportsNewUserNS();
On 2015/01/23 02:48:23, jln wrote:
> I wonder if we should keep this as "CanCreateProcessInNewUserNS"?

Done - at this point, there pretty messy separation of responsibility between
namespace_utils.cc and credentials.cc - I'll try to clean this up after getting
the non-setuid sandbox in.

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/c...
File sandbox/linux/services/credentials_unittest.cc (right):

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/c...
sandbox/linux/services/credentials_unittest.cc:152: CHECK_EQ(-1,
unshare(CLONE_NEWUSER));
On 2015/01/23 02:48:23, jln wrote:
> If we decide to not keep SupportsNewUserNS() under a new name, let's still
make
> this a separate function!
Ended up keeping the function

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

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:33: bool
NamespaceUtils::WriteToIdMapFile(const char* map_file, generic_id_t id) {
On 2015/01/23 02:48:23, jln wrote:
> Nit: add "// static" before the type.

Done.

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils.cc:48: bool
NamespaceUtils::SupportsUnprivilegedNamespace(int type) {
On 2015/01/23 02:48:23, jln wrote:
> Note: this works for kernel support, but it still doesn't tell you if
switching
> to a user NS will work.
> 
> The most common case where this won't work is if Chrome is run from within a
> chroot().
> 
> This could be problematic, so I wonder if we should keep the previous function
> (with the probe process) under a name such as CanCreateProcessInNewUserNS().
> 
> And maybe rename this one to KernelSupports*
> 
> WDYT?

Yeah, that makes sense. CanCreateProcessInNewUserNS will be useful for deciding
whether to fall back to the setuid sandbox, and
KernelSupportsUnprivilegedNamespace will be useful for determining CLONE_*
flags.

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

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils_unittest.cc:39:
EXPECT_EQ(supports_user_ns, fork_succeeded);
On 2015/01/23 02:48:23, jln wrote:
> This will break from inside a chroot. fork_succeeds implies supports_user_ns
> though.

Done.

https://codereview.chromium.org/849893004/diff/80001/sandbox/linux/services/n...
sandbox/linux/services/namespace_utils_unittest.cc:48: ASSERT_NE(-1, pid);
On 2015/01/23 02:48:23, jln wrote:
> Same remark, this will fail inside a chroot().

Done.

[jln (very slow on Chromium), 2015-01-27 00:06:45]

Almost there!

The testing framework is a little crazy. sandbox/linux/ has its own stuff (so
that each test runs in its own process). And then if you fork() from in there
things become even more complicated.

Sorry about that. Hopefully it's something we can clarify a bit.

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

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.cc:59: if
(!base::PathExists(base::FilePath("/proc/self/ns/user"))) {
Note: PathExists a little problematic because the documentation doesn't say what
it will do with symlinks.

I don't think it's a big deal as long as we have good unit tests.

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

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.h:30: // not work from within a chroot.
Nite: maybe s/chroot/sandbox to be more generic?

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

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
New file, so 2015 and no '(c)'

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:30:
EXPECT_TRUE(supports_user_ns);
SANDBOX_ASSERT in this SANDBOX_TEST.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:42: uid_t uid = getuid();
const

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:43: gid_t gid = getgid();
const

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:45: EXPECT_NE(uid, getuid());
You're in a child, it needs to _exit() and not exit().

You need to use CHECK()or RAW_CHECK here.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:57: EXPECT_EQ(pid,
HANDLE_EINTR(waitpid(pid, &status, 0)));
This is a SANDBOX_TEST so use SANDBOX_ASSERT_EQ() (best) or CHECK* macros.

[rickyz (no longer on Chrome), 2015-01-27 01:08:04]

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

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/01/27 00:06:44, jln wrote:
> 2015

Done.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.cc:59: if
(!base::PathExists(base::FilePath("/proc/self/ns/user"))) {
On 2015/01/27 00:06:44, jln wrote:
> Note: PathExists a little problematic because the documentation doesn't say
what
> it will do with symlinks.
> 
> I don't think it's a big deal as long as we have good unit tests.

Acknowledged.

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

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.h:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/01/27 00:06:44, jln wrote:
> 2015

Done.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils.h:30: // not work from within a chroot.
On 2015/01/27 00:06:44, jln wrote:
> Nite: maybe s/chroot/sandbox to be more generic?

Done.

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

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2015/01/27 00:06:44, jln wrote:
> New file, so 2015 and no '(c)'

Done.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:30:
EXPECT_TRUE(supports_user_ns);
On 2015/01/27 00:06:44, jln wrote:
> SANDBOX_ASSERT in this SANDBOX_TEST.

Done.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:42: uid_t uid = getuid();
On 2015/01/27 00:06:44, jln wrote:
> const

Done.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:43: gid_t gid = getgid();
On 2015/01/27 00:06:44, jln wrote:
> const

Done.

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:45: EXPECT_NE(uid, getuid());
On 2015/01/27 00:06:44, jln wrote:
> You're in a child, it needs to _exit() and not exit().
> 
> You need to use CHECK()or RAW_CHECK here.

Gah, I keep forgetting this, sorry

https://codereview.chromium.org/849893004/diff/100001/sandbox/linux/services/...
sandbox/linux/services/namespace_utils_unittest.cc:57: EXPECT_EQ(pid,
HANDLE_EINTR(waitpid(pid, &status, 0)));
On 2015/01/27 00:06:44, jln wrote:
> This is a SANDBOX_TEST so use SANDBOX_ASSERT_EQ() (best) or CHECK* macros.

Nice catch with the asserts - using the right asserts revealed obvious bugs that
were horribly breaking this test.

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

lgtm

(Note: try to make your rebases a separate patchset to facilitate reviews).

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

The CQ bit was checked by rickyz@chromium.org

[commit-bot: I haz the power, 2015-01-27 02:18:46]

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

[commit-bot: I haz the power, 2015-01-27 03:58:27]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-01-27 03:59:12]

Patchset 5 (id:??) landed as
https://crrev.com/41fb1457a6a221b5417608154bbdc7d433520f0d
Cr-Commit-Position: refs/heads/master@{#313221}