Added support for greylisting of system calls.

Added support for greylisting of system calls.


BUG=130662
TEST=sandbox_linux_unittests


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

[Markus (顧孟勤), 2012-11-13 02:23:16]

I cleaned up the code that I was working on before getting sick and ripped out
the more complicated pieces that you didn't seem to like (i.e. code that would
magically do the right thing for sigprocmask() and sigreturn()).

Instead, I now require that these two system calls must always be allowed
unconditionally. It makes policies a little more complicated, but we can
hopefully live with that.

Apart from this little annoyance with restrictions on the policy, I am generally
happy with the API. It is powerful enough that we can do almost anything we
would ever need to do from a greylisted function.

Implementation details turned out to be still a little more complex than I had
hoped for. Comments are welcome.

[jln (very slow on Chromium), 2012-11-14 00:49:23]

Looks like the meat is missing here. Did you forget syscall.cc ? (And its unit
test? ;)

[jln (very slow on Chromium), 2012-11-15 01:51:52]

Still didn't have time to review most of this, but I'm sending a first round of
comments after quickly going through it. Hopefully I'll do it tomorrow or
Friday.

1. In general, don't forget that functions declarations should have comments
describing what the function does and how to use it.

In some cases, there are even no parameter names at the declaration site, and to
add to that very generic types (such as void *) are sometimes used.

2. syscall.cc really needs its own set of unittests. Add a syscall_unittests.cc.
You can move TEST(SandboxBpf, Syscall) there.

Please add much more tests for this. Maybe it's worth it to try some Linux
kernel API for each parameter number ? At least mmap2 (6 parameters) should be
tested.

Also did you manage to try this on a real ARM and IA32 device? I fear it could
break somewhere.

3. For architectures where it'll turn out that syscall.cc is not available, (or
if it doesn't work and we want to quickly remove support for an arthicture), I
would love if we could have the Grey Listing feature not work instead of
breaking compilation.

Maybe it's just a matter of adding a C Syscall() function at the end of
syscall.cc and calling NOTREACHED() << "This architecture does not support
syscall grey listing" or something like this?

Even better would be to bubble this up so that we break when a grey listing
ErrorCode is created. Perhaps it's just a matter of exposing a "bool
SupportsCustomSyscallImplementation()" function from syscall.cc

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/codegen.cc (right):

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/codegen.cc:148: void
CodeGen::TraverseRecursively(std::set<Instruction *> *visited,
Does this really need to be a class method ?

It looks like it would work without being declared in codegen.h and you can put
it directly in this file in the anonymous namespace (don't forget to add a
comment explaining it though).

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/codegen.cc:159: default:
Please put default at the end of the switch.

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/codegen.h (right):

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/codegen.h:84: void Traverse(Instruction *, void
(*fnc)(Instruction *, void *), void *);
Please, name and document the parameter, this is impossible to understand as-is.

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/codegen.h:94: // Helper function for Traverse()
Nit: "." at the end.

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/codegen.h:95: void
TraverseRecursively(std::set<Instruction *> *, Instruction *);
Same here

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/sandbox_bpf.h (right):

https://chromiumcodereview.appspot.com/11363212/diff/2001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:343: static void     
CheckForUnsafeErrorCodes(Instruction *, void *);
Please, add argument names and comments!

[Markus (顧孟勤), 2012-11-15 06:13:53]

PTAL

[jln (very slow on Chromium), 2012-11-20 01:08:31]

Looks good in general for a debugging feature. A few comments below.

I also really would like a test that Chrome works if we add an Unsafe trap
handler. I should find a way to do this, and I'll, but please do a one-off test
at the very least.

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

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:326: insn->k <=
SECCOMP_RET_TRAP+trapArraySize_) {
Better to do insn->k - SECCOMP_RET_TRAP <= trapArraySize_ to avoid int overflow
completely.

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:497: __BYTE_ORDER == __BIG_ENDIAN ? 4 :
0),
As agreed offline, please add an #error on __BIG_ENDIAN since this is completely
untested.

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:554:
std::make_pair(RedirectToUserspaceEvalWrapper, &evaluators_));
nit: two more spaces for indent.

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:558: has_unsafe_traps_ ?
redirected_evaluators : evaluators_, &err)) {
Nit: ident arguments together.

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:710: sigismember(&ctx->uc_sigmask,
SIGBUS)) {
This is pretty hack-ish. It's ok-ish since it's a debugging feature.

Maybe it could be made a bit more clear by wrapping this into 
SetIsInSigSysHandler() / GetIsInSigSysHandler() functions?

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:814: return MakeTrap(fnc, aux, true);
Add a comment near true "/* Safe trap */"

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:818: return MakeTrap(fnc, aux, false);
Same here.

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

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:357: 
Could you add a small explaination, saying that SIGBUS is used as a hack in the
implementation ?

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:373: 
Same thing, please add a small note about what this is testing.

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:376: sigemptyset(&mask0);
Please add comments, to make this faster to read.

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

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall.h:19: intptr_t Syscall(int nr, ...);
Should we rename this to DirectSyscall() maybe ? I fear confusion between
Syscall() and syscall().

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
File sandbox/linux/seccomp-bpf/syscall_unittest.cc (right):

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
General comment: I think in almost all cases when you write EXPECT_, you mean
ASSERT_

You could either just replace all expect with assert or figure out which should
be which, as your prefer.

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall_unittest.cc:21: #ifdef __NR_mmap2
Let's avoid #define when possible. What about having a const int kMmap6NR
instead?

https://chromiumcodereview.appspot.com/11363212/diff/6001/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/syscall_unittest.cc:27: TEST(Syscall, Syscall) {
Please use a less generic name for the test: "TrivialOneArgSyscall" or something
like this?

[Markus (顧孟勤), 2012-11-20 02:44:53]

I believe I addressed all your concerns. PTAL

[jln (very slow on Chromium), 2012-11-21 01:21:56]

On 2012/11/20 02:44:53, Markus (顧孟勤) wrote:
> I believe I addressed all your concerns. PTAL

LGTM, with one minor request for additional comment.

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

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

https://chromiumcodereview.appspot.com/11363212/diff/17004/sandbox/linux/secc...
sandbox/linux/seccomp-bpf/sandbox_bpf.h:291: // function, as it avoid the extra
round-trip to the signal handler. And
I think it's worth explaining a tad more.

"It is more efficient than calling glibc's syscall() since it will use the
white-listed system call stub and won't cause an extra trap."

Also mention at the end something like: "Thus, the return value of
ForwardSyscall is suitable as the return value for a trap handler."

[commit-bot: I haz the power, 2012-11-21 01:28:03]

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

[commit-bot: I haz the power, 2012-11-21 04:12:14]

Change committed as 168969