Linux sandbox: change API to start the sandbox

Linux sandbox: change API to start the sandbox

This CL makes SupportsSandbox() more straightforward by returning the
level of support in the kernel.
One major advantage is that being single threaded is now checked right before engaging
the sandbox.

Now, StartSandbox() is required to be able to check the number of threads
in the current process. For this the LinuxSandbox class in content/ and
in nacl/ are modified to always pass a file descriptor to /proc/self/tasks/
to SandboxBPF::StartSandbox().

In content::LinuxSandbox, such a descriptor was only available in DEBUG builds
for security reasons. We make sure to always close it, as long as InitializeSandbox()
is called. However, a few fringe processes, such as the init process or the ASAN
coverage helper need to close it manually.

BUG=434820
Committed: https://crrev.com/bd4df41c57cbfa415675602e0befc8ca128a009a
Cr-Commit-Position: refs/heads/master@{#305569}

[jln (very slow on Chromium), 2014-11-20 00:38:04]

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

[jln (very slow on Chromium), 2014-11-20 00:38:24]

jln@chromium.org changed reviewers:
- jorgelo@chromium.org

[jln (very slow on Chromium), 2014-11-20 00:40:27]

mdempsky: PTAL!

There are some dragons in the Zygote code, but thankfully a problem there
wouldn't cause a major security risk (the closing of file descriptors there is
concerned with 1. the init process, 2. the ASAN coverage helper).

[jln (very slow on Chromium), 2014-11-24 20:15:01]

Matthew: no urgency, but making sure that this didn't fall off of your radar.
Let me know if you'd like someone else to do a first pass.

[mdempsky, 2014-11-24 21:58:23]

LGTM

From the commit message:
> However, a few fring processes, such as the init process or the ASAN

Nit: s/fring/fringe/

https://chromiumcodereview.appspot.com/733303004/diff/120001/content/common/s...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/733303004/diff/120001/content/common/s...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:168:
make_scoped_ptr(broker_sandboxer_allocator()), base::ScopedFD(-1));
I think you can just use base::ScopedFD() instead of base::ScopedFD(-1)?

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

https://chromiumcodereview.appspot.com/733303004/diff/120001/content/common/s...
content/common/sandbox_linux/sandbox_linux.cc:155: // If
LinuxSandbox::PreinitializeSandbox() runs, InitializeSandbox() must run
Can we CHECK this in ~LinuxSandbox()?

[jln (very slow on Chromium), 2014-11-24 22:45:48]

Thanks, good suggestion! (even though unfortunately a lot of the difficult cases
are fork + _exit or fork + exec)

https://chromiumcodereview.appspot.com/733303004/diff/120001/content/common/s...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://chromiumcodereview.appspot.com/733303004/diff/120001/content/common/s...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:168:
make_scoped_ptr(broker_sandboxer_allocator()), base::ScopedFD(-1));
On 2014/11/24 21:58:23, mdempsky wrote:
> I think you can just use base::ScopedFD() instead of base::ScopedFD(-1)?

Done.

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

https://chromiumcodereview.appspot.com/733303004/diff/120001/content/common/s...
content/common/sandbox_linux/sandbox_linux.cc:155: // If
LinuxSandbox::PreinitializeSandbox() runs, InitializeSandbox() must run
On 2014/11/24 21:58:23, mdempsky wrote:
> Can we CHECK this in ~LinuxSandbox()?

Done.

[jln (very slow on Chromium), 2014-11-24 22:48:26]

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

[jln (very slow on Chromium), 2014-11-24 22:49:33]

Nasko: do you mind looking at the small change in
content/public/common/sandbox_init.h?

Jorgelo: Mostly FYI (but feel free to sanity check if you have time) that we're
going to keep a fd to /proc open even in release mode. It's a bit scary, but I
think it's the right move to have these sanity checks always work.

[nasko, 2014-11-24 23:01:02]

LGTM with a couple of nits.

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/common/s...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/common/s...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:160:
sandbox.set_proc_task_fd(proc_task_fd.release());
nit: Conceptually, it seems better to see fd.Pass() here, as it is passing the
ownership over to the sandbox object. Not sure if it makes sense, so leaving it
up to you.

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/public/c...
File content/public/common/sandbox_init.h (right):

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/public/c...
content/public/common/sandbox_init.h:91: // /proc,
nit: no need for a line break after the comma, join with next line.

[jln (very slow on Chromium), 2014-11-24 23:03:30]

Mark: FYI for NaCl, behavior there should be untouched.

[jln (very slow on Chromium), 2014-11-24 23:26:03]

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/common/s...
File content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/common/s...
content/common/sandbox_linux/sandbox_seccomp_bpf_linux.cc:160:
sandbox.set_proc_task_fd(proc_task_fd.release());
On 2014/11/24 23:01:02, nasko wrote:
> nit: Conceptually, it seems better to see fd.Pass() here, as it is passing the
> ownership over to the sandbox object. Not sure if it makes sense, so leaving
it
> up to you.

The problem is that it takes a plain fd, not a ScopedFD. I'm planning to
refactor for this to take a ScopedFD, but it's one CL or two away.

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/public/c...
File content/public/common/sandbox_init.h (right):

https://chromiumcodereview.appspot.com/733303004/diff/140001/content/public/c...
content/public/common/sandbox_init.h:91: // /proc,
On 2014/11/24 23:01:02, nasko wrote:
> nit: no need for a line break after the comma, join with next line.

Done. I love clang-format, but it's not good for comments.

[Jorge Lucangeli Obes, 2014-11-24 23:48:32]

jorgelo@chromium.org changed reviewers:
+ jorgelo@chromium.org

[Jorge Lucangeli Obes, 2014-11-24 23:48:34]

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

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/common/s...
content/common/sandbox_linux/sandbox_linux.h:55: // GetFileDescriptorsToClose()
This formatting is super weird.

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/common/s...
content/common/sandbox_linux/sandbox_linux.h:63: // but
Ditto. Are you trying to keep function names in their own lines? Also, you used
parenthesis on Preinitialize... and not on Initialize....

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/public/c...
File content/public/common/sandbox_init.h (right):

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/public/c...
content/public/common/sandbox_init.h:91: // /proc, |proc_task_fd| must be a
valid file descriptor to /proc/self/tasks.
the directory name is |/proc/self/task| right?

https://chromiumcodereview.appspot.com/733303004/diff/160001/sandbox/linux/se...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/733303004/diff/160001/sandbox/linux/se...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:71: // The sandbox needs to be able to
access files in "/proc/self/tasks/". If
Ditto task.

[jln (very slow on Chromium), 2014-11-25 01:30:48]

Thanks Jorge!

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

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/common/s...
content/common/sandbox_linux/sandbox_linux.h:55: // GetFileDescriptorsToClose()
On 2014/11/24 23:48:33, Jorge Lucangeli Obes wrote:
> This formatting is super weird.

Yeah, sorry clang-format seems to always start a new line for words that have
the first letter as a capital.

Done.

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/common/s...
content/common/sandbox_linux/sandbox_linux.h:63: // but
On 2014/11/24 23:48:33, Jorge Lucangeli Obes wrote:
> Ditto. Are you trying to keep function names in their own lines? Also, you
used
> parenthesis on Preinitialize... and not on Initialize....

Done.

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/public/c...
File content/public/common/sandbox_init.h (right):

https://chromiumcodereview.appspot.com/733303004/diff/160001/content/public/c...
content/public/common/sandbox_init.h:91: // /proc, |proc_task_fd| must be a
valid file descriptor to /proc/self/tasks.
On 2014/11/24 23:48:33, Jorge Lucangeli Obes wrote:
> the directory name is |/proc/self/task| right?

Done.

https://chromiumcodereview.appspot.com/733303004/diff/160001/sandbox/linux/se...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/733303004/diff/160001/sandbox/linux/se...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:71: // The sandbox needs to be able to
access files in "/proc/self/tasks/". If
On 2014/11/24 23:48:33, Jorge Lucangeli Obes wrote:
> Ditto task.

Done.

[jln (very slow on Chromium), 2014-11-25 01:31:11]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-11-25 01:32:27]

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

[commit-bot: I haz the power, 2014-11-25 02:32:40]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-11-25 02:33:21]

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