sandbox: Fix RedirectToUserSpacePolicyWrapper

sandbox: Fix RedirectToUserSpacePolicyWrapper

This fixes RedirectToUserSpacePolicyWrapper to
handle ERRNO ErrorCodes that are behind Conditional
ErrorCodes.

BUG=408953
TEST=sandbox_linux_unittests: Added new test

Committed: https://crrev.com/2652904cdf19f2e432ec52fff7657c483d358bb0
Cr-Commit-Position: refs/heads/master@{#293116}

[leecam, 2014-08-29 12:32:06]

leecam@chromium.org changed reviewers:
+ jln@chromium.org

[jln (very slow on Chromium), 2014-08-29 20:40:06]

Thanks for taking this on, Lee!

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

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:202: if ((err.err() &
SECCOMP_RET_ACTION) == SECCOMP_RET_ERRNO) {
Shouldn't use your new ChangeErrnoToTraps() method here as well,
unconditionally?

This could simplify this codepath.

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:221: void ChangeErrnoToTraps(ErrorCode*
err, SandboxBPF* sandbox_compiler) const {
Because of the recursion you need to handle all cases here (trap, allow,
conditions, errno). Please document on top of this function what it does, and
use it unconditionally from EvaluateSyscall() above.

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:232: // Have a Trap or Allow, leave as
they are.
To make this code more robust, check if it is a Trap or Allow (and do nothing)
and if not, CHECK(false) (or NOTREACHED());

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

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2264: intptr_t
NopHandler(const struct arch_seccomp_data& args, void*) {
Maybe NoOp rather than Nop? And/Or add a comment.

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2277: if (sysno ==
__NR_rt_sigprocmask || sysno == __NR_rt_sigreturn
Should we export a "IsRequiredForUnsafeTrapsSyscall()" or something along these
lines?

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2323: // Nothing to do, just
ensuring policy compiles and verifies.
It would be worth checking that the policy does what it's supposed to do, no? It
looks like it would be easy to actually call setuid/setgid and check (but it
means you might want to default allow all other syscalls rather than restrict
them).

[leecam, 2014-08-29 22:56:31]

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

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:202: if ((err.err() &
SECCOMP_RET_ACTION) == SECCOMP_RET_ERRNO) {
On 2014/08/29 20:40:05, jln wrote:
> Shouldn't use your new ChangeErrnoToTraps() method here as well,
> unconditionally?
> 
> This could simplify this codepath.

Done.

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:221: void ChangeErrnoToTraps(ErrorCode*
err, SandboxBPF* sandbox_compiler) const {
On 2014/08/29 20:40:05, jln wrote:
> Because of the recursion you need to handle all cases here (trap, allow,
> conditions, errno). Please document on top of this function what it does, and
> use it unconditionally from EvaluateSyscall() above.

Done.

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf.cc:232: // Have a Trap or Allow, leave as
they are.
On 2014/08/29 20:40:05, jln wrote:
> To make this code more robust, check if it is a Trap or Allow (and do nothing)
> and if not, CHECK(false) (or NOTREACHED());

Done.

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

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2264: intptr_t
NopHandler(const struct arch_seccomp_data& args, void*) {
On 2014/08/29 20:40:05, jln wrote:
> Maybe NoOp rather than Nop? And/Or add a comment.

Done.

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2277: if (sysno ==
__NR_rt_sigprocmask || sysno == __NR_rt_sigreturn
On 2014/08/29 20:40:05, jln wrote:
> Should we export a "IsRequiredForUnsafeTrapsSyscall()" or something along
these
> lines?

Done.

https://codereview.chromium.org/524603002/diff/60001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2323: // Nothing to do, just
ensuring policy compiles and verifies.
On 2014/08/29 20:40:05, jln wrote:
> It would be worth checking that the policy does what it's supposed to do, no?
It
> looks like it would be easy to actually call setuid/setgid and check (but it
> means you might want to default allow all other syscalls rather than restrict
> them).

Done.

[leecam, 2014-08-29 22:58:54]

PTAL changes done. 
Little uneasy adding the IsRequiredForUnsafeTrap call in this CL but see what
you think.

[jln (very slow on Chromium), 2014-09-02 18:41:54]

lgtm, but I would prefer if:

- You didn't rely on FD 100 being unused.
- Didn't block close from the testing policy.

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

https://codereview.chromium.org/524603002/diff/80001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2297: BPF_ASSERT_EQ(-1,
syscall(__NR_close, 100));
In general it's not a good idea to add strong assumptions of how file
descriptors are going to look like. We're inheriting this from where the test
suite runs, and it runs on a variety of platforms, sometimes as root, etc..

I'm also a little uneasy disallowing close() given how fundamental a system call
it is, in case of test failure, it may be called in some important way.

How about using something less fundamental like uname(2), or madvise(2)?

[leecam, 2014-09-02 19:00:13]

Thanks! I'll change as per my reply to your comment

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

https://codereview.chromium.org/524603002/diff/80001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:2297: BPF_ASSERT_EQ(-1,
syscall(__NR_close, 100));
On 2014/09/02 18:41:54, jln wrote:
> In general it's not a good idea to add strong assumptions of how file
> descriptors are going to look like. We're inheriting this from where the test
> suite runs, and it runs on a variety of platforms, sometimes as root, etc..
> 
> I'm also a little uneasy disallowing close() given how fundamental a system
call
> it is, in case of test failure, it may be called in some important way.
> 
> How about using something less fundamental like uname(2), or madvise(2)?

I agree, I was trying to think of a syscall that takes a simple parameter (not a
userspace pointer) that can be checked by the policy, exists on all
architectures and will return the same result when ran as root or not. 

uname and madvise take pointers and I wasnt keen on that. I thought setuid(0)
but that would succeed with root and fail when not. So I picked close with a
high fd but agree its not ideal. I'll go through the list and try pick something
better. Maybe just uname(-1) which will fail consistently if the policy failed
to block it.

[leecam, 2014-09-03 09:16:11]

The CQ bit was checked by leecam@chromium.org

[commit-bot: I haz the power, 2014-09-03 09:16:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leecam@chromium.org/524603002/100001

[commit-bot: I haz the power, 2014-09-03 10:04:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-09-03 11:00:03]

Committed patchset #6 (id:100001) as 9a87a07661b32eaa8073038365c3a63657249bb4

[commit-bot: I haz the power, 2014-09-10 03:25:02]

Patchset 6 (id:??) landed as
https://crrev.com/2652904cdf19f2e432ec52fff7657c483d358bb0
Cr-Commit-Position: refs/heads/master@{#293116}