Last Comment Bug 790923 - (b2g-seccomp) B2G content process sandboxing via seccomp filter
(b2g-seccomp)
: B2G content process sandboxing via seccomp filter
Status: NEW
[tech-p2][leave open][sb-]
: meta
Product: Core
Classification: Components
Component: Security: Process Sandboxing (show other bugs)
: unspecified
: All Gonk (Firefox OS)
: -- normal with 3 votes (vote)
: ---
Assigned To: Jed Davis [:jld] [⏰PDT; UTC-7]
: Guillaume Destuynder [:kang]
Mentors:
https://wiki.mozilla.org/Security/San...
Depends on: 964455 969715 977902 1078971 1191915 1214947 912791 921817 925119 1.4-seccomp 930258 932107 936145 936252 936272 936320 943774 945330 960365 962769 964427 2.0-seccomp 969088 971370 1004011 1009995 1014299 1017393 1047620 1055310 1078838 1093893 1215303
Blocks: b2g-v-next b2gSystemSecurity 846047 909658
  Show dependency treegraph
 
Reported: 2012-09-13 04:26 PDT by Guillaume Destuynder [:kang]
Modified: 2016-03-16 08:44 PDT (History)
42 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nexus s seccomp filter support (72.74 KB, patch)
2012-09-13 04:26 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
user-space seccomp patch (27.25 KB, patch)
2012-09-13 04:46 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Unagi/msn7627a-nice patch for seccompfilter (72.55 KB, patch)
2013-04-09 14:06 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Keon/C8666 seccomp filter support (42.66 KB, patch)
2013-04-12 11:12 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Keon/C8666 seccomp filter support v2 (37.92 KB, patch)
2013-04-25 17:02 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
seccomp test program (8.56 KB, application/gzip)
2013-04-26 11:45 PDT, Guillaume Destuynder [:kang]
no flags Details
Keon/C8666 seccomp filter support v3 (36.90 KB, patch)
2013-04-26 17:18 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Gecko seccomp support (18.54 KB, patch)
2013-04-26 17:27 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Keon/C8666 seccomp filter support v3 (fixed file) (80.03 KB, patch)
2013-04-26 17:28 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Regenerated bionic's unistd.h (32.66 KB, patch)
2013-04-26 17:37 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Gecko seccomp support (24.41 KB, patch)
2013-04-30 17:54 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Gecko seccomp support (24.42 KB, patch)
2013-04-30 17:59 PDT, Guillaume Destuynder [:kang]
ian.melven: feedback+
Details | Diff | Review
Gecko seccomp support (23.76 KB, patch)
2013-05-03 14:21 PDT, Guillaume Destuynder [:kang]
ian.melven: feedback+
Details | Diff | Review
Unagi/msn7627a-nice patch for seccompfilter (164.85 KB, patch)
2013-05-07 13:06 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Gecko patch 0001 Add-seccomp-sandbox-support-for-content-processes. (28.46 KB, patch)
2013-06-05 01:07 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
0002-Enable-seccomp-sandbox-in-content-process - this actually enables supports for seccomp in content processes, granted that MOZ_CONTENT_SANDBOX=1 (1.77 KB, patch)
2013-06-05 01:09 PDT, Guillaume Destuynder [:kang]
ian.melven: review+
Details | Diff | Review
0003-Do-not-fail-when-seccomp-fails-to-enable - this make sure we don't fail if the kernel of the host doesn't support seccomp-bpf. This patch would be reverted in the future. (961 bytes, patch)
2013-06-05 01:14 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
0001-Initial-support-for-the-Seccomp-bpf-Sandbox (28.32 KB, patch)
2013-06-11 17:42 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
0002-Building-support-for-the-Seccomp-bpf-Sandbox (2.65 KB, patch)
2013-06-11 17:43 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
0003-Enable-the-Seccomp-bof-Sandbox-for-sandbox.patch (1.28 KB, patch)
2013-06-11 17:44 PDT, Guillaume Destuynder [:kang]
ian.melven: review+
Details | Diff | Review
0001-Initial-support-for-the-Seccomp-bpf-Sandbox (27.90 KB, patch)
2013-06-11 17:46 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
0004-Do-not-fail-when-seccomp-fails-to-enable (1.46 KB, patch)
2013-06-11 17:51 PDT, Guillaume Destuynder [:kang]
ian.melven: review+
Details | Diff | Review
Initial-support-for-the-Seccomp-bpf-Sandbox (32.53 KB, patch)
2013-06-13 11:51 PDT, Guillaume Destuynder [:kang]
ian.melven: review+
khuey: review+
Details | Diff | Review
Initial-support-for-the-Seccomp-bpf-Sandbox (32.20 KB, patch)
2013-06-24 15:18 PDT, Guillaume Destuynder [:kang]
gdestuynder: review+
Details | Diff | Review
Initial-support-for-the-Seccomp-bpf-Sandbox (33.19 KB, patch)
2013-06-27 17:09 PDT, Guillaume Destuynder [:kang]
brian: review-
dhylands: review+
Details | Diff | Review
Initial-support-for-the-Seccomp-bpf-Sandbox (29.87 KB, patch)
2013-07-23 16:49 PDT, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
android_ucontext.diff (971 bytes, patch)
2013-07-23 17:28 PDT, Guillaume Destuynder [:kang]
gal: review+
Details | Diff | Review
android_i386_ucontext.diff (663 bytes, patch)
2013-07-23 17:29 PDT, Guillaume Destuynder [:kang]
gal: review+
Details | Diff | Review
android_arm_ucontext.diff (658 bytes, patch)
2013-07-23 17:31 PDT, Guillaume Destuynder [:kang]
gal: review+
Details | Diff | Review
linux_seccomp.diff (3.46 KB, patch)
2013-07-23 17:33 PDT, Guillaume Destuynder [:kang]
dkeeler: review+
Details | Diff | Review
Initial-support-for-the-Seccomp-bpf-Sandbox (106.45 KB, patch)
2013-07-23 19:03 PDT, Guillaume Destuynder [:kang]
dkeeler: review+
Details | Diff | Review
Initial-support-for-the-Seccomp-bpf-Sandbox (109.53 KB, patch)
2013-08-06 15:40 PDT, Guillaume Destuynder [:kang]
gerv: review+
Details | Diff | Review
Seccomp-bpf cherry-pickable commits for kernel 3.4, from http://git.chromium.org/chromiumos/third_party/kernel.git origin/chromeos-3.4 (2.72 KB, text/plain)
2013-09-19 04:42 PDT, Guillaume Destuynder [:kang]
no flags Details
generic_seccomp-bpf_3.0.x (same as nice/keon/nexus patch without defconfig changes) (78.86 KB, patch)
2013-12-16 03:04 PST, Guillaume Destuynder [:kang]
no flags Details | Diff | Review
Alternate list of kernel commits to cherry-pick for 3.4 (2.70 KB, text/plain)
2014-05-13 10:50 PDT, Jed Davis [:jld] [⏰PDT; UTC-7]
no flags Details

Description Guillaume Destuynder [:kang] 2012-09-13 04:26:02 PDT
Created attachment 660787 [details] [diff] [review]
nexus s seccomp filter support

This is an implementation of seccomp filter mode for sandboxing content processes.

seccomp allows filtering kernel system calls and thus needs kernel support. The regular seccomp mode allows only read(), write(), exit() and signal() system calls.

Since kernel 3.5, a new mode, called filter mode or mode 2 allows using a whitelist of user selected system calls instead (and is thus much more flexible) on x86.

Older kernels, and ARM kernels need patching to support this functionality.

I have ported it and tested it with the Nexus S kernel (my development device) with code available here: https://github.com/gdestuynder/android_kernel_samsung_crespo/commits/ics and as patch to this bug

Patch for the user-space to follow in the next comment.
Comment 1 Guillaume Destuynder [:kang] 2012-09-13 04:46:05 PDT
Created attachment 660797 [details] [diff] [review]
user-space seccomp patch

This is a patch to the user space to implement the aforementioned seccomp filter.

Currently, this will probably kill applications attempting to access non-whitelisted system calls (most of them do).

The important part is in dom/ipc/ContentProcess.cpp

Things that need cleanup:

- proper syscall reporter for debugging (with proper syscall list generation for the target platform)
- argue and fix the list of system calls that are whitelisted
- may want to find a different initialization point than ContentProcess::Init() if that doesn't work out (ie if all initialization tasks didn't happen prior to that)
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2013-02-25 23:53:32 PST
Will arguably boost security better than some competitors, but our sandbox will be somewhat leaky until we have sandboxed webgl.
Comment 3 Guillaume Destuynder [:kang] 2013-04-09 13:41:46 PDT
I've built and patched a unagi-like kernel with https://bugzilla.mozilla.org/attachment.cgi?id=660787 today, it works with no change except defconfig.

i'm not sure how sensitive this is or not and if i can send a patch to that here, so i'm marking it a qualcom sensitive+moco bug. Feel free to revert if that's possible.
Comment 4 Guillaume Destuynder [:kang] 2013-04-09 14:06:45 PDT
Created attachment 735403 [details] [diff] [review]
Unagi/msn7627a-nice patch for seccompfilter
Comment 5 Guillaume Destuynder [:kang] 2013-04-12 11:12:21 PDT
Created attachment 736882 [details] [diff] [review]
Keon/C8666 seccomp filter support
Comment 6 Guillaume Destuynder [:kang] 2013-04-25 17:02:43 PDT
Created attachment 742096 [details] [diff] [review]
Keon/C8666 seccomp filter support v2

This fixes the handling of SIGSYS, allowing signal based reporters to work correctly.

Note: nexus/unagi patches need a similar fix
Comment 7 Guillaume Destuynder [:kang] 2013-04-26 11:45:42 PDT
Created attachment 742483 [details]
seccomp test program

Not test units, just a small program to experiment easily on android / B2G
Comment 8 Guillaume Destuynder [:kang] 2013-04-26 17:18:02 PDT
Created attachment 742628 [details] [diff] [review]
Keon/C8666 seccomp filter support v3

Nicer Keon patch, include samples, doc and audit support
Comment 9 Guillaume Destuynder [:kang] 2013-04-26 17:27:04 PDT
Created attachment 742630 [details] [diff] [review]
Gecko seccomp support

Support a functional content-process sandbox.
Note: this sandbox is wide open and does not provide much security at the moment. It is however fully functional, with reporting.

Depending on the platform KILL_PROCESS might need to be changed into TRAP_PROCESS.
The exit in the reporter() can also be removed, however, TRAP_PROCESS will still deny the system call, and the process may behave unexpectedly.

The sandbox here is started in gecko/dom/ipc/ContentProcess.cpp, in ContentProcess::Init(). However this is subject to change as a lot of additional Gecko initialization happens after this.
Comment 10 Guillaume Destuynder [:kang] 2013-04-26 17:28:05 PDT
Created attachment 742631 [details] [diff] [review]
Keon/C8666 seccomp filter support v3 (fixed file)
Comment 11 Guillaume Destuynder [:kang] 2013-04-26 17:37:20 PDT
Created attachment 742635 [details] [diff] [review]
Regenerated bionic's unistd.h

This is necessary to compile seccomp's whitelist for ARM with bionic (android/gonk libc) headers.

The list has been regenerated from kernel headers. Not sure about the GPL part. Google laywers seems to say its ok to do that. Maybe someone has more insight than me.
Comment 12 Guillaume Destuynder [:kang] 2013-04-30 17:54:05 PDT
Created attachment 743984 [details] [diff] [review]
Gecko seccomp support

This version of the patch initializes seccomp just after the last SetCurrentProcessPrivileges() which appears to be the current best place to do so.

It also supports Desktop seccomp whitelist.
Comment 13 Guillaume Destuynder [:kang] 2013-04-30 17:59:10 PDT
Created attachment 743987 [details] [diff] [review]
Gecko seccomp support
Comment 14 Ian Melven :imelven 2013-05-03 12:01:38 PDT
Comment on attachment 743987 [details] [diff] [review]
Gecko seccomp support

Review of attachment 743987 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looks good ! 

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F and
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed have some good general guidance on how to format a patch for m-c

it looks like seccomp_filter.h android_seccomp_filter.h and seccomp_filter.h are changed multiple times in the same patch.. the settings above
should sort that out

i'm fine with sticking the seccomp stuff in ipc/chromium/src/base, others may have stronger opinions

::: ipc/chromium/src/base/linux_seccomp.h
@@ +237,5 @@
> +        BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_TRAP)
> +
> +#define ALLOW_PROCESS \
> +        BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW)
> +

maybe we might as well add TRACE_PROCESS with SECCOMP_RET_TRACE too ?

::: dom/ipc/ContentChild.cpp
@@ +540,2 @@
>    SetCurrentProcessPrivileges(privs);
> +  SetCurrentProcessSandbox();

I need to look and see if we get sent a SetProcessPrivileges on desktop, I suspect not. We discussed having a separate SetProcessSandbox message but I think it's fine to make that change later.

::: ipc/chromium/src/base/process_util_linux.cc
@@ +125,5 @@
> +
> +        if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &seccomp_prog))
> +          return 1;
> +        return 0;
> +    }

mixed 2 space and 4 space indentation here, looks like most of the file is 2 space (as it should be per https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)

@@ +398,5 @@
>      gProcessLog.print("==> could not chdir()\n");
>  }
>  
> +void SetCurrentProcessSandbox(void) {
> +  if (install_syscall_reporter()) {

do we want to install the reporter by default for now ?
Comment 15 Guillaume Destuynder [:kang] 2013-05-03 14:21:25 PDT
Created attachment 745349 [details] [diff] [review]
Gecko seccomp support

Since SetCurrentProcessSandbox() its standalone it can be called anywhere. On Desktop we can call it in ContentProcess::Init(). In fact in B2G it should also move there eventually. (need feedback for this)
I prefer this approach to sending a message telling the process to start the sandbox if we can - since there's less chance for the sandbox to be left uninitialized.

fixed whitespace, added trace and errno support
reporter and trap only enabled in debug
Comment 16 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-03 14:28:40 PDT
(In reply to Ian Melven :imelven from comment #14)
> i'm fine with sticking the seccomp stuff in ipc/chromium/src/base, others
> may have stronger opinions

It would be nice to avoid adding anything new to ipc/chromium/* because there is some implication that everything in ipc/chromium/* is/was imported from Chromium. I think it is good to maintain some distinction between what we've imported from Chromium and what is Mozllla-originated, especially in case we need to merge the chromium/ipc code to/from upstream at some point.
Comment 17 Guillaume Destuynder [:kang] 2013-05-03 14:32:08 PDT
I put the privilege code in there as well in the past (its still in there in m-c) so I thought it's a good place - it already has mozilla specific code, but this code is not necessarily gecko-specific.
I don't have a strong opinion on where it should sit, I could even put it in dom/ipc or /sandbox/ or something  like that
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-05-03 16:57:58 PDT
ipc/chromium was our original pull from chromium. We've since had to (grudgingly) make changes, but I fantasize about the day that I get to 'rm -rf' it. Please let's find some other place.
Comment 19 Ian Melven :imelven 2013-05-06 11:01:19 PDT
Comment on attachment 745349 [details] [diff] [review]
Gecko seccomp support

Review of attachment 745349 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm not sure you're using -U 8 (8 lines of context around each change) but everything else
looks right :)
Comment 20 Ian Melven :imelven 2013-05-06 11:02:47 PDT
(In reply to Ian Melven :imelven from comment #19)
> Comment on attachment 745349 [details] [diff] [review]
> Gecko seccomp support
> 
> Review of attachment 745349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I'm not sure you're using -U 8 (8 lines of context around each
> change) but everything else
> looks right :)

oh yeah, obviously we should move the stuff out of ipc/chromium as Ben asks in comment 18 :)
Comment 21 Ian Melven :imelven 2013-05-06 11:06:08 PDT
Guillaume: i'd suggest maybe sandbox/linux or sandbox/seccomp perhaps ? Like I said I don't feel strongly, just trying to push this forward.
Comment 22 Guillaume Destuynder [:kang] 2013-05-06 14:09:32 PDT
The keon patch pull request is here https://github.com/gp-b2g/gp-keon-kernel/pull/1

:imelven I suppose I'll go with sandbox/seccomp-bpf.
If we  need to extend more we can do it when the code exists(like sandbox/linux/seccomp-bpf)

I think one difference with our seccomp implementation and other project's like OpenSSH and Chromium is that we initialize it "late" in the process, something that doesnt work for rlimit or chroot very well for example (need root), or even selinux,  rsbac, grsec rbac, etc. (albeit it can be made to work that way for selinux/rsbac)

note: im not sure that the move is obviously, ipc/chromium has a bunch of custom mozilla code already. I don't mind at all tho :) I'll do it soon.
Comment 23 Guillaume Destuynder [:kang] 2013-05-07 11:28:33 PDT
Removed confidential flags as per :mwu - unagi patch is based off public code
Comment 24 Guillaume Destuynder [:kang] 2013-05-07 13:06:08 PDT
Created attachment 746594 [details] [diff] [review]
Unagi/msn7627a-nice patch for seccompfilter

Updated the unagi patch to the same level as the Keon patch (ie full seccomp bpf support)
Comment 25 Ian Melven :imelven 2013-05-14 14:48:53 PDT
Capturing some previous discussions here:

We (Guillaume and I) think we would like the Gecko patch to put seccomp-bpf behind a compile time switch/define for desktop right now. We might also want to only switch it on for desktop when we're doing an e10s build (e.g. #if defined(SECCOMP_SANDBOX) && #if defined(E10S) n.b. these are not the actual correct definitions to check ;) ).

We would like seccomp-bpf to be on by default for B2G. The rationale here is the sooner we get it enabled, the sooner we will start catching things that should be blocked.

We need to add logic that degrades gracefully if seccomp is not available (ie the prctl to enable seccomp fails) for when we're running (either on B2G or desktop) with a kernel that does not support seccomp-bpf.
Comment 26 Guillaume Destuynder [:kang] 2013-05-14 16:27:00 PDT
currently i have (compared to the current latest patch) a --enable-seccomp-sandbox in configure.in (enabled by default for B2G, else off) and it does not fail if the kernel doesn't support seccomp (even thus ideally it should, but that's not realistic right now).

I'm planning to put most of the code in /security/sandbox when i figured the build system and propose that as a patch
Comment 27 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-15 18:49:52 PDT
(In reply to Ian Melven :imelven from comment #25)
> We (Guillaume and I) think we would like the Gecko patch to put seccomp-bpf
> behind a compile time switch/define for desktop right now. We might also
> want to only switch it on for desktop when we're doing an e10s build (e.g.
> #if defined(SECCOMP_SANDBOX) && #if defined(E10S) n.b. these are not the
> actual correct definitions to check ;) ).

if SECCOMP_SANDBOX is the flag that controls whether or not sandboxing is enabled, then configure.in should have a check that causes configure to fail when SECCOMP_SANDBOX is set but the E10S flag is not set.

> We would like seccomp-bpf to be on by default for B2G. The rationale here is
> the sooner we get it enabled, the sooner we will start catching things that
> should be blocked.

+1.

> We need to add logic that degrades gracefully if seccomp is not available
> (ie the prctl to enable seccomp fails) for when we're running (either on B2G
> or desktop) with a kernel that does not support seccomp-bpf.

On B2G at least, we should *not* degrade gracefully when SECCOMP_SANDBOX is set. Ideally the only time seccomp should be disabled is when the person building B2G has explicitly disabled it (presumably because they are building for a device that doesn't have a new enough kernel). If we were to gracefully degrade then we could easily get in the situation where a bug causes us to accidentally never use seccomp. It is better to fail safe.

(In reply to Guillaume Destuynder [:kang] from comment #26)
> currently i have (compared to the current latest patch) a
> --enable-seccomp-sandbox in configure.in (enabled by default for B2G, else
> off) and it does not fail if the kernel doesn't support seccomp (even thus
> ideally it should, but that's not realistic right now).

Is a configure flag the right thing for this? It seems like lately we've been discouraging such configure flags. Also, on B2G, the only (AFAICT) mechanism for customizing the Gecko build is editing .userconfig, and I don't think that .userconfig has a way to pass flags to Gecko's configure.

> 
> I'm planning to put most of the code in /security/sandbox when i figured the
> build system and propose that as a patch

I suggest that you grep everything for security/build and you add a corresponding line for security/sandbox alongside it. In particular:

1. in build/dumbbuild-dependencies:

           security/build
        +  security/sandbox
           security/manager

2. in toolkit.mozbuild:

         if CONFIG['MOZ_PSM'] and not CONFIG['MOZ_NATIVE_NSS']:
             add_tier_dir('nss', 'security/build')

        +if CONFIG['MOZ_SANDBOX']:
            add_tier_dir('sandbox', 'security/sandbox')

I am skeptical that this will work, but it is what I would do to start with.
Comment 28 Guillaume Destuynder [:kang] 2013-06-05 01:07:39 PDT
Created attachment 758439 [details] [diff] [review]
Gecko patch 0001 Add-seccomp-sandbox-support-for-content-processes.

This would be how the patch would be like.
Things I need feedback for in particular:

* location on the filesystem/source code, is that ok or do you prefer /security/sandbox/seccomp-bpf? (if you don't care, that's ok then :)

* error msgs, currently uses stderr for compatibility. I saw various way to handle error messages depending where they're made in Gecko and I'm not sure what is generally expected, specially since it's not pure Android/Gonk code, and it doesn't touch xpcom either

* namespace. I figured that since we have at least Hal and Chromium components already using "sandbox" names (they don't directly collide), I might as well introduce mozilla::security. If that's wrong or useless, I'd happily use mozilla.

Thanks!
Comment 29 Guillaume Destuynder [:kang] 2013-06-05 01:09:22 PDT
Created attachment 758440 [details] [diff] [review]
0002-Enable-seccomp-sandbox-in-content-process - this actually enables supports for seccomp in content processes, granted that MOZ_CONTENT_SANDBOX=1
Comment 30 Guillaume Destuynder [:kang] 2013-06-05 01:14:28 PDT
Created attachment 758441 [details] [diff] [review]
0003-Do-not-fail-when-seccomp-fails-to-enable - this make sure we don't fail if the kernel of the host doesn't support seccomp-bpf. This patch would be reverted in the future.

This patch would be reverted when all upcoming devices have seccomp-bpf patch ready and/or we have the seccomp-bpf patch in the "must have" patches for vendors.
Without this patch, running b2g master would fail on all devices without seccomp-bpf support.

The alternative is to not default MOZ_CONTENT_SANDBOX to 1 on Gonk - however, if we do that, the use and awareness will be very limited.

Right now, Keon and Peak have the seccomp-bpf merged upstream.
Comment 31 David Keeler [:keeler] (use needinfo?) 2013-06-05 09:22:55 PDT
(In reply to Guillaume Destuynder [:kang] from comment #28)
> Created attachment 758439 [details] [diff] [review]
> Gecko patch 0001 Add-seccomp-sandbox-support-for-content-processes.
> 
> * error msgs

One option might be to use the NSPR logging functionality: https://developer.mozilla.org/en-US/docs/NSPR_API_Reference/Logging
I don't know if you want to avoid linking against NSPR, though.
Comment 32 Guillaume Destuynder [:kang] 2013-06-11 17:42:00 PDT
Created attachment 761240 [details] [diff] [review]
0001-Initial-support-for-the-Seccomp-bpf-Sandbox
Comment 33 Guillaume Destuynder [:kang] 2013-06-11 17:43:15 PDT
Created attachment 761241 [details] [diff] [review]
0002-Building-support-for-the-Seccomp-bpf-Sandbox
Comment 34 Guillaume Destuynder [:kang] 2013-06-11 17:44:01 PDT
Created attachment 761242 [details] [diff] [review]
0003-Enable-the-Seccomp-bof-Sandbox-for-sandbox.patch
Comment 35 Guillaume Destuynder [:kang] 2013-06-11 17:46:23 PDT
Created attachment 761244 [details] [diff] [review]
0001-Initial-support-for-the-Seccomp-bpf-Sandbox

Sorry, wrong patch. Fixed.
Comment 36 Guillaume Destuynder [:kang] 2013-06-11 17:51:32 PDT
Created attachment 761245 [details] [diff] [review]
0004-Do-not-fail-when-seccomp-fails-to-enable

and that's the last one.

Patches to review are:
0001 (support for seccomp bpf sandbox)
0002 (build system for seccomp bpf sandbox)
0003 (enable seccomp bpf sandbox in content-processes)
0004 (do not fail when seccomp bpf sandbox fail to enable/lack of kernel support)

0004 will be reverted via bug 880797 when possible (see comments in the code)
0003 will be moved to a better location via bug 880808 when possible

if reviews pass I intend to also get reviews from a couple of B2G people.
Comment 37 Ian Melven :imelven 2013-06-12 17:11:59 PDT
Comment on attachment 761245 [details] [diff] [review]
0004-Do-not-fail-when-seccomp-fails-to-enable

Review of attachment 761245 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me.
Comment 38 Ian Melven :imelven 2013-06-12 17:47:41 PDT
Comment on attachment 761244 [details] [diff] [review]
0001-Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 761244 [details] [diff] [review]:
-----------------------------------------------------------------

Some small comments, mostly about the.. comments.

security/sandbox is fine with me, when we do Windows etc. on desktop, we might want security/sandbox/windows, i think we also
talked about security/sandbox/seccomp/ before. Naming the files Sandbox.h / Sandbox.cpp might also be confusing when we do
other platforms..

::: security/sandbox/Makefile.in
@@ +23,5 @@
> +  $(NULL)
> +
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk
> +include $(topsrcdir)/config/rules.mk

seems fine to me, but we should have a build peer check out the build stuff overall. it might make more sense to break out all the build stuff into one patch,
all the seccomp stuff into another patch etc. in terms of getting reviews, or maybe just something to keep in mind going forward.

if the sandbox stuff is large, we may also want it not to be in libxul long term.. others are better informed to give guidance on that than me though.

::: security/sandbox/Sandbox.cpp
@@ +93,5 @@
> +}
> +
> +void SetCurrentProcessSandbox(void)
> +{
> +#ifdef MOZ_CONTENT_SANDBOX

seems fine to me. We'll use the same #define on desktop :)

@@ +113,5 @@
> +  if (InstallSyscallFilter()) {
> +#ifdef PR_LOGGING
> +    PR_LOG(gSeccompSandboxLog, PR_LOG_DEBUG, ("install_syscall_filter() failed\n"));
> +#endif
> +    _exit(127);

a note to other reviewers that patch-004 comments this exit() out so unavailability of seccomp-bpf won't be fatal initially.

::: security/sandbox/android_arm_ucontext.h
@@ +1,5 @@
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +/* This file has been imported from 

the dreaded trailing whitespace

@@ +14,5 @@
> +
> +// We also need greg_t for the sandbox, include it in this header as well.
> +typedef unsigned long greg_t;
> +
> +//typedef unsigned long sigset_t;

in general, remove commented out code before committing.

::: security/sandbox/android_seccomp_filter.h
@@ +21,5 @@
> +#define SECCOMP_WHITELIST_ADD
> +#endif
> +
> +/* Most used system calls should be at the top of the whitelist
> + * for performance reasons. The whitelist BPF filter exits on any

good comment re performance. Not to bikeshed the comment, but maybe 'exits after processing any ALLOW_SYSCALL macro'

@@ +23,5 @@
> +
> +/* Most used system calls should be at the top of the whitelist
> + * for performance reasons. The whitelist BPF filter exits on any
> + * ALLOW_SYSCALL macro.
> + * Current list order could be further optimized.

i don't think you need to point this out, unless the real situation is
'this list is not currently optimized in that way at all', which you should say, if so.

@@ +27,5 @@
> + * Current list order could be further optimized.
> + * How are those syscalls found?
> + * 1) via strace -p <child pid> or/and
> + * 2) with MOZ_DEBUG set, the child will report which system call
> + *    has been denied by seccomp-bpf, just before exiting

maybe be specific and say, will report to stdout/console

@@ +30,5 @@
> + * 2) with MOZ_DEBUG set, the child will report which system call
> + *    has been denied by seccomp-bpf, just before exiting
> + * System call number to name mapping is found in:
> + * bionic/libc/kernel/arch-arm/asm/unistd.h
> + * or your libc's unistd.h/kernel headers.

another very useful comment, kudos.

@@ +75,5 @@
> +  ALLOW_SYSCALL(getdents64), \
> +  /* Should be removed in the future, if possible */ \
> +  ALLOW_SYSCALL(rt_sigreturn), \
> +  ALLOW_SYSCALL(sigreturn), \
> +  ALLOW_SYSCALL(sigprocmask)

good prioritization of the sets of syscalls. At some point we should probably make a wiki page with our reasoning for why things are OK/
must remove/should be removed and have this list point to it.

::: security/sandbox/linux_seccomp.h
@@ +1,5 @@
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +/* This file has been initially imported from 

trailing whitespace

@@ +79,5 @@
> +#ifndef BPF_XOR
> +#define BPF_XOR                    0xA0
> +#endif
> +
> +// In order to build will older tool chains, we currently have to avoid

s/will/with

@@ +80,5 @@
> +#define BPF_XOR                    0xA0
> +#endif
> +
> +// In order to build will older tool chains, we currently have to avoid
> +// including <linux/seccomp.h>. Until that can be fixed (if ever). Rely on

Until that can be fixed (if ever), we rely on

@@ +81,5 @@
> +#endif
> +
> +// In order to build will older tool chains, we currently have to avoid
> +// including <linux/seccomp.h>. Until that can be fixed (if ever). Rely on
> +// our own definitions of the seccomp kernel ABI.

own copy of the definitions

(ie please make it explicit we didn't just make these up)

@@ +85,5 @@
> +// our own definitions of the seccomp kernel ABI.
> +#ifndef SECCOMP_MODE_FILTER
> +#define SECCOMP_MODE_DISABLED         0
> +#define SECCOMP_MODE_STRICT           1
> +#define SECCOMP_MODE_FILTER           2  // User user-supplied filter

doubled 'user' ?

::: security/sandbox/moz.build
@@ +12,5 @@
> +
> +CPP_SOURCES += [
> +    'Sandbox.cpp',
> +]
> +

same comment re a build peer looking at the build stuff.

::: security/sandbox/seccomp_filter.h
@@ +4,5 @@
> + */
> +
> +#include "linux_seccomp.h"
> +
> +/* This is the actual seccomp whitelist. */

probably want a "this is used for Firefox desktop linux content processes" here like there
is in android_seccomp_filter.h

@@ +21,5 @@
> +
> +/* Most used system calls should be at the top of the whitelist
> + * for performance reasons. The whitelist BPF filter exits on any
> + * ALLOW_SYSCALL macro.
> + * Current list order could be further optimized.

same comment on these comments as in the android file.

is it worth splitting these into two separate files ? also if this is for the desktop stuff
why are their ARM calls above this ?
Comment 39 Ian Melven :imelven 2013-06-12 17:51:37 PDT
Comment on attachment 761242 [details] [diff] [review]
0003-Enable-the-Seccomp-bof-Sandbox-for-sandbox.patch

Review of attachment 761242 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me, with some slight comment nitpicks. I can't wait to see what happens when we push this all to try :)

::: dom/ipc/ContentChild.cpp
@@ +543,5 @@
>                            aPrivs;
>    // If this fails, we die.
>    SetCurrentProcessPrivileges(privs);
> +#ifdef MOZ_CONTENT_SANDBOX
> +  /* SetCurrentProcessSandbox should be moved at process initialization time

moved as close to process initializtion time

@@ +544,5 @@
>    // If this fails, we die.
>    SetCurrentProcessPrivileges(privs);
> +#ifdef MOZ_CONTENT_SANDBOX
> +  /* SetCurrentProcessSandbox should be moved at process initialization time
> +   * if/when possible. SetCurrentProcessPrivileges should probably be moved

as possible.

@@ +545,5 @@
>    SetCurrentProcessPrivileges(privs);
> +#ifdef MOZ_CONTENT_SANDBOX
> +  /* SetCurrentProcessSandbox should be moved at process initialization time
> +   * if/when possible. SetCurrentProcessPrivileges should probably be moved
> +   * as well. Right now this is set ONLY if we receive the 

trailing whitespace
Comment 40 Ian Melven :imelven 2013-06-12 17:54:01 PDT
Comment on attachment 758440 [details] [diff] [review]
0002-Enable-seccomp-sandbox-in-content-process - this actually enables supports for seccomp in content processes, granted that MOZ_CONTENT_SANDBOX=1

Review of attachment 758440 [details] [diff] [review]:
-----------------------------------------------------------------

Works for me.

::: dom/ipc/ContentChild.cpp
@@ +540,4 @@
>    ChildPrivileges privs = (aPrivs == PRIVILEGES_DEFAULT) ?
>                            GeckoChildProcessHost::DefaultChildPrivileges() :
>                            aPrivs;
> +  // If any of these fails, we die.

sorry to keep nitpicking, but 'If any of these fail, we die'
Comment 41 Ian Melven :imelven 2013-06-12 18:16:34 PDT
Comment on attachment 761241 [details] [diff] [review]
0002-Building-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 761241 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +5179,5 @@
> +[  --enable-content-sandbox        Enable sandboxing support for content-processes],
> +    MOZ_CONTENT_SANDBOX=1,
> +    MOZ_CONTENT_SANDBOX= )
> +
> +if test -n "$gonkdir"; then

fine with me, although build peers may have other suggestions on how to know if we're building B2G/Gonk.
Comment 42 Ian Melven :imelven 2013-06-12 18:33:44 PDT
I noticed that these patches aren't formatted exactly as they should be.. again I'll point to http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed in particular the diff = -p -U 8 part :)
Comment 43 Guillaume Destuynder [:kang] 2013-06-13 11:51:50 PDT
Created attachment 762193 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox
Comment 44 Guillaume Destuynder [:kang] 2013-06-13 11:53:17 PDT
Discussed this on IRC - decided to unify the patches except 0004-Do-not-fail-when-seccomp-fails-to-enable to avoid confusion.

I fixed :imelven's comments except for issues that were in upstream files from Chromium (those are noted as being taken from upstream in the header)

I also changed the diff format to what appears to be as similar as possible to the hg settings.

What's needed to build:

742635: Regenerated bionic's unistd.h
762193: Initial-support-for-the-Seccomp-bpf-Sandbox

What's to be landed after review:
742635: Regenerated bionic's unistd.h
762193: Initial-support-for-the-Seccomp-bpf-Sandbox
761245: 0004-Do-not-fail-when-seccomp-fails-to-enable
Comment 45 Ian Melven :imelven 2013-06-14 20:20:49 PDT
Comment on attachment 762193 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Looks good to me. Time to get a build peer to look at the build stuff as well as other appropriate reviewers for the modifications of existing code and poke Brian to have a look at it too, I think.
Comment 46 Guillaume Destuynder [:kang] 2013-06-17 15:43:39 PDT
Comment on attachment 762193 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 762193 [details] [diff] [review]:
-----------------------------------------------------------------

Adding Khuey specially for the build system part and the /dom/ipc change.
Comment 47 Kyle Huey [:khuey] (khuey@mozilla.com) 2013-06-24 09:54:05 PDT
Comment on attachment 762193 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 762193 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +5181,5 @@
> +    MOZ_CONTENT_SANDBOX= )
> +
> +if test -n "$gonkdir"; then
> +    MOZ_CONTENT_SANDBOX=1
> +fi

You want this above MOZ_ARG_ENABLE_BOOL.  As you've written it the configure option does nothing on B2G.
Comment 48 Guillaume Destuynder [:kang] 2013-06-24 15:18:32 PDT
Created attachment 766928 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox
Comment 49 Guillaume Destuynder [:kang] 2013-06-24 15:29:21 PDT
Comment on attachment 766928 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 766928 [details] [diff] [review]:
-----------------------------------------------------------------

carrying over r+ from imelven and khuey
addressed comment 47
Comment 50 [:mmc] Monica Chew (no longer reading bugmail) 2013-06-24 19:14:24 PDT
Comment on attachment 766928 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 766928 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Guillaume,

Hope you don't mind some drive-by comments. I was so excited that you are working on this that I couldn't help but take a look :)

- When importing stuff from 3rd party sources, reviewers have generally asked me for shell scripts or at least READMEs to keep things up to date. It seems like a good idea for the *_context.h files, anyway.
- Some high level comments, or a link to a wiki where you describe how seccomp consumes all its filters, would be most welcome :) In particular functions that you expect non-security/sandbox code to call need to be documented so that new readers know what's going on.
- Tests? It would be great proof of concept to show how the sandbox works to have a test with calls that get caught by seccomp, etc. I notice the Chromium code does have sandbox tests (https://codereview.chromium.org/12207029/patch/3014/4003), and someone (BenWa?) recently imported GUnit, so maybe it is possible.

All the best,
Monica

::: security/sandbox/Sandbox.cpp
@@ +102,5 @@
> +#endif
> +
> +#ifdef MOZ_DEBUG
> +  if (InstallSyscallReporter()) {
> +#ifdef PR_LOGGING

I think PR_LOG is defined to nothing if undef DEBUG, so maybe these conditional compiles aren't doing anything?

http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prlog.h#186

::: security/sandbox/Sandbox.h
@@ +10,5 @@
> +namespace mozilla {
> +  // Starts the Seccomp sandbox for this process.
> +  // Generally called just after SetCurrentProcessPrivileges.
> +  // Should be called only once, and before any potentially harmful content is loaded.
> +  void SetCurrentProcessSandbox(void);

no comment love for InstallSyscallFilter and Reporter? :)
It looks like InstallSyscallFilter and InstallSyscallReporter and Reporter are only used in Sandbox.cpp, in which case why export them. I still think they need comments in Sandbox.cpp, though.
Comment 51 Guillaume Destuynder [:kang] 2013-06-24 20:46:52 PDT
I gladly take any feedback no worries :)

Imported files:
I did specify where they came from exactly in the comment header, but I don't think those should be strictly updated. Do we generally update them anyway, just for the sake of updating? (its a serious question - if so and we generally want scripts, I'll add a script that wget/curl em or whatever is the accepted practice.)

Documentation:
It's here: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Security/System_security
I could add the link in the source code as well if that's common practice, although I usually fear those URLs change over time.
The function that non-security/sandbox code may call is actually _also_ described in the code with a lengthy comment specifying how to use it - is that not sufficient?

Tests:
Chrome's implementation of seccomp is slightly different - not sure what Gunit does, but tests are possible - chrome tests their custom compiler works in particular (our compiler is as straight forward as possible). If you have hints of what tests are generally expected tho, I'll try to add that.
2 things I can think of are:
- check the content process has seccomp actually enabled (thats also quite useful of a check :P)
- check the calls are properly denied (although with the current compiler, its hard to get that wrong but it's always nice to have)

PR_LOGGING:
yes it's only defined if debug is also defined. I copied the behavior from files I found. If you're sure checking for PR_LOGGING being enabled is not necessary I can remove it.

InstallSyscallFilter/Reporter:
Ok, I'll address that tomorrow :)
Comment 52 Guillaume Destuynder [:kang] 2013-06-27 17:09:53 PDT
Created attachment 768663 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Tested on try and so on, includes changes from previous comment.
Comment 53 Guillaume Destuynder [:kang] 2013-06-27 17:11:33 PDT
Comment on attachment 761245 [details] [diff] [review]
0004-Do-not-fail-when-seccomp-fails-to-enable

Review of attachment 761245 [details] [diff] [review]:
-----------------------------------------------------------------

Included in patch for simplicity. I'll make 2 separate commits when actually committing.
Comment 54 Dave Hylands [:dhylands] 2013-07-03 10:47:46 PDT
Comment on attachment 768663 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 768663 [details] [diff] [review]:
-----------------------------------------------------------------

Are we considering sandboxing the main process as well?

It would obviously use more system calls, but it still seems like a reasonable thing to do.

::: security/sandbox/Sandbox.cpp
@@ +39,5 @@
> +  len: (unsigned short)(sizeof(seccomp_filter)/sizeof(seccomp_filter[0])),
> +  filter: seccomp_filter,
> +};
> +
> +void Reporter(int nr, siginfo_t *info, void *void_context)

nit: Other files I see would do this like:

void
Reporter(int nr, siginfo_t* info, void* void_context)

(so placement of return type on a line by itself and placement of * in pointer declaration)

@@ +44,5 @@
> +{
> +  ucontext_t *ctx = (ucontext_t *)(void_context);
> +  unsigned int syscall, arg1;
> +
> +  if (nr != SIGSYS)

nit: Shouldn't single line ifs still have braces?

@@ +55,5 @@
> +  syscall = SECCOMP_SYSCALL(ctx);
> +  arg1 = SECCOMP_PARM1(ctx);
> +
> +#ifdef PR_LOGGING
> +  PR_LOG(gSeccompSandboxLog, PR_LOG_DEBUG, ("PID %u is missing syscall %u, arg1 %u\n", getpid(), syscall, arg1));

It seems that the #ifdef around PR_LOG calls aren't required. https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prlog.h#182 PR_LOG is defined as a do-nothing macro when PR_LOGGING isn't defined.

@@ +69,5 @@
> + * handler can choose make the process exit. This is also used to report
> + * to the user which system call has been denied.
> + * This function should not be used in production and thus generally be
> + * called from debug code. In production, the process is directly killed.
> + * 

nit: trailing space

@@ +99,5 @@
> + * This function installs the syscall filter, a.k.a. seccomp.
> + * PR_SET_NO_NEW_PRIVS ensures that it is impossible to grant more
> + * syscalls to the process beyond this point (even after fork()).
> + * SECCOMP_MODE_FILTER is the "bpf" mode of seccomp which allows
> + * to pass a bpf program (in our case, it contains a syscall 

nit: trailing whitespace

@@ +110,5 @@
> +{
> +  if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0))
> +    return 1;
> +
> +  if (prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &seccomp_prog))

This call to prctl isn't passing in the extra args (missing 0, 0), so I'm betting you're getting a warning at compile time.

@@ +129,5 @@
> +#ifdef PR_LOGGING
> +if (!gSeccompSandboxLog)
> +  gSeccompSandboxLog = PR_NewLogModule("SeccompSandbox");
> +PR_ASSERT(gSeccompSandboxLog);
> +#endif

The contents of this #ifdef PR_LOGGING is at the wrong indentation level.

::: security/sandbox/android_seccomp_filter.h
@@ +37,5 @@
> + * It could be further optimized by analyzing the output of:
> + * 'strace -c -p <child pid>' for most used web apps.
> + */
> +#define SECCOMP_WHITELIST \
> +  SECCOMP_WHITELIST_ADD, \

Does it make sense for these to be at the beginning of the list? Seems like they should go at the end instead.

::: security/sandbox/seccomp_filter.h
@@ +38,5 @@
> + * Current list could also be commented. See android_seccomp_filter.h
> + * for examples.
> + */
> +
> +#define SECCOMP_WHITELIST \

Does it make sense to factor this into common, desktop, and android?

It seems to me that most of this list should be the same as the android one.
Comment 55 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-07-07 23:21:54 PDT
Comment on attachment 768663 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 768663 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the long delay.

My main concern is that we seem to be leaving some performance on the table by avoiding the use of the BPF compiler, and also because we don't have a great way of sort the macro-based list based on performance measurements. However, we can fix this in a follow-up bug after we've agreed on a plan.

I think the thing that needs to change most with this patch is the factoring out of the desktop-specific stuff into a separate patch.

I want to review the signal handling stuff a little more carefully. Also, I made a note of some follow-up items (where do we go from here) that we should discuss soon.

For the code imported from Chromium, I have a suggestion:

0. Keep the file names the same as the Chromium names, so it is easier to keep track of how they diverge.
1. Create a patch that imports the Chromium files, unmodified.
2. Create a patch that removes as much of the unused stuff as possible. For example, we don't use a lot of the macros like SECCOMP_PARM2 so why do we have dozens of lines of code to define them? For somebody new to seccomp filter, those are very distracting.
3. Create a patch, to be applied on top of the first two, that contains our modifications. Then we can review these modifications.

I pointed out some style nits below. In general, I only pointed out the style nit once, even though the issues need to be fixed throughout the patch.

::: build/dumbmake-dependencies
@@ +23,3 @@
>    accessible/build
>      accessible
>    dom

I think that this file should be changed to have this hierarchy:

toolkit/library
  dom
    ipc
    security/sandbox

because ContentChild.cpp is in DOM and it calls functions in both ipc/ and now security/sandbox.

::: configure.in
@@ +5218,5 @@
> +dnl = Content process sandboxing
> +dnl ========================================================
> +if test -n "$gonkdir"; then
> +    MOZ_CONTENT_SANDBOX=1
> +fi

Do we really only want to do this on Gonk? Can we do it on B2G Desktop too? We should create a follow-up bug for enabling mandatory sandboxing on B2G desktop by default, if it is practical to do so.

::: dom/ipc/ContentChild.cpp
@@ +546,5 @@
> +#ifdef MOZ_CONTENT_SANDBOX
> +  /* SetCurrentProcessSandbox should be moved close to process initialization
> +   * time if/when possible. SetCurrentProcessPrivileges should probably be
> +   * moved as well. Right now this is set ONLY if we receive the
> +   * RecvSetProcessPrivileges message. See also bug 880808 */

Nit: s/See also/See/. Also end the sentence with a period.

::: security/sandbox/Makefile.in
@@ +15,5 @@
> +EXPORT_LIBRARY = 1
> +FAIL_ON_WARNINGS = 1
> +
> +ifdef MOZ_DEBUG
> +DEFINES += -DMOZ_DEBUG=1

Is this necessary? content/base/src/nsContentSink.cpp contains an #ifdef MOZ_DEBUG but it doesn't seem to have this logic in its makefile. Also, overwelmingly "#ifdef DEBUG" is used throughout Gecko, whereas nsContentSink.cpp is the only place using "#ifdef MOZ_DEBUG".

@@ +19,5 @@
> +DEFINES += -DMOZ_DEBUG=1
> +endif
> +
> +LOCAL_INCLUDES += \
> +  $(NULL)

This can be removed since it is a no-op.

::: security/sandbox/Sandbox.cpp
@@ +21,5 @@
> +
> +namespace mozilla {
> +#ifdef PR_LOGGING
> +static PRLogModuleInfo* gSeccompSandboxLog;
> +#endif

Question: Is NSPR logging actually going to work in a sandboxed content process? It seems like the various logging outputs are things we'd want to protect from the sandbox. We should discuss how we're going to do that.

@@ +35,5 @@
> +#endif
> +};
> +
> +struct sock_fprog seccomp_prog = {
> +  len: (unsigned short)(sizeof(seccomp_filter)/sizeof(seccomp_filter[0])),

#include "mozilla/Util.h" and then use MOZ_ARRAY_LENGTH(seccomp_filter).

@@ +39,5 @@
> +  len: (unsigned short)(sizeof(seccomp_filter)/sizeof(seccomp_filter[0])),
> +  filter: seccomp_filter,
> +};
> +
> +void Reporter(int nr, siginfo_t *info, void *void_context)

I agree with Dave.

Also, Reporter should be static.

@@ +41,5 @@
> +};
> +
> +void Reporter(int nr, siginfo_t *info, void *void_context)
> +{
> +  ucontext_t *ctx = (ucontext_t *)(void_context);

Nit: Use C++ casting: ucontext_t* ctx = static_cast<ucontext_t*>(void_context).

@@ +44,5 @@
> +{
> +  ucontext_t *ctx = (ucontext_t *)(void_context);
> +  unsigned int syscall, arg1;
> +
> +  if (nr != SIGSYS)

Yes, braces please.

@@ +57,5 @@
> +
> +#ifdef PR_LOGGING
> +  PR_LOG(gSeccompSandboxLog, PR_LOG_DEBUG, ("PID %u is missing syscall %u, arg1 %u\n", getpid(), syscall, arg1));
> +#endif
> +  _exit(127);

Add a blank line before the call to _exit();

@@ +68,5 @@
> + * that has not been allowed. The call is also denied, and the SIGSYS
> + * handler can choose make the process exit. This is also used to report
> + * to the user which system call has been denied.
> + * This function should not be used in production and thus generally be
> + * called from debug code. In production, the process is directly killed.

I recommend moving this comment to be the comment above Reporter().

@@ +73,5 @@
> + * 
> + * @return 0 on sucess, -1 on failure
> + * @see Reporter() function
> + */
> +int InstallSyscallReporter(void)

Should be static.

@@ +88,5 @@
> +    return -1;
> +  }
> +  if (sigemptyset(&mask) ||
> +        sigaddset(&mask, SIGSYS) ||
> +        sigprocmask(SIG_UNBLOCK, &mask, NULL)) {

Nit: indent the two wrapped lines four spaces instead of six.

TO ME: review this function more carefully

@@ +105,5 @@
> + *
> + * @return 0 on success, 1 on failure
> + * @see sock_fprog (the seccomp_prog)
> + */
> +int InstallSyscallFilter(void)

should be static.

@@ +124,5 @@
> + * Should normally make the process exit on failure.
> +*/
> +void SetCurrentProcessSandbox(void)
> +{
> +#ifdef MOZ_CONTENT_SANDBOX

This #ifdef is unnecessary because this code won't even be compiled if MOZ_CONTENT_SANDBOX is disabled.

@@ +131,5 @@
> +  gSeccompSandboxLog = PR_NewLogModule("SeccompSandbox");
> +PR_ASSERT(gSeccompSandboxLog);
> +#endif
> +
> +#ifdef MOZ_DEBUG

#ifdef DEBUG?

@@ +153,5 @@
> +     * This will be re-enabled when all B2G devices are required to support seccomp-bpf
> +     * See bug 880797 for reversal
> +     */
> +    /* _exit(127); */
> +  }

Blank line here.

@@ +155,5 @@
> +     */
> +    /* _exit(127); */
> +  }
> +#endif
> +}

Blank line here.

::: security/sandbox/Sandbox.h
@@ +7,5 @@
> +#ifndef mozilla_Sandbox_h
> +#define mozilla_Sandbox_h
> +
> +namespace mozilla {
> +  void SetCurrentProcessSandbox(void);

namespace mozilla {

void SetCurrentProcessSandbox(void);

} // namespace mozilla

(See the coding style guide.)

::: security/sandbox/android_arm_ucontext.h
@@ +2,5 @@
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +/* This file has been imported from
> + * git.chromium.org/chromium.git/sandbox/linux/services/android_arm_ucontext.h

Unmodified? please include the git revision number.

::: security/sandbox/android_i386_ucontext.h
@@ +2,5 @@
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +/* This file has been imported from
> + * git.chromium.org/chromium.git/sandbox/linux/services/android_i386_ucontext.h

Unmodified?

Please include the git revision number.

::: security/sandbox/android_seccomp_filter.h
@@ +5,5 @@
> +
> +#include "linux_seccomp.h"
> +
> +/* This is the actual seccomp whitelist.
> + * This is used for FirefoxOS (B2G) content-processes.

remove the FirefoxOS reference and just refer to it as B2G.

@@ +9,5 @@
> + * This is used for FirefoxOS (B2G) content-processes.
> + */
> +
> +/* ARM Specific calls */
> +#if defined(__arm__) && (defined(__thumb__) || defined(__ARM_EABI__))

For readability, this should be #ifdef ALLOW_ARM_SYSCALL, because that is only defined when this complicated condition is set.

@@ +15,5 @@
> +  ALLOW_ARM_SYSCALL(breakpoint), \
> +  ALLOW_ARM_SYSCALL(cacheflush), \
> +  ALLOW_ARM_SYSCALL(usr26), \
> +  ALLOW_ARM_SYSCALL(usr32), \
> +  ALLOW_ARM_SYSCALL(set_tls)

Are all of these calls with the same approximate frequency?

IMO, we might as well just change inline each of the ALLOW_ARM_SYSCALL(x) calls into the list below, like this:

#define SECCOMP_WHITELIST \
   ...
#ifdef ALLOW_ARM_SYSCALL
   ALLOW_ARM_SYSCALL(breakpoint),
#endif
   ...
#ifdef ALLOW_ARM_SYSCALL
   ALLOW_ARM_SYSCALL(usr32),
#endif
   ...

This way, we don't feel any pressure to sort the list in a way that is sub-optimal for performance.

@@ +50,5 @@
> +  ALLOW_SYSCALL(close), \
> +  ALLOW_SYSCALL(clone), \
> +  ALLOW_SYSCALL(clock_gettime), \
> +  ALLOW_SYSCALL(exit_group), \
> +  ALLOW_SYSCALL(exit), \

exit and exit_group seem to be much higher on the list than I would guess they deserve to be.

@@ +63,5 @@
> +  ALLOW_SYSCALL(sigreturn), \
> +  /* ioctl() is for GL. Remove when GL proxy is implemented.
> +   * Additionally ioctl() might be a place where we want to have
> +   * argument filtering */ \
> +  ALLOW_SYSCALL(ioctl), \

I am surprised that ioctl is so low on the list. I would expect it to be near brk, in order to accomodate WebGL-based things.

@@ +71,5 @@
> +  ALLOW_SYSCALL(munmap), \
> +  ALLOW_SYSCALL(mmap2), \
> +  ALLOW_SYSCALL(mprotect), \
> +  /* Must remove in the future, when no longer used */ \
> +  /* open() is for some legacy APIs such as font loading. */ \

I suggest that, except for ioctl, we move all the "bad, needs to be removed" syscalls to the end of the list. If we know we need to remove the uses of those syscalls anyway, then regressing performance on them more than others will just increase the prioritization of us doing the right thing, while minimizing the negative performance impact of the filter for the cases where we are doing things we consider to be OK.

Besides these suggestions of reorderings, I think we can punt the optimization of the performance of this feature to a follow-up bug.

@@ +78,5 @@
> +  ALLOW_SYSCALL(stat64), \
> +  ALLOW_SYSCALL(prctl), \
> +  ALLOW_SYSCALL(access), \
> +  ALLOW_SYSCALL(getdents64), \
> +  /* Should be removed in the future, if possible */ \

I guess this comment means "All of the following must be removed in the future, if possible." As I said above, I would move "open" to be down here. Also, we need to file follow-up bugs for the removal of items from this list.

::: security/sandbox/android_ucontext.h
@@ +2,5 @@
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +/* This file has been imported from
> + * git.chromium.org/chromium.git/sandbox/linux/services/android_ucontext.h

Unmodified? please include the git revision number.

::: security/sandbox/linux_seccomp.h
@@ +2,5 @@
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.
> +
> +/* This file has been initially imported from
> + * git.chromium.org/chromium.git/sandbox/linux/seccomp-bpf/sandbox_bpf.h

Unmodified? Please include the git revision number.

Also, wouldn't it be clearer to keep the name the same as it is in the Chromium repo, to build on shared knowledge? I think the Chromium name is clearer anyway.

::: security/sandbox/seccomp_filter.h
@@ +38,5 @@
> + * Current list could also be commented. See android_seccomp_filter.h
> + * for examples.
> + */
> +
> +#define SECCOMP_WHITELIST \

In the interest of expediency, I suggest that we factor out the desktop-specific stuff into a separate patch, after we've all agreed on the best way of dealing with the differences between B2G Gonk, B2G Desktop, Android, and Desktop Linux. 

I agree with Dave. In particular, I think it is likely that it would be best to maintain a single (sorted) list. But, I am not sure how long we're even going to be using this macro-based approach considering that the compiler used by the ChromiumOS guys (or a similar we create) seems likely to be notably better for performance and will likely be the long-term solution.

The main motivation for having separate lists would be if the ordering needed to be different. However, I don't think the ordering of this desktop list is based on any measurements or guesses of the performance impact considering that it is in alphabetical order.

kang annotated the list for B2G to indicate which syscalls need to be removed. AFAICT, most of the extra syscalls added for Firefox Desktop also belong in the "need to be removed" category and so they need to be similarly annotated.

Also, in general the concept of B2G is that it is supposed to work like Gecko on Android and like Gecko on Desktop. That means we should be minimizing the differences between things as much as possible. That means that every difference between the B2G, Android, and desktop versions of the list is a cause for concern that should be clearly documented. I think this also points towards having a single list with platform conditionals and clear annotations of the rationale for the platform differences.
Comment 56 Andreas Gal :gal 2013-07-08 06:33:17 PDT
We should definitely sandbox the parent/main process as well in a 2nd iteration of this once we have sandboxed the child process and punt all critical resource access into a new parent-of-main process that will be the sole privileged process and that doesn't do anything but manage those resources and launch the actual parent process. Chris Jones filed a bug about this a year ago or so. I will try to find it.
Comment 57 Guillaume Destuynder [:kang] 2013-07-08 10:27:59 PDT
@comment 56
this is the first step to move the parent/main process in it's own sandbox: bug 845191
I track all of the system sec bugs in bug 862082
Comment 58 Guillaume Destuynder [:kang] 2013-07-10 18:22:42 PDT
Quick status update:

I addressed all of dhylands's comments and most of briansmith comments locally.
Will chat with Brian whenever possible for the unfixed comments so that I can upload a new patch for review.

I've also addressed some comments from our IRC discussion (w/ dhylands & briansmith):
- unified the whitelist (no desktop whitelist file)
- s/MOZ_DEBUG/MOZ_CONTENT_SANDBOX_REPORTER/ (with a configure flag to enable)
Comment 59 Guillaume Destuynder [:kang] 2013-07-23 16:49:47 PDT
Created attachment 780083 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Carrying r+: imelven, dhylands, khuey

Changes:

::: build/dumbmake-dependencies
Done :)

::: configure.in
>> @@ +5218,5 @@
>> +dnl = Content process sandboxing
>> +dnl ========================================================
>> +if test -n "$gonkdir"; then
>> +    MOZ_CONTENT_SANDBOX=1
>> +fi

>Do we really only want to do this on Gonk? Can we do it on B2G Desktop too? We should create a follow-up bug for enabling mandatory sandboxing on B2G desktop by default, if it is practical to do so.

Not fixed. Follow up bug is needed, or another run of patches after discussion. Need a good flag that works for everyone. $target match *-*linux* ?

::: dom/ipc/ContentChild.cpp
> Nit: s/See also/See/. Also end the sentence with a period.

Also changed comments to "//". Coding style is not very precise (unless I'm blind :) regarding those. I guess both might be allowed. I'd rather fix the whole file in a different patch than having conflicting style for something like this if not ok.

::: security/sandbox/Makefile.in
Done. And in fact, disabled need for DEBUG everywhere. Rationale: building debug is long, and it was mainly there for the reporting mode. Instead, I added a --enable-content-sandbox-reporter (off by default). Faster, easier, more flexible, IMO.
The logging is also now always enabled (and only used with reporter on)

::: security/sandbox/Sandbox.cpp
> Question: Is NSPR logging actually going to work in a sandboxed content process? It seems like the various logging outputs are things we'd want to protect from the sandbox. We should discuss how we're going to do that.

Right now, yes. As long as initialized before the sandbox, since the fd is opened, it should in fact always work (write() always whitelisted). If it ever breaks or logging behavior changes, fprintf(std{err,out}..) and ptrace() may be used.
To use: export NSPR_LOG_MODULES=SeccompSandbox:2 (in /system/bin/b2g.sh or manually)
On B2G, this logs to logcat by default (adb logcat -v thread #for example)

> #include "mozilla/Util.h" and then use MOZ_ARRAY_LENGTH(seccomp_filter).
Done.

> static functions
Done.

> Nit: Use C++ casting
Done.

> Yes, braces please.
Done.

> Add a blank lines (each time near _exit())
Not sure if those are for making _exit() clear or blanks after #idefs and braces. Took the second option, so "done".

> I recommend moving this comment to be the comment above Reporter().
Done.

> Wrapping issues
Done.

>> @@ +124,5 @@
>> + * Should normally make the process exit on failure.
>> +*/
>> +void SetCurrentProcessSandbox(void)
>> +{
>> +#ifdef MOZ_CONTENT_SANDBOX
>
> This #ifdef is unnecessary because this code won't even be compiled if MOZ_CONTENT_SANDBOX is disabled.
Done.

::: security/sandbox/Sandbox.h
Done.

::: security/sandbox/android_arm_ucontext.h
::: security/sandbox/android_i386_ucontext.h
::: security/sandbox/android_ucontext.h
::: security/sandbox/linux_seccomp.h
git.chromium.org/chromium.git/sandbox/linux/seccomp-bpf/sandbox_bpf.h
Patches for understanding this will follow in the next comment (as requested).

::: security/sandbox/android_seccomp_filter.h
Removed file. Using single whitelist in security/sandbox/seccomp_filter.h instead. "Done".

::: security/sandbox/seccomp_filter.h
> For readability, this should be #ifdef ALLOW_ARM_SYSCALL, because that is only defined when this complicated condition is set.
Done.

> Move ARM calls at the bottom (not used often)
Done. Note: can't ifdef in the define, hence the "SECCOMP_WHITELIST_ADD" def.

> exit and exit_group seem to be much higher on the list than I would guess they deserve to be.
Anything below the first few get rarely called. Initially prefered to keep all the calls in "we allow" together. Moved exit at the end of that list, although perf difference is hoepfully unsignficant (or impossible to measure), but it is indeed more logical.

> I am surprised that ioctl is so low on the list. I would expect it to be near brk, in order to accomodate WebGL-based things.
I would expect the GL proxy to have a much, much higher than this list ordering.
For now, made an exception for the ioctl comment and moved it up.
Could also comment the list with one comment per line, and have an optimal .. optimized list instead. Not sure if perf difference will always be measurable. I'm talking to the performance team about a good way to measure this accross various apps.

> I guess this comment means "All of the following must be removed in the future, if possible."
Modified for clarity.
Comment 60 Guillaume Destuynder [:kang] 2013-07-23 17:28:40 PDT
Created attachment 780097 [details] [diff] [review]
android_ucontext.diff
Comment 61 Guillaume Destuynder [:kang] 2013-07-23 17:29:27 PDT
Created attachment 780098 [details] [diff] [review]
android_i386_ucontext.diff
Comment 62 Guillaume Destuynder [:kang] 2013-07-23 17:31:06 PDT
Created attachment 780099 [details] [diff] [review]
android_arm_ucontext.diff
Comment 63 Guillaume Destuynder [:kang] 2013-07-23 17:33:14 PDT
Created attachment 780100 [details] [diff] [review]
linux_seccomp.diff

Patches 780097 780098 78009 have minimal changes with upstream.
This patch has more changes.
Comment 64 Guillaume Destuynder [:kang] 2013-07-23 19:03:32 PDT
Created attachment 780130 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Revised the patch to include kernel headers.
Rationale:
- our bionic is currently unmodified, adding the bionic patch would mean having to fork it. I'm not sure I'll get support for that.
- If built on desktop, this provides the kernel headers as well if they're missing.

Those headers only include the system calls for 32/64bit x86 and arm.

The files are, like android*_ucontext.h files unmodified except for the header, will revision and location of the original. Let me know if you want diffs of them nevertheless.. and of course if you agree with having those headers built-in.
Comment 65 David Keeler [:keeler] (use needinfo?) 2013-08-01 11:32:38 PDT
Comment on attachment 780100 [details] [diff] [review]
linux_seccomp.diff

Review of attachment 780100 [details] [diff] [review]:
-----------------------------------------------------------------

I see no problems with these changes.

::: linux_seccomp.h.ori
@@ +9,4 @@
>  #ifndef SANDBOX_LINUX_SECCOMP_BPF_LINUX_SECCOMP_H__
>  #define SANDBOX_LINUX_SECCOMP_BPF_LINUX_SECCOMP_H__
>  
> +struct arch_seccomp_data {

Are we copying these definitions from somewhere? If so, it would be nice to have a comment to that effect.

@@ +212,4 @@
>  
>  #endif
>  
> +/* Macros to common filters */

Again, did you write these, or were they from somewhere else originally?

@@ +253,2 @@
>  #endif  // SANDBOX_LINUX_SECCOMP_BPF_LINUX_SECCOMP_H__
> +

Is there a style guideline about having an empty line at the end of headers? If not, I would leave this how it was.
Comment 66 David Keeler [:keeler] (use needinfo?) 2013-08-01 11:52:44 PDT
Comment on attachment 780130 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Review of attachment 780130 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

::: security/sandbox/Sandbox.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set sw=2 ts=8 et ft=cpp : */

For mode lines, I think this is what is agreed on nowadays:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */

(the second line is different)
Personally, I'm against mode lines, but if we've agreed on them as a project, I guess we should adhere to that.

@@ +82,5 @@
> + * We register an action for that signal (calling the Reporter function).
> + *
> + * This function should not be used in production and thus generally be
> + * called from debug code. In production, the process is directly killed.
> + * For this reason, the function is ifdef'd, as there is no reason to

Why don't we ifdef Reporter as well?

::: security/sandbox/android_arm_ucontext.h
@@ +1,3 @@
> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style license that can be
> +// found in the LICENSE file.

Is the appropriate LICENSE file included (or otherwise already available)?

@@ +14,5 @@
> +
> +// We also need greg_t for the sandbox, include it in this header as well.
> +typedef unsigned long greg_t;
> +
> +//typedef unsigned long sigset_t;

I suppose this was commented out upstream?
Comment 67 Guillaume Destuynder [:kang] 2013-08-01 12:16:49 PDT
@comment 65

::: linux_seccomp.h.ori
@@ +9,4 @@
>>  #ifndef SANDBOX_LINUX_SECCOMP_BPF_LINUX_SECCOMP_H__
>>  #define SANDBOX_LINUX_SECCOMP_BPF_LINUX_SECCOMP_H__
>>  
>> +struct arch_seccomp_data {

This is similar to:
http://src.chromium.org/svn/trunk/src/sandbox/linux/seccomp-bpf/sandbox_bpf.h
I'll put a comment. Althought this can also be reconstructed from reading the kernel code basically, its used to figure out the seccomp data (length for offsets primarily) returned by sigsys.

@@ +212,4 @@
>>  
>>  #endif
>>  
>> +/* Macros to common filters */

Again, did you write these, or were they from somewhere else originally?

I wrote some of them (it's using the BPF macros, see /usr/include/linux/filter.h). The others are from or similar to: http://outflux.net/teach-seccomp/step-5/seccomp-bpf.h
(he also uses a seccomp data struct for similar purposes with, of course, the same data in the struct). It already has their copyright, though, as they're the same guys making chromium.

Maybe there should be a link to this instead (we might use some of that in the future as well, although I wanna look at the current filter compiler and verifier they're using in chromium too)

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/prctl/seccomp_filter.txt and https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/samples/seccomp

@@ +253,2 @@
>>  #endif  // SANDBOX_LINUX_SECCOMP_BPF_LINUX_SECCOMP_H__
>> +

I'm not clear about that (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style). I'd rather just remove them for close-to-original files, indeed.


@comment 66

::: security/sandbox/Sandbox.cpp
@@ +1,2 @@
>> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>> +/* vim: set sw=2 ts=8 et ft=cpp : */
Hmm not sure how this happened to be different. Will do.

@@ +82,5 @@
>> + * We register an action for that signal (calling the Reporter function).
>> + *
>> + * This function should not be used in production and thus generally be
>> + * called from debug code. In production, the process is directly killed.
>> + * For this reason, the function is ifdef'd, as there is no reason to

> Why don't we ifdef Reporter as well?
Its not directly callable/can't really be misused, so I haven't done it. However I agree that it's logical and not actually in use. Will do.


::: security/sandbox/android_arm_ucontext.h
@@ +1,3 @@
>> +// Copyright (c) 2012 The Chromium Authors. All rights reserved.
>> +// Use of this source code is governed by a BSD-style license that can be
>> +// found in the LICENSE file.

>Is the appropriate LICENSE file included (or otherwise already available)?
its indeed there, but, its also indeed not clear. it used to be clear when it was in the chromium ipc directory. I'll include it in the same directory unless someone instructs otherwise.

@@ +14,5 @@
>> +
>> +// We also need greg_t for the sandbox, include it in this header as well.
>> +typedef unsigned long greg_t;
>> +
>> +//typedef unsigned long sigset_t;

>I suppose this was commented out upstream?
Yes, see attachment 780099 [details] [diff] [review] which is diff of our version vs upstream


Will correct as stated in this comment ASAP (im currently at blackhat), thanks!
Comment 68 Guillaume Destuynder [:kang] 2013-08-05 16:46:59 PDT
I've corrected the aboev except for the license file, i've emailed gerv for guidance. I suspect I'll drop the LICENSE file in /security/sandbox/. 
Once this is done I plan to post the patches here again and proceed with landing them, unless there are objections.
Comment 69 Guillaume Destuynder [:kang] 2013-08-06 15:40:05 PDT
Created attachment 786563 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Carrying over r+ from dkeeler,imelven,khuey

Added licensing fixes (the license is also clearly listing the directory in the about:license as per gerv, and a LICENSE file has been added to /security/sandbox)

Fixed things as described in comment 67
Comment 70 Guillaume Destuynder [:kang] 2013-08-06 15:42:12 PDT
this also carry over r+ from dhylands on patch 786563 from comment 69
Comment 71 Guillaume Destuynder [:kang] 2013-08-12 13:45:24 PDT
last try run: https://tbpl.mozilla.org/?tree=Try&rev=cde1cef1a3dc
Comment 72 :Ms2ger 2013-08-13 04:31:18 PDT
Backed out for missing build peer and legal review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca193619b815
Comment 73 Carsten Book [:Tomcat] 2013-08-13 05:06:38 PDT
https://hg.mozilla.org/mozilla-central/rev/9a57f0f347e3
Comment 74 Guillaume Destuynder [:kang] 2013-08-13 10:08:06 PDT
Comment on attachment 786563 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

Gerv - you answered some licenses questions by email - as per comment 72, I might need legal review or your review (I haven't found this documented so I'm not sure).

:Msg2ger mentionned on IRC that having a comment from you in the bug would be sufficient. I'm marking it as review? - if you can't review it, can you point me in the right direction? I didn't find much on the wiki.

Thanks!
Comment 75 [:mmc] Monica Chew (no longer reading bugmail) 2013-08-13 10:55:59 PDT
(In reply to :Ms2ger from comment #72)
> Backed out for missing build peer and legal review.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ca193619b815

khuey gave an r+ in comment 46 for build changes.
Comment 76 Guillaume Destuynder [:kang] 2013-08-13 11:18:30 PDT
Yep, it seems I missed his r= in the commit - my fault :(

@gerv
I looked for a way to make the licensing review easier - however there's the header in most of the files, so i'm not sure if splitting helps (I've a splitted diff with only the LICENSE file itself and the license.html change otherwise).
Comment 77 Gervase Markham [:gerv] 2013-08-14 17:32:19 PDT
Comment on attachment 786563 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

r=gerv.

Gerv
Comment 78 Guillaume Destuynder [:kang] 2013-08-15 15:40:12 PDT
For simplicity, review summary:

r+ gerv comment 77 (licenses)
r+ keeler comment 66 (replaced briansmith, core functionality)
r+ agal after comment 64 (upstream files)
r+ dhylands comment 54 (core functionality)
r+ khuey comment 47 (build peer)
r+ imelven comment 45 comment 39 comment 37 (core functionality)

Newest push to try, just in case:
https://tbpl.mozilla.org/?tree=Try&rev=a0d659c963f6
Comment 79 Carsten Book [:Tomcat] 2013-08-16 02:04:44 PDT
https://hg.mozilla.org/mozilla-central/rev/ded622a6ad19
Comment 80 Guillaume Destuynder [:kang] 2013-09-19 04:42:21 PDT
Created attachment 807154 [details]
Seccomp-bpf cherry-pickable commits for kernel 3.4, from http://git.chromium.org/chromiumos/third_party/kernel.git origin/chromeos-3.4
Comment 81 Jan Varga [:janv] 2013-10-09 07:52:46 PDT
Ok, so once the sandbox is enabled, will it be possible to do I/O in content processes ?
Like writing to a file ?
We need to know since we want to add multiprocess support for some file based APIs and even IndexedDB will need to write to files in content processes at some point.
Comment 82 David Keeler [:keeler] (use needinfo?) 2013-10-09 09:27:34 PDT
(In reply to Jan Varga [:janv] from comment #81)
> Ok, so once the sandbox is enabled, will it be possible to do I/O in content
> processes ?
> Like writing to a file ?
> We need to know since we want to add multiprocess support for some file
> based APIs and even IndexedDB will need to write to files in content
> processes at some point.

My understanding is that if a sandboxed process needs to read/write a file, it will ask its supervisor process to open the file and pass it a file descriptor. It can then read/write/close as necessary.
Comment 83 Guillaume Destuynder [:kang] 2013-10-10 14:43:51 PDT
(In reply to David Keeler (:keeler) from comment #82)
> (In reply to Jan Varga [:janv] from comment #81)
> > Ok, so once the sandbox is enabled, will it be possible to do I/O in content
> > processes ?
> > Like writing to a file ?
> > We need to know since we want to add multiprocess support for some file
> > based APIs and even IndexedDB will need to write to files in content
> > processes at some point.
> 
> My understanding is that if a sandboxed process needs to read/write a file,
> it will ask its supervisor process to open the file and pass it a file
> descriptor. It can then read/write/close as necessary.
correct. the supervisor in this case is the "parent" ie b2g. I think :jonas looked at IndexedDB and having all the code logic in the content process is a good idea (its tracked in bug 898694). The parent just does checking when resource access (such as getting a file open) is needed. write() and read() are allowed from the content process.
Comment 84 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-10-10 15:21:18 PDT
Guillaume, how about things like dup and mmap? Are those available on already-opened fds in the child?
Comment 85 Dave Hylands [:dhylands] 2013-10-10 16:00:37 PDT
I don't see any reason why these shouldn't work.
Comment 86 Guillaume Destuynder [:kang] 2013-10-10 16:07:48 PDT
yep - basically see http://hg.mozilla.org/mozilla-central/file/40c7c53fe9b0/security/sandbox/seccomp_filter.h#l118  (or whatever the latest version is if you read this in the distant future) to get an idea of what is allowed, what's not allowed, and finally, what's currently allowed but will be removed in the future.
Comment 87 Guillaume Destuynder [:kang] 2013-12-16 03:04:45 PST
Created attachment 8347964 [details] [diff] [review]
generic_seccomp-bpf_3.0.x (same as nice/keon/nexus patch without defconfig changes)

For simplicity/convenience, more generic patch for seccomp-bpf on kernel 3.0.x.
Just add:
CONFIG_SECCOMP=y
CONFIG_SECCOMP_FILTER=y
CONFIG_SECCOMP_DEBUG=y

To the corresponding defconfig (arch/arm/configs/<YOUR_DEVICE_CODE>_defconfig) or your local .config to enable seccomp-bpf
Comment 89 Guillaume Destuynder [:kang] 2014-02-05 18:46:16 PST
cleaned up some dependent bugs which are already childrens of the deps of the seccomp bug (yea.)
Comment 90 Guillaume Destuynder [:kang] 2014-02-07 11:28:43 PST
note regarding attachement 807154

the current URL to cherry pick from is https://chromium.googlesource.com/chromiumos/third_party/kernel origin/chromeos-3.4
Comment 91 Jed Davis [:jld] [⏰PDT; UTC-7] 2014-05-13 10:50:28 PDT
Created attachment 8421852 [details]
Alternate list of kernel commits to cherry-pick for 3.4

I've needed to make two slight adjustments to attachment 807154 [details] to make it work with 3.4-based Android kernels I've encountered:

* Drop 85e7bac33b8d (seccomp: audit abnormal end to a process due to seccomp) because it's already present.

* Add 43520a3be4bf (CHROMIUM: arch/arm: add asm/syscall.h) before the patch that changes that file.
Comment 92 Amod [:AMoz] 2015-09-11 03:07:15 PDT
hi, I am interested in working on this project. I had a word with Stéphanie Ouillon who directed me to few bugs including this one.
I have gone through the security architecture of FxOS.
I also thoroughly read the research papers published by FxOS team in IEEE xplore.

I found this project as a very good one proving to be a great learning experience for me. 
Since I am a rep, I can request a device for testing purpose.
I am planning this project along with bug 845191

@Jed, Since the bug is assigned to you, can I help you with either full or part of the work?

What else do I need to get started with this project ?
Comment 93 Jed Davis [:jld] [⏰PDT; UTC-7] 2015-11-06 19:15:34 PST
Sorry; I meant to answer this needinfo much sooner.  I'm not working on sandboxing much now, and B2G is in much better shape than desktop Linux sandboxing, but there are still things to do.   This was originally the bug for the initial seccomp-bpf support on B2G, but it turned into a tracking bug for ongoing work, which is mostly about reducing the whitelist.  (B2G requires seccomp-bpf support if the base system is KitKat or newer, and all the emulators also have it.)

The biggest thing there was bug 930258, removing open() and other filesystem operations, and instead using a broker that can restrict the filesystem path.  That has a few follow-up items:

* Bug 1214947 covers enabling brokering on non-emulator devices, which is relatively easy but needs access to a variety of hardware; I'd been thinking of that as something for MoCo contributors with larger collections of devices, like :pauljt's team.

* Moving the broker out of the parent process, which I haven't filed a bug for yet; that requires serializing/deserializing the policy, building the broker as its own executable, and trying to keep that executable's memory usage relatively small (similarly to bug 845191).

* Whitelist reduction.  There are a lot of bugs filed under bug 1121295, but some of them aren't likely to be fixed, and the security impact varies.

And there are other ways the seccomp-bpf policy can be reduced.  One thing in particular that could be useful, and which I think there isn't a bug for yet, is restricting the "request" argument to ioctl(); this is another exercise in constructing a whitelist that covers existing usage, and it will be partially hardware-dependent.  There may also be syscalls that currently need to be allowed but could be avoided without too much work; I haven't tried to go through the policy and look for that kind of thing yet.

(Re-needinfo'ing myself to go file the bugs I mentioned....)

Note You need to log in before you can comment on or make changes to this bug.