Closed Bug 790923 (b2g-seccomp) Opened 12 years ago Closed 7 years ago

B2G content process sandboxing via seccomp filter

Categories

(Core :: Security: Process Sandboxing, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: kang, Assigned: jld)

References

(Depends on 1 open bug, )

Details

(Keywords: meta, Whiteboard: [tech-p2][leave open][sb-])

Attachments

(10 files, 25 obsolete files)

8.56 KB, application/gzip
Details
1.46 KB, patch
imelven
: review+
Details | Diff | Splinter Review
971 bytes, patch
gal
: review+
Details | Diff | Splinter Review
663 bytes, patch
gal
: review+
Details | Diff | Splinter Review
658 bytes, patch
gal
: review+
Details | Diff | Splinter Review
3.46 KB, patch
keeler
: review+
Details | Diff | Splinter Review
109.53 KB, patch
gerv
: review+
Details | Diff | Splinter Review
2.72 KB, text/plain
Details
78.86 KB, patch
Details | Diff | Splinter Review
2.70 KB, text/plain
Details
Attached patch nexus s seccomp filter support (obsolete) — Splinter Review
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.
Attached patch user-space seccomp patch (obsolete) — Splinter Review
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)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Will arguably boost security better than some competitors, but our sandbox will be somewhat leaky until we have sandboxed webgl.
Whiteboard: [tech-p2]
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.
Group: mozilla-corporation-confidential, qualcomm-confidential
This fixes the handling of SIGSYS, allowing signal based reporters to work correctly.

Note: nexus/unagi patches need a similar fix
Attachment #736882 - Attachment is obsolete: true
Attached file seccomp test program
Not test units, just a small program to experiment easily on android / B2G
Nicer Keon patch, include samples, doc and audit support
Attachment #742096 - Attachment is obsolete: true
Attached patch Gecko seccomp support (obsolete) — Splinter Review
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.
Attachment #660797 - Attachment is obsolete: true
Attached patch Regenerated bionic's unistd.h (obsolete) — Splinter Review
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.
Attached patch Gecko seccomp support (obsolete) — Splinter Review
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.
Attachment #742630 - Attachment is obsolete: true
Attached patch Gecko seccomp support (obsolete) — Splinter Review
Attachment #743984 - Attachment is obsolete: true
Attachment #743987 - Flags: feedback?(imelven)
Attachment #743987 - Attachment is patch: true
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 ?
Attachment #743987 - Flags: feedback?(imelven) → feedback+
Attached patch Gecko seccomp support (obsolete) — Splinter Review
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
Attachment #743987 - Attachment is obsolete: true
Attachment #745349 - Flags: feedback?(imelven)
(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.
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
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 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 :)
Attachment #745349 - Flags: feedback?(imelven) → feedback+
(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 :)
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.
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.
Removed confidential flags as per :mwu - unagi patch is based off public code
Group: mozilla-corporation-confidential, qualcomm-confidential
Updated the unagi patch to the same level as the Keon patch (ie full seccomp bpf support)
Attachment #735403 - Attachment is obsolete: true
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.
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
(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.
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!
Attachment #745349 - Attachment is obsolete: true
Attachment #758439 - Flags: feedback?(bsmith)
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.
Attachment #758441 - Flags: feedback?(imelven)
Attachment #758441 - Flags: feedback?(bsmith)
(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.
Attachment #758439 - Attachment is obsolete: true
Attachment #758439 - Flags: feedback?(bsmith)
Attachment #761240 - Flags: review?(imelven)
Attachment #761240 - Flags: review?(bsmith)
Attachment #761241 - Attachment is obsolete: true
Attachment #761241 - Flags: review?(imelven)
Attachment #761241 - Flags: review?(bsmith)
Attachment #761242 - Flags: review?(imelven)
Attachment #761242 - Flags: review?(bsmith)
Sorry, wrong patch. Fixed.
Attachment #761240 - Attachment is obsolete: true
Attachment #761240 - Flags: review?(imelven)
Attachment #761240 - Flags: review?(bsmith)
Attachment #761244 - Flags: review?(imelven)
Attachment #761244 - Flags: review?(bsmith)
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.
Attachment #758441 - Attachment is obsolete: true
Attachment #758441 - Flags: feedback?(imelven)
Attachment #758441 - Flags: feedback?(bsmith)
Attachment #761245 - Flags: review?(imelven)
Attachment #761245 - Flags: review?(bsmith)
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.
Attachment #761245 - Flags: review?(imelven) → review+
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 ?
Attachment #761244 - Flags: review?(imelven)
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
Attachment #761242 - Flags: review?(imelven) → review+
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'
Attachment #758440 - Flags: review+
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.
Attachment #761241 - Attachment is obsolete: false
Attachment #758440 - Attachment is obsolete: true
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 :)
Attachment #761241 - Attachment is obsolete: true
Attachment #761242 - Attachment is obsolete: true
Attachment #761244 - Attachment is obsolete: true
Attachment #761242 - Flags: review?(bsmith)
Attachment #761244 - Flags: review?(bsmith)
Attachment #762193 - Flags: review?(imelven)
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 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.
Attachment #762193 - Flags: review?(imelven) → review+
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.
Attachment #762193 - Flags: review?(khuey)
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.
Attachment #762193 - Flags: review?(khuey) → review+
Attachment #762193 - Attachment is obsolete: true
Attachment #762193 - Flags: review?(dhylands)
Attachment #762193 - Flags: review?(bsmith)
Attachment #766928 - Flags: review?(dhylands)
Attachment #766928 - Flags: review?(bsmith)
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
Attachment #766928 - Flags: review+
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.
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 :)
Tested on try and so on, includes changes from previous comment.
Attachment #766928 - Attachment is obsolete: true
Attachment #766928 - Flags: review?(dhylands)
Attachment #766928 - Flags: review?(bsmith)
Attachment #768663 - Flags: review?(dhylands)
Attachment #768663 - Flags: review?(bsmith)
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.
Attachment #761245 - Flags: review?(dhylands)
Attachment #761245 - Flags: review?(bsmith)
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.
Attachment #768663 - Flags: review?(dhylands) → review+
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.
Attachment #768663 - Flags: review?(brian) → review-
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 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
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)
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.
Attachment #768663 - Attachment is obsolete: true
Attachment #780083 - Flags: review?(brian)
Patches 780097 780098 78009 have minimal changes with upstream.
This patch has more changes.
Attachment #780100 - Flags: review?(brian)
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.
Attachment #742635 - Attachment is obsolete: true
Attachment #780083 - Attachment is obsolete: true
Attachment #780130 - Flags: review?(brian)
Attachment #780097 - Flags: review?(brian) → review+
Attachment #780098 - Flags: review?(brian) → review+
Attachment #780099 - Flags: review?(brian) → review+
Assignee: nobody → gdestuynder
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.
Attachment #780100 - Flags: review?(brian) → review+
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?
Attachment #780130 - Flags: review?(brian) → review+
@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!
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.
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
Attachment #780130 - Attachment is obsolete: true
Attachment #786563 - Flags: review+
this also carry over r+ from dhylands on patch 786563 from comment 69
Whiteboard: [tech-p2] → [tech-p2][leave open]
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!
Attachment #786563 - Flags: review+ → review?(gerv)
(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.
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 on attachment 786563 [details] [diff] [review]
Initial-support-for-the-Seccomp-bpf-Sandbox

r=gerv.

Gerv
Attachment #786563 - Flags: review?(gerv) → review+
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
Depends on: 912791
Component: General → Emulator
Depends on: 920372
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.
(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.
Depends on: 925119
(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.
Guillaume, how about things like dup and mmap? Are those available on already-opened fds in the child?
I don't see any reason why these shouldn't work.
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.
Depends on: 921817
Depends on: 907006
Depends on: 930258
Depends on: 932098
Depends on: 932107
Depends on: 932104
Depends on: 936145
Depends on: 936252
Depends on: 936272
Depends on: 936320
Depends on: 941375
Depends on: 943774
Depends on: 945330
Depends on: 946407
Depends on: 948779
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
Attachment #660787 - Attachment is obsolete: true
Attachment #742631 - Attachment is obsolete: true
Attachment #746594 - Attachment is obsolete: true
Depends on: 956961
Depends on: 960365
Depends on: 962769
Depends on: 964427
Depends on: 964455
Depends on: 966547
Depends on: 967967
cleaned up some dependent bugs which are already childrens of the deps of the seccomp bug (yea.)
No longer depends on: 907006, 920372, 930258, 932098, 932104, 941375, 946407
Alias: b2g-seccomp → seccomp-sandbox
Depends on: 969088
note regarding attachement 807154

the current URL to cherry pick from is https://chromium.googlesource.com/chromiumos/third_party/kernel origin/chromeos-3.4
Depends on: 971370
Depends on: 977902
Depends on: 1004011
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.
Depends on: 1009995
Depends on: 1014299
Depends on: 1017393
Assignee: gdestuynder → nobody
QA Contact: gdestuynder
Depends on: 1047620
Depends on: 1055310
Depends on: 930258
Depends on: 964455
Depends on: 969715
Depends on: 1078838
Depends on: 1078971
Depends on: 1093893
Alias: seccomp-sandbox → b2g-seccomp
Assignee: nobody → jld
No longer blocks: desktop-seccomp
Component: Emulator → Security: Process Sandboxing
Keywords: meta
Product: Firefox OS → Core
Hardware: ARM → All
See Also: → desktop-seccomp
Summary: Content process sandboxing via seccomp filter → B2G content process sandboxing via seccomp filter
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 ?
Flags: needinfo?(jld)
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....)
Flags: needinfo?(jld)
Whiteboard: [tech-p2][leave open] → [tech-p2][leave open][sb-]
B2G is no more, so I don't think it makes sense to continue to [leave open] this bug.  But the thing in the summary did happen, so, RESOLVED FIXED.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.