Sandbox the OpenH264 plugin for Linux

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mreavy, Assigned: jld)

Tracking

(Blocks 2 bugs)

unspecified
mozilla34
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

For security reasons we want to sandbox the OpenH264 plugin (which runs in separate process).  This bug handles Linux. This is a spin off from Bug 985252.
OS: Windows 7 → Linux
Hardware: x86_64 → All
Assignee: nobody → jld
Depends on: 1020090
If bug 1020090 can't happen soon enough, it might be possible to "fix" that problem by having the parent create the shm segments and pass them to the child.  (The performance implications would remain.)
I applied the patch from bug 985252 and modified it to attach strace(1) to the process at the point where sandboxing would be started, and ran attachment 8436213 [details].  System call counts:

      1 restart_syscall
      4 clone
      4 prctl
      4 set_robust_list
     16 gettid
   1354 ftruncate
   1354 open
   1354 stat
   1354 unlink
   2708 dup
   2708 fcntl
   2708 lseek
   3396 sendmsg
   4085 munmap
   4109 mmap
   4467 recvmsg
   4891 epoll_wait
   5426 close
   5432 fstat
   6234 poll
   6270 futex
   7510 read
   7510 write
  19034 mprotect

The open/stat/unlink calls are all for SHM segments, which bug 1020090 should take care of; the fcntl calls are also part of that.  prctl is always PR_SET_NAME.  And everything else should be relatively harmless.
So, with the current WIP on bug 1020090, plus the PGMP::SetProcessSandbox stuff from bug 985252, there are no more filesystem references.

But also, if we wanted to turn on sandboxing before the call to PR_LoadLibrary and prevent any part of a media plugin from running unsandboxed, that should be possible: the path to the library file is known ahead of time, so we can pre-open it before sandboxing and simulate the actual open() in the SIGSYS handler.  That plus preloading locale and iconv support seems to be enough, judging by strace.
Current work in progress.  Depends on patches from bug 985252 and bug 1020090.  Assorted caveats noted in commit message and in comments (and it probably breaks the build on non-Tier-1 Linux platforms).  On my system this lets OpenH264 work (and exit normally, and invoke the crash reporter) without sandbox violations.
No longer depends on: 985252
Shiny new work-in-progress.  Starts the sandbox before the library is loaded.
Attachment #8450724 - Attachment is obsolete: true
Finally getting around to updating the bug component to security (which is more accurate).
Component: WebRTC → Security
I think this has reached the point where I can stop calling it "work in progress".  I've tested locally on Debian unstable and Ubuntu 12.04, both 64-bit and 32-bit x86.  

The f? is because I've changed how the Windows build enables GMP sandboxing (no functional change intended).  Because not all Linux-based platforms will use GMP sandboxing (for now) I wanted a flag for it instead of duplicating the test in multiple places, so it seemed reasonable to generalize it.

Try: https://tbpl.mozilla.org/?tree=Try&rev=0ea0bf1ce749
Attachment #8454102 - Attachment is obsolete: true
Attachment #8459424 - Flags: review?(ted)
Attachment #8459424 - Flags: review?(gdestuynder)
Attachment #8459424 - Flags: feedback?(tabraldes)
Comment on attachment 8459424 [details] [diff] [review]
bug1012951-gmp-sandbox-hg0.diff

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

> The f? is because I've changed how the Windows build enables GMP sandboxing
> (no functional change intended).  Because not all Linux-based platforms will
> use GMP sandboxing (for now) I wanted a flag for it instead of duplicating
> the test in multiple places, so it seemed reasonable to generalize it.

This seems fine to me, but the real person who should take a look is a build system peer.
Attachment #8459424 - Flags: feedback?(tabraldes) → feedback+
Comment on attachment 8459424 [details] [diff] [review]
bug1012951-gmp-sandbox-hg0.diff

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

::: content/media/gmp/GMPChild.cpp
@@ +25,5 @@
>  #if defined(XP_WIN)
>  #define TARGET_SANDBOX_EXPORTS
>  #include "mozilla/sandboxTarget.h"
> +#elif defined(XP_LINUX) && defined(MOZ_GMP_SANDBOX)
> +#include "mozilla/Sandbox.h"

Kind of sucks that the API is different here per-platform.
Attachment #8459424 - Flags: review?(ted) → review+
Rebase.  Carrying over r+es.
Attachment #8459424 - Attachment is obsolete: true
Attachment #8464329 - Flags: review+
…and with this trivial change, it passes the recently added H.264 tests on try: https://tbpl.mozilla.org/?tree=Try&rev=f1d674e3e736
Attachment #8464329 - Attachment is obsolete: true
Attachment #8464502 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d50d7e88f35e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
I'll request uplift once this has had some time on m-c.
Flags: needinfo?(jld)
Depends on: 1046165
Backed out for causing bug 1046165 (not your fault; test coverage was down).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/99e1f3c50a12
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: 33
Whiteboard: 33
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/99e1f3c50a12
Target Milestone: mozilla34 → ---
Duplicate of this bug: 1046538
I'll squash the patches for review and commit, because I don't think it makes sense to land them separately and leave a broken intemediate state, but this is in case of bugzilla interdiff failure.
Flags: needinfo?(jld)
I think I should get re-review for the incremental changes (see also attachment 8466472 [details] [diff] [review]).

Try: https://tbpl.mozilla.org/?tree=Try&rev=db0c695346a1
Attachment #8464502 - Attachment is obsolete: true
Attachment #8466473 - Flags: review?(gdestuynder)
(In reply to Jed Davis [:jld] from comment #18)
> Created attachment 8466472 [details] [diff] [review]
> Incremental changes from bug 1046538
> 
> I'll squash the patches for review and commit, because I don't think it
> makes sense to land them separately and leave a broken intemediate state,
> but this is in case of bugzilla interdiff failure.

…actually, the interdiff is a little larger (or would be if it could handle this case, which it can't).  I switched from nsCString to a strdup()ed const char* to store the plugin path given that bug 1041886 will require that anyway; and given that assuming nsCString::get to be async-signal-safe, even if that currently happens to be true, seems like a bad idea.
Comment on attachment 8466473 [details] [diff] [review]
bug1012951-gmp-sandbox-hg3.diff

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

Thanks for the details :) Better interdiff would be nice ;)
Attachment #8466473 - Flags: review?(gdestuynder) → review+
Bug 1034138 means that the sched_getaffinity soft-fail needs to be unifdef'ed, it looks like.
https://hg.mozilla.org/mozilla-central/rev/b60e4395f141
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Approval Request Comment
[Feature/regressing bug #]: Bug 948160
[User impact if declined]: No sandboxing for OpenH264 on Linux.
[Describe test coverage new/current, TBPL]: Buildbot has been successfully using this since it landed on m-c.
[Risks and why]: We may encounter Linux distributions that require sandbox policy changes (different versions of libc, etc.) — but that will happen regardless of what train this rides, and I want to get as much lead time between that and shipping EME as possible.
[String/UUID change made/needed]: None.
Attachment #8473928 - Flags: review+
Attachment #8473928 - Flags: approval-mozilla-aurora?
Attachment #8473928 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.