1257361 - (CVE-2017-5426) Firefox doesn't actually enable GMP sandboxing if started with a seccomp-bpf filter already applied

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jim Mathies [:jimm]]

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.

Comment 2

[Jim Mathies [:jimm]]

(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

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.)

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 5

[Haik Aftandilian [:haik]]

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

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 7

[Haik Aftandilian [:haik]]

(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.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I have a patch; it's relatively simple.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Patch: intercept PR_SET_NO_NEW_PRIVS

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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 12

[Julian Hector [:tedd] [:jhector]]

Comment on attachment 8815444 [details] [diff] [review]
Patch: intercept PR_SET_NO_NEW_PRIVS

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

lgtm.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26025c7f0d298872c5fe88739c9897254da6fc75

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26025c7f0d29

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Should we consider uplifting this to 52 for the next ESR?

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: C program that applies an allow-all seccomp-bpf policy

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 18

[Julien Cristau [:jcristau]]

Comment on attachment 8815444 [details] [diff] [review]
Patch: intercept PR_SET_NO_NEW_PRIVS

seccomp sandboxing fix, beta52+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52318ef46d8b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/52318ef46d8b