SECCOMP-BPF: Refactor the BPF sandbox API to use fewer "static" fields and methods.

SECCOMP-BPF: Refactor the BPF sandbox API to use objects rather than "static" methods.

This change allows us to stack multiple instances of the sandbox.

Also, split up headers in a generally saner fashion.

BUG=130662
TEST=sandbox_linux_unittests


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184541

[Markus (顧孟勤), 2013-02-13 22:03:27]

Hi Julien, I think this CL is now ready for reviews. I refactored a good amount
of code, but the end result is that we can now stack sandboxes (with a couple of
caveats mentioned in the API comments).

[jln (very slow on Chromium), 2013-02-15 20:58:24]

This is very exciting!

Very quick, high level comment:

Please try to keep a real (non nested) anonymous namespace as much as possible.
Some "using" directives may help". 

Otherwise, nested anonymous is "ok".

https://chromiumcodereview.appspot.com/12223109/diff/23001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/12223109/diff/23001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:35: namespace playground2 {
Most of this can remain a real anonymous namespace, can't it ?

Please try to save it as much as you can instead of embedding it when you don't
need to.

https://chromiumcodereview.appspot.com/12223109/diff/23001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:979: }  // namespace
// namespace playground2

https://chromiumcodereview.appspot.com/12223109/diff/23001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/verifier.cc (right):

https://chromiumcodereview.appspot.com/12223109/diff/23001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/verifier.cc:14: namespace {
I really would rather not nest the anonymous namespace inside of playground2.
But I understand why you do it in that case.

Please continute to make sure that the anonymous namespace is the first thing in
the other namespace if you do that, as it is now.

https://chromiumcodereview.appspot.com/12223109/diff/23001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/verifier.cc:444: }  // namespace
namespace playground 2

[jln (very slow on Chromium), 2013-02-16 06:53:04]

I've been dragged to other things until now and I'm exhausted. I'll take a look
after the week-end, sorry.

[jln (very slow on Chromium), 2013-02-20 01:35:49]

This is definitely going in the right direction!

Looks pretty good in general. The Sandbox object issue is really hairy and I
need to think about it more.

I think with the few nits below and more comment this would be ok as an
incremental improvement.

https://chromiumcodereview.appspot.com/12223109/diff/35001/content/common/san...
File content/common/sandbox_seccomp_bpf_linux.cc (right):

https://chromiumcodereview.appspot.com/12223109/diff/35001/content/common/san...
content/common/sandbox_seccomp_bpf_linux.cc:1543:
sandbox->SetSandboxPolicy(syscall_policy, broker_process);
It looks like this is where the Sandbox object should be  created instead ?
(Please add a comment about the life time of this object as mentioned below).

https://chromiumcodereview.appspot.com/12223109/diff/35001/content/common/san...
content/common/sandbox_seccomp_bpf_linux.cc:1558: Sandbox sandbox;
This looks very suspicious to have a sandbox object that is this short lived.

The main comment belongs in sandbox_bpf.h, but another comment here would be
helpful.

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:67: Sandbox();
It's not clear what the lifetime of this object is.

In particular, we should make it clear that it's ok to delete the sandbox object
after starting the sandbox and the implementation takes care of maintaining
state!

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:286:
BPF_ASSERT(syscall(__NR_getpid, 0) > 0);
Do you want to replace this by "uname" or something else ?

It's better to not use a "useful" system call in there. It could confuse tools
such as ASAN or Valgrind or really make something painful to debug.

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/util.cc (right):

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/util.cc:121: proc_fd = open("/proc/",
O_RDONLY|O_DIRECTORY);
It really scares me to do that. We absolutely don't want to keep an open fd to
/proc.

It looks like this whole function could be deleted anyways and is not used by
demo.cc  ?

If you really want to do that, please use a ScopedFD to make sure it gets closed
before we return.

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
File sandbox/linux/seccomp-bpf/util.h (right):

https://chromiumcodereview.appspot.com/12223109/diff/35001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/util.h:7: 
Could you #ifdef all of it to SANDBOX_STANDALONE ?

It's be reassuring.

Even better: couldn't you move this to the anonymous namespace in demo.cc and
get rid of util.h / util.cc ?

[commit-bot: I haz the power, 2013-02-20 08:27:04]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Markus (顧孟勤), 2013-02-20 08:38:57]

PTAL

I guess, I didn't have an official LGTM, yet. Made all the changes that you
asked for (mostly, they were pretty obvious comments or similarly trivial
changes)

[Markus (顧孟勤), 2013-02-20 09:59:42]

Ah, I shouldn't have listened to you :-)

There was a reason why the unittest used getpid() when testing for stacking of
sandboxes. We wanted to verify that when stacking, we can tighten policies, but
we can't relax them. There even was a comment saying so.

So, we need a system call that is actually ERR_ALLOWED in the first policy and
then denied in the second stacked policy. We can't just use any arbitrary system
call and then always return a magic "errno" value.

We obviously /could/ use almost any type of system call, if only we hand it
suitable parameters each time we call it, but it is easiest to reason about this
when using a read-only system call that doesn't take any arguments.

getpid() is the obvious choice, and there really is no good reason why it should
break. But just in case, I now changed it to getppid(). That should always be
OK.

And in the unlikely event that this breaks after all, it results in a unittest
failure. That's a good thing. It'll make us investigate and figure out what new
craziness has found its way into our runtime libraries.

[jln (very slow on Chromium), 2013-02-22 00:39:48]

lgtm, but please try to check with me before landing.

I would like to make merging as painless as possible during these difficult
weeks and this touches our policies. I think landing early next week would cause
no problem.

[commit-bot: I haz the power, 2013-02-25 20:15:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markus@chromium.org/12223109/53007

[commit-bot: I haz the power, 2013-02-26 01:39:41]

Change committed as 184541