Namespace sandbox: add important security checks

Namespace sandbox: add important security checks

When engaging the namespace sandbox, add important checks that the process
is single threaded and has no directory file descriptor open.

As part of this change, move the function engaging the namespace
sandbox from the Zygote to the LinuxSandbox class.

BUG=457377, 312380
Committed: https://crrev.com/b94f6817d3a0e20ec5c3393a4eb13dd360acbd4e
Cr-Commit-Position: refs/heads/master@{#315932}

[jln (very slow on Chromium), 2015-02-11 00:08:04]

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

[jln (very slow on Chromium), 2015-02-11 00:08:39]

Ricky: PTAL!

This adds the required security check. I'll write another CL to pass a fd to
these APIs rather than requiring the caller to make the security checks.

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

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

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

Matthew: I'm adding you since I'll need you to take a look in any case (until
rickyz gets committer status).

Feel free to have a look if you have time, or to wait for Ricky to review it
first if you would rather expedite this.

[rickyz (no longer on Chrome), 2015-02-11 22:59:56]

lgtm, thanks for doing this - just a couple of comments on comments

https://codereview.chromium.org/915823002/diff/100001/content/common/sandbox_...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://codereview.chromium.org/915823002/diff/100001/content/common/sandbox_...
content/common/sandbox_linux/sandbox_linux.cc:198: // safe, as this class is
purposedly keeping a file descriptor to /proc.
nit: purposely (or just remove if you prefer)

https://codereview.chromium.org/915823002/diff/100001/content/common/sandbox_...
File content/common/sandbox_linux/sandbox_linux.h (right):

https://codereview.chromium.org/915823002/diff/100001/content/common/sandbox_...
content/common/sandbox_linux/sandbox_linux.h:33: // can be implement either with
unprivileged namespaces or with the setuid
nit: implemented

https://codereview.chromium.org/915823002/diff/100001/content/common/sandbox_...
content/common/sandbox_linux/sandbox_linux.h:72: // This requires "sealing" the
sandbox later to be effective by calling
Maybe: In order for this sandbox to be effective, it must be "sealed" by calling
InitializeSandbox().

https://codereview.chromium.org/915823002/diff/100001/content/common/sandbox_...
content/common/sandbox_linux/sandbox_linux.h:83: // Currently seccomp-bpf and
address space limitations (the setuid sandbox
I'm a little confused by this comment and also can't quite parse the next
sentence - not clear what "the given pre-built configuration" refers to.

https://codereview.chromium.org/915823002/diff/100001/content/zygote/zygote_m...
File content/zygote/zygote_main_linux.cc (right):

https://codereview.chromium.org/915823002/diff/100001/content/zygote/zygote_m...
content/zygote/zygote_main_linux.cc:43: #include
"sandbox/linux/services/credentials.h"
Can we get rid of this include in this case?

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

https://codereview.chromium.org/915823002/diff/100001/sandbox/linux/services/...
sandbox/linux/services/credentials.h:30: // for ensuring it is single threaded
when calling this API.
It might be good to explicitly mention the reason - that capabilities are
per-thread

[jln (very slow on Chromium), 2015-02-11 23:12:51]

New patchsets have been uploaded after l-g-t-m from rickyz@chromium.org

[jln (very slow on Chromium), 2015-02-11 23:13:07]

Thanks Ricky!

Matthew: PTAL!

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/common/s...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/common/s...
content/common/sandbox_linux/sandbox_linux.cc:198: // safe, as this class is
purposedly keeping a file descriptor to /proc.
On 2015/02/11 22:59:56, rickyz wrote:
> nit: purposely (or just remove if you prefer)

Done.

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/common/s...
File content/common/sandbox_linux/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/common/s...
content/common/sandbox_linux/sandbox_linux.h:33: // can be implement either with
unprivileged namespaces or with the setuid
On 2015/02/11 22:59:56, rickyz wrote:
> nit: implemented

Done.

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/common/s...
content/common/sandbox_linux/sandbox_linux.h:72: // This requires "sealing" the
sandbox later to be effective by calling
On 2015/02/11 22:59:56, rickyz wrote:
> Maybe: In order for this sandbox to be effective, it must be "sealed" by
calling
> InitializeSandbox().

Done.

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/common/s...
content/common/sandbox_linux/sandbox_linux.h:83: // Currently seccomp-bpf and
address space limitations (the setuid sandbox
On 2015/02/11 22:59:56, rickyz wrote:
> I'm a little confused by this comment and also can't quite parse the next
> sentence - not clear what "the given pre-built configuration" refers to.

Ooch, this comment was a mess. Hopefully it's better now.

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/zygote/z...
File content/zygote/zygote_main_linux.cc (right):

https://chromiumcodereview.appspot.com/915823002/diff/100001/content/zygote/z...
content/zygote/zygote_main_linux.cc:43: #include
"sandbox/linux/services/credentials.h"
On 2015/02/11 22:59:56, rickyz wrote:
> Can we get rid of this include in this case?

Done.

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

https://chromiumcodereview.appspot.com/915823002/diff/100001/sandbox/linux/se...
sandbox/linux/services/credentials.h:30: // for ensuring it is single threaded
when calling this API.
On 2015/02/11 22:59:56, rickyz wrote:
> It might be good to explicitly mention the reason - that capabilities are
> per-thread

Done.

[mdempsky, 2015-02-12 03:07:15]

lgtm

https://chromiumcodereview.appspot.com/915823002/diff/120001/components/nacl/...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/915823002/diff/120001/components/nacl/...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:119:
CHECK(!HasOpenDirectory());
This matches the ordering under IsSuidSandboxChild(), but I wonder if it would
be better to do the HasOpenDirectory() check *after* we've dropped filesystem
access?  (Also in LinuxSandbox, if appropriate.)

[jln (very slow on Chromium), 2015-02-12 03:25:28]

https://chromiumcodereview.appspot.com/915823002/diff/120001/components/nacl/...
File components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/915823002/diff/120001/components/nacl/...
components/nacl/loader/sandbox_linux/nacl_sandbox_linux.cc:119:
CHECK(!HasOpenDirectory());
On 2015/02/12 03:07:14, mdempsky wrote:
> This matches the ordering under IsSuidSandboxChild(), but I wonder if it would
> be better to do the HasOpenDirectory() check *after* we've dropped filesystem
> access?  (Also in LinuxSandbox, if appropriate.)

We actually also do that as part of the "sealing". "Sealing" checks for open
directories and then closes the /proc file descriptor.

This is mainly to fail "earlier" (maybe we could DCHECK?), but the real security
check is indeed after InitializeLayerOneSandbox() has run!

[jln (very slow on Chromium), 2015-02-12 03:26:15]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2015-02-12 03:26:52]

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

[commit-bot: I haz the power, 2015-02-12 03:31:23]

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

[commit-bot: I haz the power, 2015-02-12 03:31:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jln (very slow on Chromium), 2015-02-12 03:45:47]

New patchsets have been uploaded after l-g-t-m from mdempsky@chromium.org

[jln (very slow on Chromium), 2015-02-12 03:56:02]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2015-02-12 03:56:57]

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

[commit-bot: I haz the power, 2015-02-12 04:53:14]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-02-12 04:53:54]

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