Linux sandbox: change seccomp detection and initialization.

Linux sandbox: change seccomp detection and initialization.

Change how we detect seccomp kernel support and its initialization.

Before, detecting seccomp kernel supports would involve starting probe processes
that would enable seccomp. A crash would mean that seccomp was not supported.

This was necessary with old kernel version and old glibc versions that were
problematic.
Now that these shouldn't exist in the field, we move the checks to unit
tests instead.

Following the refactor in https://chromiumcodereview.appspot.com/733303004/
we can greatly simplify both detection and starting of the sandbox to make the API
more sane.

BUG=434820
TBR=piman

Committed: https://crrev.com/bd576720e621951616af892bcf03ffaac49f1881
Cr-Commit-Position: refs/heads/master@{#305706}

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

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

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

mdempsky: PTAL!

keescook: could you please sanity check  KernelSupportsSeccompBPF() and
KernelSupportsSeccompTsync() in sandbox/linux/seccomp-bpf/sandbox_bpf.cc ?

rsesek: mostly FYI

This is the second step to clean-up this crazy API.

https://chromiumcodereview.appspot.com/759473002/diff/1/sandbox/linux/seccomp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/759473002/diff/1/sandbox/linux/seccomp...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:24: enum SeccompLevel {
I considered making this an enum class, but since these are the bits of a
bitmask, the lack of implicit conversion to int was problematic.


Let me know what you think.

[mdempsky, 2014-11-25 03:14:06]

lgtm

https://codereview.chromium.org/759473002/diff/1/sandbox/linux/seccomp-bpf/sa...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://codereview.chromium.org/759473002/diff/1/sandbox/linux/seccomp-bpf/sa...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:24: enum SeccompLevel {
On 2014/11/25 01:24:29, jln wrote:
> I considered making this an enum class, but since these are the bits of a
> bitmask, the lack of implicit conversion to int was problematic.
> 
> 
> Let me know what you think.

One solution might be to change "int SupportsSeccompSandbox();" into "bool
SupportsSeccompSandbox(SeccompLevel seccomp_level);", and then the callers just
need to check the level(s) they're interested in.

Minor bonus: You avoid making system calls to check for support the caller
doesn't care about.

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

https://codereview.chromium.org/759473002/diff/1/sandbox/linux/seccomp-bpf/sa...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://codereview.chromium.org/759473002/diff/1/sandbox/linux/seccomp-bpf/sa...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:24: enum SeccompLevel {
On 2014/11/25 03:14:06, mdempsky wrote:
> On 2014/11/25 01:24:29, jln wrote:
> > I considered making this an enum class, but since these are the bits of a
> > bitmask, the lack of implicit conversion to int was problematic.
> > 
> > 
> > Let me know what you think.
> 
> One solution might be to change "int SupportsSeccompSandbox();" into "bool
> SupportsSeccompSandbox(SeccompLevel seccomp_level);", and then the callers
just
> need to check the level(s) they're interested in.
> 
> Minor bonus: You avoid making system calls to check for support the caller
> doesn't care about.

I considered that, but in practice in content:: the Status should only be
checked once and I would rather only have to make one call to that function
rather than as many calls as there are sandbox types.

But maybe it's worth it just to use an enum class and because a bitmask is a
little error prone as well.

[mdempsky_google, 2014-11-25 03:44:54]

On 2014/11/25 03:36:01, jln wrote:
> I considered that, but in practice in content:: the Status should only be
> checked once and I would rather only have to make one call to that function
> rather than as many calls as there are sandbox types.
> 
> But maybe it's worth it just to use an enum class and because a bitmask is a
> little error prone as well.

Fair enough, and I'm fine either way.  It seemed like in practice callers
usually only cared about one type.  E.g., most calls look like:

    if (SupportsSeccompSandbox() & level) { ... }

which could just become

    if (SupportsSeccompSandbox(level)) { ... }

and probably be a bit more readable, IMO.

It seemed like it was only two SandboxBPF unit tests that cared about checking
both at the same time.

[jln (very slow on Chromium), 2014-11-25 05:47:58]

On 2014/11/25 03:44:54, mdempsky_google wrote:
> On 2014/11/25 03:36:01, jln wrote:
> > I considered that, but in practice in content:: the Status should only be
> > checked once and I would rather only have to make one call to that function
> > rather than as many calls as there are sandbox types.
> > 
> > But maybe it's worth it just to use an enum class and because a bitmask is a
> > little error prone as well.
> 
> Fair enough, and I'm fine either way.  It seemed like in practice callers
> usually only cared about one type.  E.g., most calls look like:
> 
>     if (SupportsSeccompSandbox() & level) { ... }
> 
> which could just become
> 
>     if (SupportsSeccompSandbox(level)) { ... }
> 
> and probably be a bit more readable, IMO.
> 
> It seemed like it was only two SandboxBPF unit tests that cared about checking
> both at the same time.

Yeah, I was thinking that I didn't make my point across well: right now, nothing
uses TSYNC, we don't even report it in the status.

An upcoming CL will report the full status and it felt weird to have to iterate
over all the possible levels.

But I think that you're right: it's well worth the trade-off. It's not an area
that will change significantly and currently "iterating over all the levels"
means 2 iterations, in one place (that runs only once, from the Zygote).

And I think I really dislike the bitmask. It's too easy to make a typo / error.
Thanks!

[hidehiko, 2014-11-25 11:47:36]

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

[hidehiko, 2014-11-25 11:47:36]

Thank you for working on this, and CC'ing to me.

https://codereview.chromium.org/759473002/diff/40001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/759473002/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:63: int rv = prctl(PR_SET_SECCOMP,
SECCOMP_MODE_FILTER, nullptr);
As for nullptr, although PNaCl toolchain uses clang++, but c++11 is not enabled
by default.
We need either:
1) use 0 (i.e. do not use c++11 feature for now).
2) enable c++11 by default in native_client/build/untrusted.gypi.
3) enable c++11 only for sandbox building via compile_flags.
Mark, any thoughts?

[hidehiko, 2014-11-25 11:51:53]

One more stuff.
linux_chromium_rel_ng failure looks a real failure. Could you look into it?
Also, could you run linux_rel_precise32 bot by adding a following line to your
description (just below the TEST= line).

CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32

[jln (very slow on Chromium), 2014-11-25 18:16:08]

Thanks for the comments. We should probably discuss Newlib in another thread. Is
there a design doc somewhere?

https://codereview.chromium.org/759473002/diff/40001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/759473002/diff/40001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:63: int rv = prctl(PR_SET_SECCOMP,
SECCOMP_MODE_FILTER, nullptr);
On 2014/11/25 11:47:36, hidehiko wrote:
> As for nullptr, although PNaCl toolchain uses clang++, but c++11 is not
enabled
> by default.
> We need either:
> 1) use 0 (i.e. do not use c++11 feature for now).
> 2) enable c++11 by default in native_client/build/untrusted.gypi.
> 3) enable c++11 only for sandbox building via compile_flags.
> Mark, any thoughts?

(1) is not an option since we use C++11 both in base/ and in sandbox/. I'm
surprised you didn't run into that for base/ stuff.

Is there a design doc somewhere about switching to Newlib? I'm surprised this
hasn't been discussed before.

sandbox/ is in a tough place, because it needs to serve Chromium, but also soon
Chrome OS daemons and Mojo. I would be worried if Newlib slowed us down.
Hopefully we can get the PNaCl toolchain capabilities on par with what's
supported in Chromium.

[Robert Sesek, 2014-11-25 18:44:50]

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

[Robert Sesek, 2014-11-25 18:44:51]

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:65: if (-1 == rv && EFAULT == errno) {
Here the test is |-1 == rv|, whereas on line 77 it's inverted. FWIW I prefer the
test on line 77.

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:44: static int SupportsSeccompSandbox();
Supports kind of implies boolean. Maybe GetSupportedSeccompLevel() ?

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:74: // The calling process must a
|seccomp_level| to tell the sandbox which type
You accidentally the whole verb.

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

Matthew: I switched to an enum class and your API suggestion. Would you mind
taking another quick look?

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:65: if (-1 == rv && EFAULT == errno) {
On 2014/11/25 18:44:50, Robert Sesek wrote:
> Here the test is |-1 == rv|, whereas on line 77 it's inverted. FWIW I prefer
the
> test on line 77.

Done.

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:44: static int SupportsSeccompSandbox();
On 2014/11/25 18:44:50, Robert Sesek wrote:
> Supports kind of implies boolean. Maybe GetSupportedSeccompLevel() ?

I switched to Matthew's suggestion of GetSupportedSeccompLevel(SeccompLevel);

https://codereview.chromium.org/759473002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:74: // The calling process must a
|seccomp_level| to tell the sandbox which type
On 2014/11/25 18:44:50, Robert Sesek wrote:
> You accidentally the whole verb.

Done.

[Kees Cook, 2014-11-25 20:09:44]

https://codereview.chromium.org/759473002/diff/80001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/759473002/diff/80001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:139: if
(IsSingleThreaded(proc_task_fd_)) {
Isn't it valid to start a single thread but request multi-thread filter support?

[Robert Sesek, 2014-11-25 20:11:36]

https://codereview.chromium.org/759473002/diff/80001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/759473002/diff/80001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:139: if
(IsSingleThreaded(proc_task_fd_)) {
On 2014/11/25 20:09:44, Kees Cook wrote:
> Isn't it valid to start a single thread but request multi-thread filter
support?

Yes, but we once thought it was wise to ensure that the intent to start the
sandbox in a specific mode matched reality. It'd be fine to lift this, and we
debated adding it. Over time it won't make sense to conflate threadedness with
using the new seccomp syscall.

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

https://codereview.chromium.org/759473002/diff/80001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://codereview.chromium.org/759473002/diff/80001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:139: if
(IsSingleThreaded(proc_task_fd_)) {
On 2014/11/25 20:09:44, Kees Cook wrote:
> Isn't it valid to start a single thread but request multi-thread filter
support?

Yeah, this is a little contentious.
The reasoning is that if you can make SeccompLevel::SINGLE_THREADED work, you
*really* should use it.

If you're passing SeccompLevel::MULTI_THREADED, you'll require more recent
kernel support and you'll run the very real risk of races (where in some runs,
threads run forbidden system calls before the sandbox is engaged, but in other
runs they run them after the sandbox is engaged).

So, this is essentially crashing to tell you "You probably good make
::SINGLE_THREADED work". However we should probably only do that in Debug mode..

Let's address that in another CL though, since I'm not changing the behavior
here.

[Kees Cook, 2014-11-25 20:23:04]

lgtm

[mdempsky, 2014-11-25 20:39:07]

lgtm

https://codereview.chromium.org/759473002/diff/100001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://codereview.chromium.org/759473002/diff/100001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:25: INVALID = 0,
Currently the only use of this value is to check that it's not used, so it seems
like you could prevent that easily by getting rid of it. :)  But if you expect
to need it in the future, I'm fine with keeping it.

[jln (very slow on Chromium), 2014-11-25 20:58:13]

https://codereview.chromium.org/759473002/diff/100001/sandbox/linux/seccomp-b...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://codereview.chromium.org/759473002/diff/100001/sandbox/linux/seccomp-b...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:25: INVALID = 0,
On 2014/11/25 20:39:06, mdempsky wrote:
> Currently the only use of this value is to check that it's not used, so it
seems
> like you could prevent that easily by getting rid of it. :)  But if you expect
> to need it in the future, I'm fine with keeping it.

Yeah, true that it doesn't make a ton of sense anymore with an enum class.

Done.

[jln (very slow on Chromium), 2014-11-25 20:58:21]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-11-25 20:58:50]

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

[jln (very slow on Chromium), 2014-11-25 20:59:13]

The CQ bit was unchecked by jln@chromium.org

[jln (very slow on Chromium), 2014-11-25 20:59:43]

Piman: TBR for the trivial change in
content/renderer/renderer_main_platform_delegate_android.cc

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

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-11-25 21:00:51]

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

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

The CQ bit was unchecked by jln@chromium.org

[jln (very slow on Chromium), 2014-11-25 22:12:59]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-11-25 22:13:33]

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

[commit-bot: I haz the power, 2014-11-25 22:14:39]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-25 22:15:59]

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