Bug 1257361 (CVE-2017-5426)

Firefox doesn't actually enable GMP sandboxing if started with a seccomp-bpf filter already applied

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

({sec-moderate})

Trunk
mozilla53
Unspecified
Linux
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+] sb+)

Attachments

(2 attachments)

Back in bug 969040 / bug 970676, I dealt with needing to start seccomp-bpf after the process is already multithreaded by sending a signal to each thread to do the prctl() call for itself, because seccomp state is (like most things in Linux) per-thread.  And, because threads could clone themselves while this is happening, I had the signal pseudo-broadcast repeat until all threads found themselves already sandboxed.

Problem is, I used PR_GET_SECCOMP, which returns 2 if the thread has one or more seccomp-bpf filters applied, which is not the same as whether it has *our* filter applied.  If Firefox is started with a seccomp-bpf filter already applied, that's inherited by child processes/threads, and the plugin-container threads will all think they have nothing to do and leave threads unsandboxed.  (I haven't tested this yet, but I'm pretty sure that's what will happen.)

In particular, LXC apparently runs containers with a seccomp-bpf filter applied to everything.  I don't know if this applies to all uses of containers -- it would be useful to know what the Docker containers we're using for TaskCluster do, in particular.

This shouldn't be a problem on kernels that have thread sync support (bug 1004011), but that conjecture is also not tested yet.

If unprivileged user namespaces are available… probably GeckoMediaPlugins will completely break, because we'll chroot to a deleted directory before calling dlopen(), and without seccomp-bpf to intercept the open() call it will fail.


Possible approaches:

* Keep a global std::set of tids signalled: not quite perfect, because tid reuse is theoretically possible, unless we do something like block each thread in the signal handler (more DIY futex()ing? read()ing a pipe that the main thread closes?) until everything is done.

* Thread local storage isn't async signal safe, so that won't work.

* Take another signal and use its blocked bit as a per-thread boolean.  That's kind of messy.

glibc does the same thing to implement setuid() and similar, but it doesn't have this problem because it can take a libc-internal lock that prevents concurrent thread creation/destruction.
Group: core-security → dom-core-security
How prevalent of an issue is this? Sounds like it only affect hosting solutions that use something LXC? Basically, a non-user for desktop users though.
Flags: needinfo?(jld)
Whiteboard: sb?
(In reply to Jim Mathies [:jimm] from comment #1)
> How prevalent of an issue is this? Sounds like it only affect hosting
> solutions that use something LXC? Basically, a non-user for desktop users
> though.

s/non-user/non-issue
Probably a non-issue for typical use cases?  There's probably *someone* running desktop apps inside containers, but I'd tend to suspect it's not common.  I only found out about this because someone was trying to get started with desktop sandboxing and was working inside a container (and was looking at the seccomp-bpf state as reported in /proc).

With that in mind, since it seems hard to justify actually fixing this, a simpler approach would be to at least make it fail closed instead of open: adjust the feature tests in SandboxInfo.cpp to report no seccomp-bpf support if the initial state isn't 0 and there's no tsync support.  (This would want a log message and a pointer back to this bug so it's not just silently disabling things.)
Flags: needinfo?(jld)
Hi Jed, based on comment 3, it's unclear if you feel this should be addressed. On one hand, you mention this might be hard to justify fixing, but then suggest a code change. That would qualify, to me, as a potential fix that we'd want (and possibly file a new bug for). Can you shed more light? Thank you.
Flags: needinfo?(jld)
Assuming we don't have this problem with SECCOMP_FILTER_FLAG_TSYNC-enabled kernels, the question is how important is it for the sandbox to work correctly on older kernels where an exist seccomp filter has been applied.

From what Jed explained and what I can see in the code, although not proven, this problem shouldn't occur on kernel's that support SECCOMP_FILTER_FLAG_TSYNC which allows a filter to be reliably applied to all threads in a process. Given that, I think this is low priority. I'd agree that it's probably a non-issue for most users, but we never know how much LXC or environments where Firefox runs with an existing filter will be used in the future. Once we have 1098428, we should know what percentage of our Linux users run kernels with SECCOMP_FILTER_FLAG_TSYNC.

Which kernels have SECCOMP_FILTER_FLAG_TSYNC? That support appears to have gone into Linux kernel version 3.17 [1]. From wikipedia, all supported versions of Ubuntu have kernel 3.2 or newer [2]. Though Debian stable appears to be on 3.16 without this feature [3].

As far as resolving this, could we go along way by just changing SetThreadSandbox to call InstallSyscallFilter even when a filter is already applied yet still return false? It's not perfect, but it seems to me it would solve the LXC problem for older kernels leaving just a small window where threads could be created without our policy applied.

[1] https://github.com/torvalds/linux/commit/c2e1f2e30daa551db3c670c0ccfeab20a540b9e1
[2] https://en.wikipedia.org/wiki/List_of_Ubuntu_releases#Table_of_versions
[3] https://www.debian.org/News/2015/20150426
See Also: → 1098428
(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #4)
> Hi Jed, based on comment 3, it's unclear if you feel this should be
> addressed. On one hand, you mention this might be hard to justify fixing,
> but then suggest a code change. That would qualify, to me, as a potential
> fix that we'd want (and possibly file a new bug for). Can you shed more
> light? Thank you.

The thing I'd like to see fixed (and had been trying to avoid volunteering to do, because at least at first it seemed like a lot of work) is having a case where sandboxing seems to be enabled but actually isn't.  So, that could be fixed either by making sandboxing work in that case or by correctly reporting that it won't work.  The latter is relatively easy, and the former might be much easier than I thought (see the end of this comment). 

(In reply to Haik Aftandilian [:haik] from comment #5)
> Which kernels have SECCOMP_FILTER_FLAG_TSYNC? That support appears to have
> gone into Linux kernel version 3.17 [1]. From wikipedia, all supported
> versions of Ubuntu have kernel 3.2 or newer [2]. Though Debian stable
> appears to be on 3.16 without this feature [3].

Ubuntu's 3.13 branch has it.  That means Ubuntu 14.04 “Trusty Tahr”, and according to https://wiki.ubuntu.com/Kernel/LTSEnablementStack should also include 12.04 “Precise Pangolin” iff it was installed as 12.04.2 or newer (but not 12.04.[01] systems upgraded to the same newer patchlevels; yes, this is confusing).  It's the same set of Ubuntu instances as unprivileged user namespaces, I think (see bug 1041885, but some of the early comments there are wrong and superseded by later ones).

> As far as resolving this, could we go along way by just changing
> SetThreadSandbox to call InstallSyscallFilter even when a filter is already
> applied yet still return false? It's not perfect, but it seems to me it
> would solve the LXC problem for older kernels leaving just a small window
> where threads could be created without our policy applied.

Wouldn't that try to apply the filter twice to every thread under normal circumstances?  And that would raise SIGSYS, because our filter doesn't allow… oh.

So here's another idea: the filter can make prctl(PR_SET_SECCOMP, 2, _) return an error that wouldn't normally happen, and then the threads can just go ahead and call it and see if it succeeds.
Flags: needinfo?(jld)
(In reply to Jed Davis [:jld] from comment #6)
> > As far as resolving this, could we go along way by just changing
> > SetThreadSandbox to call InstallSyscallFilter even when a filter is already
> > applied yet still return false? It's not perfect, but it seems to me it
> > would solve the LXC problem for older kernels leaving just a small window
> > where threads could be created without our policy applied.
> 
> Wouldn't that try to apply the filter twice to every thread under normal
> circumstances?  And that would raise SIGSYS, because our filter doesn't
> allow… oh.
> 
> So here's another idea: the filter can make prctl(PR_SET_SECCOMP, 2, _)
> return an error that wouldn't normally happen, and then the threads can just
> go ahead and call it and see if it succeeds.

I think that's a cool idea. It's slightly imperfect because the existing policy could also have prctl return whatever value is chosen by us, but as a workaround for lack of SECCOMP_FILTER_FLAG_TSYNC, it is probably good enough.

Updated

3 years ago
Whiteboard: sb? → sb+
I have a patch; it's relatively simple.
Assignee: nobody → jld
It's both less and more simple than I thought.  Intercepting PR_SET_SECCOMP can be done, and I can even use statically allocate the `struct sock_fprog` so the filter can excuse only our own seccomp-bpf filter but still trap any library that tries to use seccomp-bpf on its own, which is useful.

But the first thing we do is prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) — a prerequisite for being allowed to use seccomp-bpf — so (1) my first patch broke GMP because we have a default-deny policy for prctl there, and (2) the double-filter prevention can just catch the PR_SET_NO_NEW_PRIVS instead.
Attachment #8815444 - Flags: review?(julian.r.hector)
Attachment #8815444 - Flags: review?(gpascutto)
Attachment #8815444 - Flags: review?(gpascutto) → review+
Comment on attachment 8815444 [details] [diff] [review]
Patch: intercept PR_SET_NO_NEW_PRIVS

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

::: security/sandbox/linux/Sandbox.cpp
@@ -274,5 @@
>  // already was enabled.  Crashes if sandboxing could not be enabled.
>  static bool
>  SetThreadSandbox()
>  {
> -  if (prctl(PR_GET_SECCOMP, 0, 0, 0, 0) == 0) {

Something I noticed after I wrote this patch: the sandbox rule that allows PR_GET_SECCOMP can be removed now.  It's probably harmless, but no need to allow it if nothing is using it.
Comment on attachment 8815444 [details] [diff] [review]
Patch: intercept PR_SET_NO_NEW_PRIVS

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

lgtm.
Attachment #8815444 - Flags: review?(julian.r.hector) → review+
https://hg.mozilla.org/mozilla-central/rev/26025c7f0d29
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: dom-core-security → core-security-release
Should we consider uplifting this to 52 for the next ESR?
Flags: needinfo?(jld)
Comment on attachment 8815444 [details] [diff] [review]
Patch: intercept PR_SET_NO_NEW_PRIVS

Yes, let's try to get this on 52 for ESR.

Approval Request Comment
[Feature/Bug causing the regression]: This bug goes back to the original seccomp-bpf sandboxing in the B2G era; specifically, bug 970676.
[User impact if declined]: A silent lack of sandboxing for media plugins.  Limited to users running Linux Firefox inside certain kinds of containers (or something that applies a seccomp-bpf policy); this may be more common on ESR than regular Release.
[Is this code covered by automated tests?]: Media plugin tests will cover it.
[Has the fix been verified in Nightly?]: Yes; it's been on Nightly since 2016-12-12.
[Needs manual test from QE? If yes, steps to reproduce]: I've manually tested; I don't think QE needs to verify.  (There is STR, but it's complicated and uses a small C program that I haven't attached to this bug yet.)
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not particularly.
[Why is the change risky/not risky?]: The patch is subtle, but (1) in the absolute worst case the damage is limited to media plugins (OpenH264 and EME) on Linux, and (2) this patch has been on Nightly for ~2 months now, and on Aurora since the last merge.
[String changes made/needed]: None.


Incidentally, this patch was uplifted to the 52 branch and then immediately backed out.  I'm not quite sure what was going on there, but see https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=2f4972fb62ed and https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=b54a7014e03d.  (The build breakages were caused by the patch that mentions minidump-analyzer, not this one.)  I mention this in case there are tools that would get confused by those earlier commits.
Flags: needinfo?(jld)
Attachment #8815444 - Flags: approval-mozilla-beta?
STR, for the record:
1. Compile this program ("cc -Wall -o seccomp-null seccomp-null.c" or similar)
2. MOZ_FAKE_NO_SECCOMP_TSYNC=1 MOZ_ASSUME_USER_NS=0 /path/to/seccomp-null ./mach run
(The env vars disable using features of newer kernels that will, respectively, make the bug not reproduce or make it completely break GMP instead of silently lowering security.)
3. Launch a media plugin:
3a. Wait for OpenH264 to download if it isn't already (check about:addons)
3b. Go to https://mozilla.github.io/webrtc-landing/pc_test.html
3c. Check “Require H.264”
3d. Start a chat
3e. Make sure the video loopback is working
4. gdb -p $(pgrep -f geckomediaplugin) --batch -ex 'call reboot(1)'
(This assumes there's only Gecko media plugin running anywhere on the system; if not, find the right one's pid and use it directly.  I'm also assuming that any system security features that restrict debugging are disabled.)
4a. Expected result: plugin crash, "sandbox violation" message on terminal (after a few seconds of frozen video while gdb starts up)
4b. Bad result: gdb says "$1 = -1", plugin continues running
4c. Inconclusive result: gdb prints errors and immediately exits
Comment on attachment 8815444 [details] [diff] [review]
Patch: intercept PR_SET_NO_NEW_PRIVS

seccomp sandboxing fix, beta52+
Attachment #8815444 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: sb+ → [post-critsmash-triage] sb+
Whiteboard: [post-critsmash-triage] sb+ → [post-critsmash-triage][adv-main52+] sb+
Alias: CVE-2017-5426
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.