1506291 - Linux sandboxing for the media decoder process (RDD)

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement, P1)

Description

[Jed Davis [:jld] (on leave until 08-31)]

I'm going to take the Linux part of bug 1498624.  It's a bit more complicated than Mac: the policy will have to be slightly more permissive than for GMP (to allow allocating shared memory), and the glue that's needed in RDD{Parent,Child} will be more similar to content processes, but also there's boilerplate common to content and media that doesn't need to be duplicated at all (e.g., SandboxInfo::CanSandboxMedia). 

With bug 1500297, the seccomp-bpf policy should need few if any overrides from SandboxPolicyCommon.

Comment 1

[Jed Davis [:jld] (on leave until 08-31)]

I have a patch; it compiles but it's not tested yet.

Comment 2

[Jed Davis [:jld] (on leave until 08-31)]

Attachment: Bug 1506291 - Add Linux sandboxing for the RDD (media decoder) process.

The seccomp-bpf policy is currently just the "common" policy with no
additions (but with the fixes in bug 1511560 to enable shared memory
creation).  The file broker policy allows shared memory creation and
nothing else.  The namespace setup is the same as for GMP (i.e., as
restrictive as we currently can be).

The sandbox can be turned off for troubleshooting by setting the
environment variable MOZ_DISABLE_RDD_SANDBOX, similarly to the other
process types.

Tested against https://demo.bitmovin.com/public/firefox/av1/ with the
necessary prefs set.

Depends on D14524

Comment 3

[Jed Davis [:jld] (on leave until 08-31)]

There's a slight problem I didn't pick up at first: PR_GetNumberOfProcessors, which opens files both explicitly[1] and, failing that, implicitly via sysconf().  It also does some fcntl()ing as a side-effect of fopen, which would also have to be allowed.  With the patches as posted, it's going to quietly limit the decoding to a single thread.

The calls seems to be from [2] and [3], so that could be adjusted to use a cached value instead.  I don't know if responding dynamically to CPU hotplugging; the comment (see also bug 663970) suggests that CPUs could be offlined for power management, but I don't know if anyone in 2018 does that or how important it is to adjust the thread count to follow it.  Another option is interposing PR_GetNumberOfProcessors.


[1] https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/nsprpub/pr/src/misc/prsystem.c#221
[2] https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/dom/media/platforms/agnostic/AOMDecoder.cpp#56
[3] https://searchfox.org/mozilla-central/rev/d6f2d4bb73446534a9231faefae05934fcd05911/dom/media/platforms/agnostic/DAV1DDecoder.cpp#31

Comment 4

[Jed Davis [:jld] (on leave until 08-31)]

There's also the matter of XPCOM_MEM_BLOAT_LOG (see bug 1514274) and any other similar debug-only log files, if they aren't opened unconditionally before sandboxing is started.  That needs to be tested to see whether it's a problem on Linux.

Comment 5

[Jed Davis [:jld] (on leave until 08-31)]

Actually, if we block everything, PR_GetNumberOfProcessors() will return 2, not 1, on newer glibcs: https://sourceware.org/bugzilla/show_bug.cgi?id=21542

That's still not what it should be.  In a content process we could probably use nsSystemInfo's cached CPU count, but that component doesn't seem to exist in the RDD process (the do_GetService returns nullptr), so that won't work.

I've used symbol interposition to freeze the value of PR_GetCurrentNumberOfProcessors() the first time it's called, and ensure that that happens before sandbox start, and that works, but landing it might be a little controversial.

Comment 6

[Jed Davis [:jld] (on leave until 08-31)]

Another idea for the CPU count problem: use sched_getaffinity with pid 0 (current thread) and count the number of CPUs available to the calling thread and its descendents.  No filesystem access, and easy to separate from more dangerous calls with seccomp-bpf, and the technique is already used by other libraries we use, like https://crates.io/crates/num_cpus; on the other hand, it can theoretically include CPUs that don't exist (e.g., offline), and it's annoying to handle systems with CPU indices above 1023 if we wanted to do that.

This could even be upstreamed into NSPR, but I think I'd prefer to do this by interposition first so this doesn't block on an NSPR release cycle (and/or any problems that come up on distros that NSPR supports but Firefox's Linux sandboxing doesn't, like RHEL 5/6: for example, we may not care about machines with a very large number of CPUs, but Red Hat might).

Allowing that in the common policy means allowing it for GMP when it wasn't before, but explicitly exposing the CPU count is probably not a significant privacy risk given that it could already be determined by starting threads and measuring timing (assuming it wasn't in memory somewhere already, which it probably was).

Comment 7

[Jed Davis [:jld] (on leave until 08-31)]

gcp, do you have any opinions about CPU counting, since you were the last person to touch this?

I'm actually leaning towards the earlier idea of interposing PR_GetCurrentNumberOfProcessors to save the value from the first time it's called, because that's using code that's already well-tested (and the sched_getaffinity man page suggests that using sysfs or procfs is actually the correct way to count CPUs), and the only real drawback is that it won't notice dynamic changes to online CPUs, which sched_getaffinity also doesn't notice (in fact it seems as if it would count the maximum number of CPUs the system could ever have, if I'm understanding the docs correctly) and it's not clear that that's actually an issue in practice.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Your last comment seems to suggest that keeping the logic from PR_GetCurrentNumberOfProcess is preferable, even if that means calling in the parent (with or without caching).

It's unfortunate using nsSystemInfo does not work - it seems that would be the cleanest. Interposing functions seems like a weird solution given that we control all code here (to some extent...)? Can't we just add a new function, get the value before the sandbox goes up, and use that?

If we make more types of processes, this kind of probing of basic information about the system is likely to be a recurring issue.

Dynamic onlining of processors if a bit of a weird technique in that on the software side (not system software, which may know the truth) it makes it very hard to predict how many threads to use for work - I doubt it's very common and it's OK to not handle it well I think.

Comment 9

[Jed Davis [:jld] (on leave until 08-31)]

Attachment: Bug 1506291 - Move the AV1 decoders to a sandbox-friendly CPU counting wrapper.

Counting CPUs accesses the filesystem (sysfs or procfs), which we'd like
to disallow when sandboxed if possible, and fails silently if access
is denied. Because the CPU count rarely changes, this patch handles
that problem for the RDD process by caching a copy before starting
sandboxing.

Tested with a local patch to have the sandbox file broker client crash
if accessing the sysfs node for the CPU count, to verify that it's not
accessed.

Depends on D14524

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf58d8320f5a
Move the AV1 decoders to a sandbox-friendly CPU counting wrapper. r=gcp,mjf
https://hg.mozilla.org/integration/autoland/rev/493b443954fe
Add Linux sandboxing for the RDD (media decoder) process. r=gcp,mjf,flod

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bf58d8320f5a
https://hg.mozilla.org/mozilla-central/rev/493b443954fe