+++ This bug was initially created as a clone of Bug #919090 +++ I've found two more un-whitelisted syscalls while trying to use profiling features on my keon: 1. Starting the profiler requires sigaction, to establish the signal handler. This isn't a problem for starting the profiler with MOZ_PROFILER_STARTUP, because that runs before the seccomp filter is established, but blocking it prevents starting the profiler asynchronously later on. 2. tgkill is used to send signals to a specific thread when the "threads" feature is enabled. It'd be a trivial change to add those syscalls, but we could do better: 1. We can't inspect the struct sigaction passed by pointer, but we can restrict sigaction() to specific signals — i.e., we can allow changing the handling of SIGPROF only (but not, e.g., SIGSEGV or SIGSYS). 2. tgkill is always called with tgid equal to the current process ID (which is constant over the lifetime of the filter program), and used only to send SIGPROF. Other uses can be disallowed. --- There are probably other syscalls we're allowing (or may need to allow in the future) that can be made less worrisome in a similar way. So let's consider that. Related question: how much runtime information will the filter wind up needing? Just something simple like writing getpid() into an immediate field, or more than that? (i.e., will we ever want to write an ssembler library to assemble it at runtime?)

I have looked at using a runtime compiler like chromium started doing, which helps in particular for argument filtering. In general, I would much favor not filtering arguments when other solutions are possible, as filtering adds not only performance penalties but also complexity, which generally leads to bugs. In fact, I would argue that for argument filtering there are currently frameworks which allow for a better overview of the involved objects than seccomp (such as selinux). Seccomp on the other hand has the advantage of being fast (bpf) and extremely straight-forward (as long as argument filtering is not used too extensively). I'm also aware of some other efforts to bring the same high level overview of various objects for seccomp, through a more advanced policy compiler, but that's basically bringing seccomp to something more similar to selinux, albeit in userspace. Anyhow, this is to say that I believe we'll want argument filtering on some specific calls because it brings us to a better place *right now*, but that we should be careful when using it, and generally attempt to find a cleaner solution, IMO. I'm guessing this type of profiling is not used for performance measurements, since it impacts performance anyay, thus it may be ok to have a runtime flag that enables a different whitelist, where we would be less strict. Also pinging :briansmith to get his opinion on this.

I was only thinking of using it for calls like ioctl and prctl that multiplex lots of stuff (ioctl is of particular interest given issues like [1]), and locking down signal handling for the profiler. There's a limit to how much complexity we can have in any case, since (by design) we don't have access to the process's address space, only the register values passed in as parameters. Also, I don't think we'd need to compile the filter program at runtime, at least not for anything I can think of. We probably won't need anything fancier than filling in immediates, like for getpid() and tgkill and the profiler. What we will want is a more expressive assembler, but we already have an actual assembler in the toolchain. [1] https://www.codeaurora.org/projects/security-advisories/multiple-issues-diagkgsl-system-call-handling-cve-2012-4220-cve-2012

that sounds good, this is where i would also use it (ioctl, prctl for example) - in fact, this is where we wanted to make that exception when we discussed it with :briansmith in the past.

bbondy is now working on bringing the entire Chromium sandbox to Gecko. We should coordinate how that is going to work. It seems ideal for us to use the same sandboxing logic on desktop Linux and B2G, as much as possible. It also seems ideal to avoid investing too much work in the existing seccomp-bpf filtering if we are likely to switch to the Chromium sandboxing code wholesale anyway.

One thing about my "minimalist" assembler-based approach: ioctl selectors are defined with C macros that use sizeof on C types, so they wouldn't work with plain asm. We could hard-code them (either the size or the whole thing), but we might not want to do that. Values of compile-time constant C expressions could be transported into an inline asm with "i" input parameters and the not-very-documented %c[name] form (which may or may not be supported by not-quite-GCC compilers like Clang), but there's a limit of 30 — and inline asm can't use the C preprocessor the way a .S file can, which causes problems for getting syscall numbers and the BPF opcode constants into the asm. It'd be possible to use hidden absolute symbols to smuggle numbers from one file into another (and have them resolved by the dynamic loader(!)), but that seems kind of sketchy. And that's on top of the defines I've already copypasted and adjusted to separate them from C-specific definitions. So a runtime assembler might be the less bad approach. I think the amount of code involved can be made small enough to scrutinize usefully — or, as mentioned, that might be a cue to see if the Chromium sandbox is less work.

…and it looks like some of the gap between comment #2 and comment #5 is not actually in this bug yet. So, I had a patch where I converted the whitelist from CPP macros to assembler macros (expanding to .long/.short/.byte directives, to populate a symbol in the data section), and used the GNU assembler's label resolution facilities (with a bit of arithmetic) to compute the BPF instruction offsets. End result being a way to easily deal with logic more complex than a single if-chain (and to represent that if-chain in ½ as many instructions), and label individual instructions for patching (as with getpid()). But see previous comment for some of the shortcomings.

Something that needs to happen given if b2g on x86 (e.g., the Geeksphone Revolution) takes off: the architecture-specific "ipc" system call, which multiplexes all the syscalls involving sockets or SysV IPC.

I took a look at the Chromium sandbox's filter-building code. We should be able to use at least the codegen module easily enough — it's most of what I was trying to avoid writing, and it comes with unit tests (for the Chromium part) and a disassembler (so we can visually inspect the filter program to check the Gecko-specific part of this).

I've attached my current work-in-progress state, which is mostly done. It depends on the patch from bug 997409. Also, patch 1 (the fix to the Chromium code) is already committed upstream, if I recall correctly, so what I should actually do is update patch 0 (the Chromium import) with a newer upstream version and drop that one. Trying: https://tbpl.mozilla.org/?tree=Try&rev=8642d376f75e

I don't know if this also needs license review, given that there's already a partial import of Chromium here.

Reasons we want to do all of this: 1. Filtering on arguments, like it says in the bug summary. There are patches here for tgkill and socketcall, but ioctl is a goal (constructing a sufficiently large whitelist will be nontrivial, though), and there are a few others like prctl and fcntl. 2. Gives us the ability to use more complex conditions without error-prone hand-coding of BPF assembly. Obviously we'd like to keep the filter simple if possible, but this allows us to do that if it's the least bad alternative to allowing a potentially dangerous syscall unconditionally. 3. Dynamically constructing the filter, which gives us the option of including values and decisions determined at runtime. 4. It will make it easier to use more of the Chromium sandbox code instead of our own semi-reinvention, if it makes sense to go that way.

Comment on attachment 8417700 [details] [diff] [review] bug920372-p0-import-hg0.diff Review of attachment 8417700 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/sandbox/chromium/sandbox/linux/seccomp-bpf/bpf_tests.h @@ +91,5 @@ > + } else { > + printf("This BPF test is not fully running in this configuration!\n"); > + // Android and Valgrind are the only configurations where we accept not > + // having kernel BPF support. > + if (!IsAndroid() && !IsRunningOnValgrind()) { This might be different than our current implementation, ie maybe we're not testing for this on desktop with the chromium implemetation/require seccomp (?)

Comment on attachment 8417703 [details] [diff] [review] bug920372-p3-socket-hg0.diff Review of attachment 8417703 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/sandbox/linux/SandboxFilter.cpp @@ +103,5 @@ > * 'strace -c -p <child pid>' for most used web apps. > */ > > Allow(SYSCALL(futex)); > + // FIXME, bug 920372: We should check the selector. do you plan to add this in a future patch on the same bug?

i would recommend having an additional reviewer due to the size and complexity of the code in the patches- albeit i guess chromium patches already got thorough review at Google.

(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #21) > Comment on attachment 8417703 [details] [diff] [review] > > + // FIXME, bug 920372: We should check the selector. > > do you plan to add this in a future patch on the same bug? I think I can just remove that comment, and the other FIXMEs pointing at this bug. I thought I'd already removed them, actually, but there were a few rebases and a partial rewrite between then and now. (In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #20) > Comment on attachment 8417700 [details] [diff] [review] > ::: security/sandbox/chromium/sandbox/linux/seccomp-bpf/bpf_tests.h > @@ +91,5 @@ > > + } else { > > + printf("This BPF test is not fully running in this configuration!\n"); > > + // Android and Valgrind are the only configurations where we accept not > > + // having kernel BPF support. > > + if (!IsAndroid() && !IsRunningOnValgrind()) { > > This might be different than our current implementation, ie maybe we're not > testing for this on desktop with the chromium implemetation/require seccomp > (?) We're not actually running these tests, at least not yet. I just copied the entire seccomp-bpf directory, even though only a small part of it is actually being used here, because that seemed simpler. I can try to be clearer about this in the commit message.

(In reply to Guillaume Destuynder [:kang] (use NEEDINFO!) from comment #22) > i would recommend having an additional reviewer due to the size and > complexity of the code in the patches- albeit i guess chromium patches > already got thorough review at Google. I tend to think we're okay for the code imported from Chromium. Do you think we need more review for the second patch in the series ("Use Chromium seccomp-bpf compiler to dynamically build sandbox program.")? If so, do you have any idea who'd be a good reviewer for it?

I would try :dkeeler for example. That said we're probably ok. Its just quite a bit of code :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1541c3c6e894 https://hg.mozilla.org/integration/mozilla-inbound/rev/b56d5602d0cd https://hg.mozilla.org/integration/mozilla-inbound/rev/2adbb2797d8b https://hg.mozilla.org/integration/mozilla-inbound/rev/179363be5641 ( https://tbpl.mozilla.org/?tree=Try&rev=cfa0f6ee1fac )

https://hg.mozilla.org/mozilla-central/rev/1541c3c6e894 https://hg.mozilla.org/mozilla-central/rev/b56d5602d0cd https://hg.mozilla.org/mozilla-central/rev/2adbb2797d8b https://hg.mozilla.org/mozilla-central/rev/179363be5641