SECCOMP-BPF: Added support for greylisting of system calls.

SECCOMP-BPF: Added support for greylisting of system calls.

In addition to a Sandbox::Trap() handler, we now have a Sandbox::UnsafeTrap()
handler. This feature should only be used for debugging purposes as it subverts
the security of the sandbox. But it is useful to track down problems with the
sandboxing policy. Within an unsafe trap handler, all sandbox restrictions are
lifted. This, for example, allows us to allow system calls that would normally
be denied by the policy, but to log their arguments, return value, and call stack.

N.B.: this is the second attempt at submitting this CL. See https://chromiumcodereview.appspot.com/11363212/
      for previous code reviews


BUG=130662
TEST=sandbox_linux_unittests
NOTRY=true


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169213

[jln (very slow on Chromium), 2012-11-21 23:04:46]

A few minor comments and a request for a better ForwardSyscall test.

If I understand correctly, since ForwardSyscall() failed completely, we couldn't
even write etc and everything just failed silently ? Then we need a more simple
policy to test ForwardSyscall() safely.

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:855:
static_cast<intptr_t>(args.args[0]),
Shouldn't you cast to the same type that is used in SandboxSyscall ? i.e. ptr_t

In any case, the interface to SandboxSyscall() should be better documented to
include the expected type.

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:267: 
I think the previous issue showed a missing test for ForwardSyscall().

I would add a simple policy with everything ERR_ALLOWED, but a few syscalls in
an UnsafeTrap() (ideally, some that won't be used implicitly by anything, use
some fringe API like keyctl or something like that, or maybe you can use setuid
/ setresuid, I think they work if you pass the current id).

Then use ForwardSyscall() in there.

Could you please make sure the test fails without your fix?

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:312: BPF_TEST(SandboxBpf,
GreyListedPolicy,
It's slobbering that this test passed.

Could you please add a few system calls with an argument (and make sure it fails
without your fix) ?

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall.cc (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall.cc:182: void *args[6];
Whatever type you choose here, I think we should have a CHECK for __WORDSIZE ==
sizeof(the_type_you_chose).

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall.h (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall.h:19: intptr_t SandboxSyscall(int nr, ...);
The Argument type should be specified here.

[Markus (顧孟勤), 2012-11-22 00:29:23]

PTAL

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:855:
static_cast<intptr_t>(args.args[0]),
On 2012/11/21 23:04:46, Julien Tinnes wrote:
> Shouldn't you cast to the same type that is used in SandboxSyscall ? i.e.
ptr_t

intptr_t should be the same width as ptr_t or as void* for that matter.

I could make it cast to one of the latter, but it will get very unreadable. As
far as I can tell, C++ would want me to cast to an intptr_t first and then to a
ptr_t. I don't believe I am allowed to use a C++ cast to both change the width
of the type and the fact that we are changing from an integer to a pointer type.

But maybe, I am missing something. C++ casts always confuse me. C-style casts
are so much saner.

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall.h (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall.h:19: intptr_t SandboxSyscall(int nr, ...);
On 2012/11/21 23:04:46, Julien Tinnes wrote:
> The Argument type should be specified here.

Can you elaborate what you want here. This is the same API that glibc's
syscall() exposes. And it has pretty much the same caveats.

It's somewhat dangerous to use and requires an understanding of how the kernel
and the ABI passes arguments. It is the caller's responsibility to be make sure
that it passes arguments that the ABI will handle correctly.

For x86-32 this means any 32bit integer type is probably OK. But trying to pass
a 64bit type is pretty much guaranteed to cause a bug.

For x86-64 this means that it has to be a type that will be passed in one of the
integer CPU registers. Both 32bit and 64bit is typically OK. The only caveat is
the sixth argument which will be passed on the stack. If this is a 64bit type,
it has to explicitly passed as such. I believe, the only time when this matters
is when passing a NULL. As C++ defines it as "0", the compiler would erroneously
forget to zero out the MSB. So, NULL should always be cast to (void *)NULL. For
the first five arguments this probably doesn't matter as x86-64 adds missing
zero bits when setting CPU registers. syscall() has the same problem.

For ARM, it has to be a type that will be passed in one of the regular 32bit CPU
registers. In practice, this means any 32bit type is fine. Don't know what ARM
64bit will look like yet.

I understand that this API is problematic. But I am not sure there is a good
alternative unless we want to force callers to always pass in six "void *"
arguments. This would require explicit casting. Often, it would be difficult to
do with C++ casts (see my other comments). And in fact, it is almost as easy to
introduce subtle bugs. I had to fix a bunch of those in existing code.

Also, it would differ from the API that users are used to see (i.e. glibc's
implementation of syscall()).

The upside of all of this is that this is an internal API that we don't plan on
exposing. ForwardSyscall() is a much easier to use API, that has most of the
same functionality.

[jln (very slow on Chromium), 2012-11-22 01:12:39]

LGTM with one nit: simplify PrctlPolicy

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.cc (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:855:
static_cast<intptr_t>(args.args[0]),
On 2012/11/22 00:29:23, Markus (顧孟勤) wrote:
> On 2012/11/21 23:04:46, Julien Tinnes wrote:
> > Shouldn't you cast to the same type that is used in SandboxSyscall ? i.e.
> ptr_t
> 
> intptr_t should be the same width as ptr_t or as void* for that matter.

It practice yeah, in theory int could be larger than a void*.

> I could make it cast to one of the latter, but it will get very unreadable. As
> far as I can tell, C++ would want me to cast to an intptr_t first and then to
a
> ptr_t. I don't believe I am allowed to use a C++ cast to both change the width
> of the type and the fact that we are changing from an integer to a pointer
type.
> 
> But maybe, I am missing something. C++ casts always confuse me. C-style casts
> are so much saner.

Yeah you'd need a reinterpret_cast here.

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall.h (right):

https://chromiumcodereview.appspot.com/11419121/diff/5001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall.h:19: intptr_t SandboxSyscall(int nr, ...);
On 2012/11/22 00:29:23, Markus (顧孟勤) wrote:

> I understand that this API is problematic. But I am not sure there is a good
> alternative unless we want to force callers to always pass in six "void *"
> arguments. This would require explicit casting. Often, it would be difficult
to
> do with C++ casts (see my other comments). And in fact, it is almost as easy
to
> introduce subtle bugs. I had to fix a bunch of those in existing code.
> 
> Also, it would differ from the API that users are used to see (i.e. glibc's
> implementation of syscall()).

Yeah, I took a somewhat long look at it today, and I thought that in our high
level, C++ implementation of SandboxSyscall() we should try to aim for better
type checks that glibc's syscall().

glibc's syscall() is implemented per-architecture (for instance see
sysdeps/unix/sysv/linux/x86_64/syscall.S), while ours is portable, between
already three architectures. However, you got me somewhat convinced that it
would look ugly no matter what.

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

https://chromiumcodereview.appspot.com/11419121/diff/11001/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:344: // use of UnsafeTrap()
Let's get rid of this big chunk since the default is ERR_ALLOWED?

[commit-bot: I haz the power, 2012-11-22 02:05:27]

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

[jln (very slow on Chromium), 2012-11-22 03:50:24]

I'm adding NOTRY=true, since this is failing on win_rel for unrelated reasons.
Re-sending to the CQ..

[commit-bot: I haz the power, 2012-11-22 03:50:42]

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

[commit-bot: I haz the power, 2012-11-22 03:51:08]

Change committed as 169213