Explicitly test bit 30 in the system call number to distinguish between the new x32 API and older I…

Explicitly test bit 30 in the system call number to distinguish between the new x32 API and older Intel APIs.

Also, extend the system call range from 0..512 to 0..1024. This covers the extra system calls added with x32.

As x32 isn't widely available yet, we don't add any other code to support it (e.g. we don't build a version of
demo.cc that runs in x32). But by explicitly blocking it for i386 and x86-64 we ensure that a "default allow"
policy is going to do the right thing.

TEST=make && demo32 && demo64
BUG=130662
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=141155

[Markus (顧孟勤), 2012-06-06 15:08:44]

I don't fully understand how x32 is going to work, and even if I did, it is
possible that the details are still going to change. So, this change list might
not technically be needed, but in any case it is "security in depth".

My worry is that users of the sandboxing code set a "default allow" policy in
order to just black list a few system calls (e.g. ptrace()). But they then
forget that the same system call is available as both an x86-64 version and an
x32 version.

By explicitly checking bit 30 of the system call number we make sure that we
never accidentally allow mirrored system calls, even if we managed to make it
past the initial check for the system call architecture.

[Markus (顧孟勤), 2012-06-06 22:59:34]

I agree about your concern. I have been thinking how to do this most cleanly
with this API; it's a little tricky, because I don't want the sandbox to
implement policy, unless I can't help it. Policy should be set by the user of
the sandbox.

So, I think what I'll do is to have a default range of allowed system calls. And
I'll then provide a new function to override this range, if it doesn't agree
with what the user wants (e.g. there is now a new system call, that needs to be
handled).

But I also think this doesn't really fit well with the current change list.
It'll be much more natural to implement this change, when I get to implement
generation of optimized BPF filters (probably in the next few days).

Can I punt until then? Can you review the current changelist as is, and we'll
keep the range check as a future TODO?

[Chris Evans, 2012-06-07 01:02:38]

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

https://chromiumcodereview.appspot.com/10542028/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:222: BPF_STMT(BPF_RET+BPF_K,
SECCOMP_RET_ERRNO + SECCOMP_DENY_ERRNO));
This should probably be SECCOMP_RET_KILL too, see below.

https://chromiumcodereview.appspot.com/10542028/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:231: #if defined(__i386__) ||
defined(__x86_64__)
I don't quite get this bit. If we're compiling for __i386__, why do anything? I
probably don't understand x86-32, but doesn't it only apply to 64-bit kernels?

https://chromiumcodereview.appspot.com/10542028/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:240: BPF_STMT(BPF_RET+BPF_K,
SECCOMP_RET_ERRNO + SECCOMP_DENY_ERRNO));
If we get here, seems like something extraordinarily bad has happened.
SECCOMP_RET_KILL IMHO

[Markus (顧孟勤), 2012-06-07 01:33:28]

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

https://chromiumcodereview.appspot.com/10542028/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:222: BPF_STMT(BPF_RET+BPF_K,
SECCOMP_RET_ERRNO + SECCOMP_DENY_ERRNO));
On 2012/06/07 01:02:38, Chris Evans wrote:
> This should probably be SECCOMP_RET_KILL too, see below.

I don't particularly like SECCOMP_RET_KILL, as it makes debugging pretty much
impossible. And I really want to avoid a situation where developers start
disabling the seccomp sandbox, because they think it prevents them from doing
their job.

And somebody calling us with the wrong API isn't necessarily dangerous. It
probably just means somebody linked against some third-party code that uses the
wrong API. This needs to be fixed, of course. But it's not a fatal security
problem that requires immediate termination.

Having said that, the best long-term solution is probably to generate a suitable
SECCOMP_RET_TRAP. But out trap handler is way too simplistic at this point. It
can't distinguish between different reasons for why it was called.

This is on my TODO list to be added. And then this whole discussion is hopefully
moot.

So, in the meantime, I'd be OK with returning SECCOMP_RET_KILL if that's what
you want. Or we could be nice to the users of this code (none at this moment),
and return SECCOMP_RET_ERRNO. At least, that's debuggable.

https://chromiumcodereview.appspot.com/10542028/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:231: #if defined(__i386__) ||
defined(__x86_64__)
The preprocessor tests check for the expected API that our program is using.
They have nothing to do with what the kernel actually supports.

And any 64bit kernel allows programs to use both i386 and x86-64 system calls;
no matter whether the program happens to be 32bit or 64bit.

So, I really don't want to rely on the kernel disabling the x32 API when the
program happens to be compiled as a i386 32bit binary. That probably isn't
happening in current kernels.

In an ideal world, the test for the SECCOMP_ARCH already takes care of all of
this. But I can't find any guarantee that this is always going to be the case
(in fact, there I am not even sure there is an AUDIT_ARCH_X32). And there have
been bugs in this part of the kernel before.

https://chromiumcodereview.appspot.com/10542028/diff/1/sandbox/linux/seccomp-...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:240: BPF_STMT(BPF_RET+BPF_K,
SECCOMP_RET_ERRNO + SECCOMP_DENY_ERRNO));
On 2012/06/07 01:02:38, Chris Evans wrote:
> If we get here, seems like something extraordinarily bad has happened.
> SECCOMP_RET_KILL IMHO

Let me know, what you prefer until we get a better SIGSYS handler in place.
There are downsides to killing the program, but since we don't really have users
at this very moment, it probably doesn't matter much.

If I don't hear from you, I'll just upload a new CL with SECCOMP_RET_KILL and a
comment that this should eventually become SECCOMP_RET_TRAP.

[Chris Evans, 2012-06-07 23:01:36]

LGTM with one nit.

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

https://chromiumcodereview.appspot.com/10542028/diff/1003/sandbox/linux/secco...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:236: #if defined(__i386__) ||
defined(__x86_64__)
I'd be tempted to remove this outer conditional. It makes the logic easier to
read, and I don't see any harm on forcing the bit to be clear on e.g. ARM. In
fact we'll probably want this one day for the future ARM64 vs. a32 mode :P