Linux Sandbox: Add support for SECCOMP_RET_TRACE.

Linux Sandbox: Add support for SECCOMP_RET_TRACE.

To use, make a BPF program return ErrorCode(ERR_TRACE + ret_data), where
ret_data is a 16 bit value that will be available to the tracing process
via PTRACE_GETEVENTMSG.

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

[rickyz (Google), 2014-05-09 04:47:59]

Hey, here's an initial attempt at adding SECCOMP_RET_TRACE.

[rickyz (Google), 2014-05-16 19:27:39]

On 2014/05/09 04:47:59, Ricky Zhou wrote:
> Hey, here's an initial attempt at adding SECCOMP_RET_TRACE.

ping, does this look reasonable?

[jln (very slow on Chromium), 2014-05-16 19:31:22]

On 2014/05/16 19:27:39, Ricky Zhou wrote:
> On 2014/05/09 04:47:59, Ricky Zhou wrote:
> > Hey, here's an initial attempt at adding SECCOMP_RET_TRACE.
> 
> ping, does this look reasonable?

I should be able to review today. I skimmed through it last Monday and it did
look reasonable at first sight :)

[jln (very slow on Chromium), 2014-05-20 03:02:09]

Sorry for the awfully long wait. This caught me right around "branch point",
which was a crazy burst of activity.

I promise that we are all in general much more responsive than this.

This generally looks good. I'm a bit horrified to see sandbox_bpf.h grow though,
I'll think of ways to tame that.

We're doing a lot of cleanup and new tests should use BPF_ASSERT_* macros
(technically this test should use SANDBOX_ASSERT_* macros, but I didn't add them
yet).

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
File sandbox/linux/seccomp-bpf/errorcode.cc (right):

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/errorcode.cc:21: if (err & ERR_TRACE) {
We should also check that the rest of |err| does indeed fit in
SECCCOMP_RET_DATA.

Maybe simply add a nested: "if (err & ~SECCOMP_RET_DATA) break;" ?

Or add it to the original "if" if you think it's readable.

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
File sandbox/linux/seccomp-bpf/errorcode.h (right):

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/errorcode.h:33: // If the progress is being ptraced
with PTRACE_O_TRACESECCOMP, then then
Nit: one "then"

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
File sandbox/linux/seccomp-bpf/errorcode_unittest.cc (right):

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/errorcode_unittest.cc:32: 
Could you add a death test if trying to use SECCOMP_RET_TRACE +
not_16_bits_value?

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
File sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc (right):

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1831: if
(!SandboxBPF::IsValidSyscallNumber(system_call_number)) {
This if is not needed anymore (due to a recent CL).

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1848: BPF_ASSERT(pid != -1);
BPF_ASSERT_NE()

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1852:
BPF_ASSERT(raise(SIGSTOP) == 0);
BPF_ASSERT_EQ() (etc. for the rest of the test).

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1867: BPF_ASSERT(0);
s/0/false/

https://chromiumcodereview.appspot.com/278583005/diff/20001/sandbox/linux/sec...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1876: // Give up (but don't
fail) if PTRACE_O_TRACESECCOMP is not supported.
It should now be supported anywhere seccomp-bpf is supported. I think it's ok to
not give up here. Did you notice anywhere where it's not true?

[rickyz (Google), 2014-05-20 22:34:01]

Thanks for the comments.  Yeah, it's pretty annoying that ptrace and signal
handler contexts give you registers in completely different structs (and I have
no idea why they decided to make values signed for 32 bit and unsigned for 64
bit).

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/errorcode.cc (right):

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/errorcode.cc:21: if (err & ERR_TRACE) {
On 2014/05/20 03:02:10, jln wrote:
> We should also check that the rest of |err| does indeed fit in
> SECCCOMP_RET_DATA.
> 
> Maybe simply add a nested: "if (err & ~SECCOMP_RET_DATA) break;" ?
> 
> Or add it to the original "if" if you think it's readable.
I changed the condition to (err & ~SECCOMP_RET_DATA) == ERR_TRACE.

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/errorcode.h (right):

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/errorcode.h:33: // If the progress is being ptraced
with PTRACE_O_TRACESECCOMP, then then
On 2014/05/20 03:02:10, jln wrote:
> Nit: one "then"

Done.

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
File sandbox/linux/seccomp-bpf/errorcode_unittest.cc (right):

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/errorcode_unittest.cc:32: 
On 2014/05/20 03:02:10, jln wrote:
> Could you add a death test if trying to use SECCOMP_RET_TRACE +
> not_16_bits_value?

Done.

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

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1831: if
(!SandboxBPF::IsValidSyscallNumber(system_call_number)) {
On 2014/05/20 03:02:10, jln wrote:
> This if is not needed anymore (due to a recent CL).

Done.

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1848: BPF_ASSERT(pid != -1);
On 2014/05/20 03:02:10, jln wrote:
> BPF_ASSERT_NE()

Done.

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1852:
BPF_ASSERT(raise(SIGSTOP) == 0);
On 2014/05/20 03:02:10, jln wrote:
> BPF_ASSERT_EQ() (etc. for the rest of the test).

Done.

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1867: BPF_ASSERT(0);
On 2014/05/20 03:02:10, jln wrote:
> s/0/false/

Done.

https://codereview.chromium.org/278583005/diff/20001/sandbox/linux/seccomp-bp...
sandbox/linux/seccomp-bpf/sandbox_bpf_unittest.cc:1876: // Give up (but don't
fail) if PTRACE_O_TRACESECCOMP is not supported.
On 2014/05/20 03:02:10, jln wrote:
> It should now be supported anywhere seccomp-bpf is supported. I think it's ok
to
> not give up here. Did you notice anywhere where it's not true?
Yeah, I don't see any release with seccomp-bpf and without
PTRACE_O_TRACESECCOMP, so removing this.

[rickyz (Google), 2014-05-29 00:20:46]

Friendly ping :-)

[jln (very slow on Chromium), 2014-05-29 21:44:53]

lgtm

This is great work, thanks Ricky. And sorry for the delays.

[rickyz (Google), 2014-05-29 22:04:28]

The CQ bit was checked by rickyz@google.com

[commit-bot: I haz the power, 2014-05-29 22:07:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rickyz@google.com/278583005/60001

[rickyz (Google), 2014-05-29 22:19:31]

The CQ bit was checked by rickyz@google.com

[commit-bot: I haz the power, 2014-05-29 22:24:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rickyz@google.com/278583005/80001

[rickyz (Google), 2014-05-30 17:45:28]

Thanks!  The try bots caught a couple of bugs when I put this on the commit
queue:

On ARM, struct pt_regs was already defined, so I switched to using the struct
(named user_regs_struct on x86/x86_64, and user_regs on ARM) from sys/user.h and
typedeffing it to regs_struct.
Then I found out that older Bionic libc doesn't have sys/user.h, so I defined
regs_struct directly in that case.

The test failed on the Linux try bots because I had the wrong value hardcoded
for PTRACE_EVENT_SECCOMP.  It turns out that when backporting seccomp-bpf to
3.2, Debian/Ubuntu changed the constant from 7 to 8 to avoid colliding with
PTRACE_EVENT_STOP (upstream changed PTRACE_EVENT_STOP to 128 instead).  The only
workaround I could think of was to accept either 7 or 8 when
PTRACE_EVENT_SECCOMP isn't defined.

Mind giving this another quick look over since I had to add some pretty hackish
stuff to fix these?

[jln (very slow on Chromium), 2014-06-05 00:01:43]

On 2014/05/30 17:45:28, Ricky Zhou wrote:
> Thanks!  The try bots caught a couple of bugs when I put this on the commit
> queue:
> 
> On ARM, struct pt_regs was already defined, so I switched to using the struct
> (named user_regs_struct on x86/x86_64, and user_regs on ARM) from sys/user.h
and
> typedeffing it to regs_struct.
> Then I found out that older Bionic libc doesn't have sys/user.h, so I defined
> regs_struct directly in that case.
> 
> The test failed on the Linux try bots because I had the wrong value hardcoded
> for PTRACE_EVENT_SECCOMP.  It turns out that when backporting seccomp-bpf to
> 3.2, Debian/Ubuntu changed the constant from 7 to 8 to avoid colliding with
> PTRACE_EVENT_STOP (upstream changed PTRACE_EVENT_STOP to 128 instead).  The
only
> workaround I could think of was to accept either 7 or 8 when
> PTRACE_EVENT_SECCOMP isn't defined.
> 
> Mind giving this another quick look over since I had to add some pretty
hackish
> stuff to fix these?

Adding Kees who should know about the Ubuntu 3.2 backport and the business with
PTRACE_EVENT_SECCOMP.

Any recommendation Kees?

[keescook, 2014-06-05 01:51:07]

On 2014/06/05 00:01:43, jln wrote:
> On 2014/05/30 17:45:28, Ricky Zhou wrote:
> > Thanks!  The try bots caught a couple of bugs when I put this on the commit
> > queue:
> > 
> > On ARM, struct pt_regs was already defined, so I switched to using the
struct
> > (named user_regs_struct on x86/x86_64, and user_regs on ARM) from sys/user.h
> and
> > typedeffing it to regs_struct.
> > Then I found out that older Bionic libc doesn't have sys/user.h, so I
defined
> > regs_struct directly in that case.
> > 
> > The test failed on the Linux try bots because I had the wrong value
hardcoded
> > for PTRACE_EVENT_SECCOMP.  It turns out that when backporting seccomp-bpf to
> > 3.2, Debian/Ubuntu changed the constant from 7 to 8 to avoid colliding with
> > PTRACE_EVENT_STOP (upstream changed PTRACE_EVENT_STOP to 128 instead).  The
> only
> > workaround I could think of was to accept either 7 or 8 when
> > PTRACE_EVENT_SECCOMP isn't defined.
> > 
> > Mind giving this another quick look over since I had to add some pretty
> hackish
> > stuff to fix these?
> 
> Adding Kees who should know about the Ubuntu 3.2 backport and the business
with
> PTRACE_EVENT_SECCOMP.
> 
> Any recommendation Kees?

Ooh, that's nasty. That's entirely my fault. :( Looks like it got lost in the
backporting.

$ cd /src/kernels/ubuntu/precise ; git grep PTRACE_EVENT_SECCOMP
...
include/linux/ptrace.h:#define PTRACE_EVENT_SECCOMP     8

$ cd /src/kernels/ubuntu/trusty ; git grep PTRACE_EVENT_SECCOMP
...
include/uapi/linux/ptrace.h:#define PTRACE_EVENT_SECCOMP        7

The "good" news is that fixing this in the Ubuntu kernel is technically a
regression fix, since PTRACE_EVENT_STOP is wrong too. I can send a patch to the
Ubuntu kernel team for precise.

[keescook, 2014-06-05 18:45:31]

On 2014/06/05 01:51:07, keescook wrote:
> The "good" news is that fixing this in the Ubuntu kernel is technically a
> regression fix, since PTRACE_EVENT_STOP is wrong too. I can send a patch to
the
> Ubuntu kernel team for precise.

Reported and patch sent to Ubuntu:

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1326905
https://lists.ubuntu.com/archives/kernel-team/2014-June/043473.html

seccomp regression tests updated as well (it was only using
PTRACE_O_TRACESECCOMP and not checking PTRACE_EVENT_SECCOMP):
https://github.com/kees/seccomp/tree/ptrace

[rickyz (Google), 2014-06-09 22:52:07]

On 2014/06/05 18:45:31, keescook wrote:
> On 2014/06/05 01:51:07, keescook wrote:
> > The "good" news is that fixing this in the Ubuntu kernel is technically a
> > regression fix, since PTRACE_EVENT_STOP is wrong too. I can send a patch to
> the
> > Ubuntu kernel team for precise.
> 
> Reported and patch sent to Ubuntu:
> 
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1326905
> https://lists.ubuntu.com/archives/kernel-team/2014-June/043473.html
> 
> seccomp regression tests updated as well (it was only using
> PTRACE_O_TRACESECCOMP and not checking PTRACE_EVENT_SECCOMP):
> https://github.com/kees/seccomp/tree/ptrace

Hey, thanks for the quick fix :-)  Do you know where this leaves us in terms of
how this change should handle the different possible values (keeping in mind
that Chrome won't be using SECCOMP_RET_TRACE)?  Are we better off checking for
both values for now, or do we not need to ever support those once the updates
come out upstream?

[Kees Cook, 2014-06-09 22:57:48]

For strict correctness, probably only the 1 value needs to be tested
once the fix lands. For compatibility, if the existing code that
checks for two works, probably you can keep it. :)

-Kees

On Mon, Jun 9, 2014 at 3:52 PM,  <rickyz@google.com> wrote:
> On 2014/06/05 18:45:31, keescook wrote:
>>
>> On 2014/06/05 01:51:07, keescook wrote:
>> > The "good" news is that fixing this in the Ubuntu kernel is technically
>> > a
>> > regression fix, since PTRACE_EVENT_STOP is wrong too. I can send a patch
>> > to
>> the
>> > Ubuntu kernel team for precise.
>
>
>> Reported and patch sent to Ubuntu:
>
>
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1326905
>> https://lists.ubuntu.com/archives/kernel-team/2014-June/043473.html
>
>
>> seccomp regression tests updated as well (it was only using
>> PTRACE_O_TRACESECCOMP and not checking PTRACE_EVENT_SECCOMP):
>> https://github.com/kees/seccomp/tree/ptrace
>
>
> Hey, thanks for the quick fix :-)  Do you know where this leaves us in terms
> of
> how this change should handle the different possible values (keeping in mind
> that Chrome won't be using SECCOMP_RET_TRACE)?  Are we better off checking
> for
> both values for now, or do we not need to ever support those once the
> updates
> come out upstream?
>
> https://codereview.chromium.org/278583005/



-- 
Kees Cook
Chrome OS Security

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rickyz (Google), 2014-06-11 00:28:19]

On 2014/06/09 22:57:48, Kees Cook wrote:
> For strict correctness, probably only the 1 value needs to be tested
> once the fix lands. For compatibility, if the existing code that
> checks for two works, probably you can keep it. :)
All right, leaving it since the trybots will likely still need it for a while. 
Trying CQ again.

[rickyz (Google), 2014-06-11 00:28:22]

The CQ bit was checked by rickyz@google.com

[commit-bot: I haz the power, 2014-06-11 00:32:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rickyz@google.com/278583005/150001

[rickyz (Google), 2014-06-11 11:40:20]

Heh, and Android manages to fail yet again.  The cause is:

SANDBOX_DEATH_TEST specifically checks that the test exited with status 1.
Die::SandboxDie uses LOG_FATAL, which calls abort().
On non-Android, we use base::RunUnitTestsUsingBaseTestSuite, which ends up using
logging::SetLogAssertHandler to override the behavior of LOG_FATAL to _exit(1).
On Android, we use a different code path to run tests which does not set
override the log assert handler.

I modified sandbox/linux/tests/main.cc to call LogAssertHandler on Android.

[rickyz (Google), 2014-06-11 11:40:25]

The CQ bit was checked by rickyz@google.com

[commit-bot: I haz the power, 2014-06-11 11:42:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rickyz@google.com/278583005/170001

[commit-bot: I haz the power, 2014-06-11 20:43:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 20:48:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:48:14]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[rickyz (Google), 2014-06-12 04:12:50]

The CQ bit was checked by rickyz@google.com

[commit-bot: I haz the power, 2014-06-12 04:16:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rickyz@google.com/278583005/170001

[commit-bot: I haz the power, 2014-06-12 09:16:31]

Change committed as 276595

[Anton, 2014-06-12 13:00:49]

On 2014/06/12 09:16:31, I haz the power (commit-bot) wrote:
> Change committed as 276595

FYI this change breaks the build for Android x64.
http://chromegw/i/chromium.fyi/builders/Android%20x64%20Builder%20%28dbg%29/b...

[Anton, 2014-06-12 13:53:59]

On 2014/06/12 13:00:49, Anton wrote:
> On 2014/06/12 09:16:31, I haz the power (commit-bot) wrote:
> > Change committed as 276595
> 
> FYI this change breaks the build for Android x64.
>
http://chromegw/i/chromium.fyi/builders/Android%20x64%20Builder%20%28dbg%29/b...

Suggested fix:
https://codereview.chromium.org/335623002