We should use the rest of Chromium's code for constructing the sandbox program and handling traps, instead of continuing to reinvent these wheels ourselves. This will require breaking out our code for using signals as a "broadcast interrupt" to enable sandboxing in a multithreaded process; Chromium either sandboxes itself before going multithreaded or depends on the SECCOMP_FILTER_TSYNC kernel feature, depending on platform. It's not completely outside the realm of possibility that upstream Chromium might want such a patch; alternately, that code could be altered to allow supplying a hook for turning on the sandbox. Also, the Chromium compiler builds a binary search tree to dispatch on the syscall number, rather than our current sequential search with a few "hot" syscalls moved to the beginning. This will certainly be better in the worst case, and it allows grouping the policy items in the source in the way that makes the most sense (rather than for performance). The one disadvantage is that it could be slightly slower on average; however, I'm not convinced that this is significant, and it's possible to deal with it if it is (as demonstrated by benchmark regressions).

Also, Chromium's sandbox::Trap translates the siginfo_t/ucontext_t into a struct arch_seccomp_data, whereas we want the original to invoke the crash reporter. This might call for some extensions to the Chromium sandbox (ET_RAW_TRAP, Trap::RawTrapFnc, SandboxBPFDSLPolicy::RawTrap, etc.).

Updates: * After bug 1088387, the multithreading problem would also be fixable with a callback from sandbox startup after it tests for multithreadedness and before it turns on seccomp (as opposed to a callback that completely replaces turning on seccomp). * This also needs to happen to unblock upgrading the sandbox chromium version beyond the revision in bug 1041775; there's at least one API we're using that's been removed, and more refactoring to come. (The sandbox policy interface seems likely to be more stable, within chromium, than the seccomp compiler internals.)

(In reply to Jed Davis [:jld] from comment #2) > * After bug 1088387, the multithreading problem would also be fixable with a > callback from sandbox startup after it tests for multithreadedness and > before it turns on seccomp (as opposed to a callback that completely > replaces turning on seccomp). …or without Chromium changes at all, because SandboxBPF assumes the process is single-threaded if it's not passed a /proc fd with which to check the thread count. So we can do the thread count check in our code and then lie to SandboxBPF about being single-threaded.

Better: in more recent chromium, the policy compiler has been separated from the filter-installing code (commit 27e78ad7c43b, 2014-10-16). More importantly, we can install our signal handler after sandbox::Trap installs its handler and delegate, which lets us run code with access to the original signal handler parameters (e.g., for crash handling) before/after the trap handler specified in the policy. I'll block this on bug 1041775, because that brings in bpf_dsl and it seems pointless to deal with the old ErrorCode interface only to redo everything later.

…oops. Still needs to block on either bug 1088387 or bug 1102195, plus everything else.

Move process sandboxing bugs to the new Bugzilla component. (Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

Comment on attachment 8609169 [details] [diff] [review] Step 1: Convert seccomp-bpf policies to Chromium PolicyCompiler. Review of attachment 8609169 [details] [diff] [review]: ----------------------------------------------------------------- // The seccomp-bpf filter for content processes is not a true sandbox // on its own; its purpose is attack surface reduction and syscall // interception in support of a semantic sandboxing layer. On B2G // this is the Android process permission model; on desktop, // namespaces and chroot() will be used. I would propose to clarify that in https://wiki.mozilla.org/Security/Sandbox (for example) and the direction we're taking. This also makes the seccomp code "look less critical" and feel ok/justified with our level of complexity (while the chromium lib abstract a lot of things it also makes some of them more complex to verify during review)

Comment on attachment 8609170 [details] [diff] [review] Step 2: Move SIGSYS handling to Chromium TrapRegistry. Review of attachment 8609170 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/sandbox/linux/Sandbox.cpp @@ +176,5 @@ > } > #endif > > + // TODO, someday when this is enabled on MIPS: include the two extra > + // args in the error message. why no #ifdef __mips__ instead? (mainly because we do that already elsewhere in this file)

(In reply to Guillaume Destuynder [:kang] from comment #11) > > + // TODO, someday when this is enabled on MIPS: include the two extra > > + // args in the error message. > > why no #ifdef __mips__ instead? (mainly because we do that already elsewhere > in this file) That'd probably be ifdef'ed on SECCOMP_PARM[78], but in any case, I didn't bother here because it should be trivial to add when we need it. For correctly handling the architecture differences when reading error status from registers, which is the other ifdef, I wanted to make sure things were structured correctly to allow for that. (In reply to Guillaume Destuynder [:kang] from comment #10) > // The seccomp-bpf filter for content processes is not a true sandbox > // on its own; its purpose is attack surface reduction and syscall > // interception in support of a semantic sandboxing layer. On B2G > // this is the Android process permission model; on desktop, > // namespaces and chroot() will be used. > > I would propose to clarify that in https://wiki.mozilla.org/Security/Sandbox > (for example) and the direction we're taking. > This also makes the seccomp code "look less critical" and feel ok/justified > with our level of complexity (while the chromium lib abstract a lot of > things it also makes some of them more complex to verify during review) I agree. One of my Q2 Deliverables is to fix/rewrite the wiki pages (again) and in general try to communicate better about the higher-level sandboxing strategy / rationale, now that I have at least a vague idea of what it is, which should take care of that. I did a final Try run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=f44748d28a4d) and encountered bug 1168555, which is a sandboxing bug but not actually related to this patch, and the other oranges seem to be intermittent and not sandboxing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/32872aebf4ab https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f98086e8cc https://hg.mozilla.org/integration/mozilla-inbound/rev/6522add87d6b

https://hg.mozilla.org/mozilla-central/rev/32872aebf4ab https://hg.mozilla.org/mozilla-central/rev/b3f98086e8cc https://hg.mozilla.org/mozilla-central/rev/6522add87d6b