|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
We're seeing an injected library, libesets_pac.so, doing at least two things that conflict for sandboxing: 1. Calling fcntl(_, F_GETFL, _), which is relatively harmless on its own but isn't allowed by the media plugin policy because it's normally not used there. Through 53 this has been crashing the plugin process; from 54 onward (bug 1286865) it will just fail on non-Nightly (the crash reports for this have all been from release versions). 2. In content processes, where that fcntl is allowed, it seems to be using SysV message queues. Creating them was allowed in bug 1285902 (it's not mentioned in the bug itself, but there are notes elsewhere tying it to libesets_pac.so), but not actually using them. However, the telemetry from bug 1286865 has yielded a couple of pings for msgsnd(); that doesn't include the stack so I can't directly tie it to libesets_pac.so, but I suspect it's related. There's documentation at https://help.eset.com/efs/4/en-US/index.html?libc_installation_configuration.htm; in particular this seems to be a “server” targeted product, but apparently some people are applying it globally and then running Firefox. In any case we shouldn't have Firefox just crash (or lose features), because people will blame us. Ideally we'd like to have sandboxed child processes just not get problematic preloads like this. We can use an LD_AUDIT module to prevent loading a library, even if it's listed in /etc/ld.so.preload. But if there's a simpler opt-out that would work in this particular case, we might want to do that instead.
To clarify the rationale for blocking the library: on-access file scanning doesn't seem to add much to sandboxed child processes. Specifically: * Once a content process starts sandboxing, it can't open files except via the parent process; the open() system call is intercepted with seccomp-bpf and replaced by messaging the parent over a local-domain socket. Therefore, any file scanning would already have been done in the parent process. Before the process is sandboxed, the files it's accessing are part of the firefox install itself, and presumably already scanned. * Media plugins (EME CDMs and OpenH264) are a little different. They don't use a file broker in the parent process, because there's only one open() that needs to be handled: the dynamic linker opening the plugin .so file. The plugin process opens the file itself before starting sandboxing, then calls dlopen() and uses that fd. So that might be problematic for the AV threat model. For the sandbox threat model we consider the plugins themselves untrusted (this is why we sandbox the dlopen() call) and require seccomp-bpf support to enable them. As for the possibility of *not* blocking the library: loosening the policy for fcntl() might be acceptable (with some restrictions; see bug 1328896), but I don't want to allow more SysV IPC — longer term we'd like to stop using SysV shared memory in content processes and unshare the IPC namespace where possible. (And for media plugins we already unshare IPC and block all the syscalls.) It would be possible to detect the library and weaken the sandbox only if it's present, but I'd rather not do that if there's an alternative. The other outstanding question is: what happens if we return an error for some/all of these syscalls? (This is what would happen on 54 beta, and what will happen on 54 release if we don't change something.) I've been assuming that the interposed functions will “fail closed” and break everything, but it's possible that that's not the case.
Blocking the library has problems — there are failures on Try that I can reproduce locally, which seems to be https://bugs.launchpad.net/ubuntu/+source/eglibc/+bug/1243473 . Newer distributions won't have that bug, and older distributions (e.g., RHEL 5/6) won't have seccomp-bpf so none of this matters, but Ubuntu 12.04 is in the middle. (It's also end-of-life, as of this month, but Mozilla is still using it for CI.) In any case, the fact that LD_AUDIT could be that thoroughly broken without being noticed (and apparently never fixed in the entire 5-year lifespan of 12.04?) means I don't want to risk uplifting a patch that uses it, especially not this close to release. Given the state of the content sandbox policy in 54, if it's just a matter of allowing msgsnd (and planning to fix things properly later), that's not much of a problem.
I'm not sure how I missed this, but libesets_pac.so is also part of a consumer-targeted product, NOD32, not just the enterprise server product documented by the link in comment #0, and I'd guess that that's actually what's behind most if not all of the usage with Firefox. It's also freely downloadable and can be licensed without having to negotiate with salespeople, which is useful given the time pressure. So I did that, and tested with beta 54. Short version: content works fine, and media plugins also work now (because bug 1286865 means rejected syscalls fail with ENOSYS instead of crashing like they did in versions ≤53). Longer version: I haven't yet found steps to reproduce the content process calling msgsnd(), so it's not too surprising that the content process works. A media plugin process, however, rejects many syscalls and still works (tested with OpenH264), so the interpositions seem to not be failing closed, which suggests there's no cause for alarm about msgsnd(). More specifically: it's trying to use named Unix-domain sockets, SysV message queues, and SysV shared memory; all of this is going to be blocked in content processes eventually. (Also, write to /dev/console, but I think that's already blocked.) So, for the longer-term solution: not-Nightly seems to be fine, and for Nightly we could just turn off the crash-on-error behavior if we detect this library (dlopen with RTLD_NOLOAD).
Comment on attachment 8875527 [details] Bug 1362601 - Don't crash on sandbox violation if known-problem injected libs are present. https://reviewboard.mozilla.org/r/146952/#review151372
Attachment #8875527 - Flags: review?(gpascutto) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/f7450fa2d7aa Don't crash on sandbox violation if known-problem injected libs are present. r=gcp
Branch Freeze Note: this is all inside #ifdef NIGHTLY_BUILD, so it won't affect Beta.
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
This is also a problem on ESR52 with EME: bp-f677fe7b-bdae-4b36-84df-7b7750170808 I need to decide what to do about that; the “fix” to uplift is actually part of bug 1286865.
On second thought, I really don't think release management would agree to uplift anything unless the crash volume were a lot larger.
You need to log in before you can comment on or make changes to this bug.