1012951 - Sandbox the OpenH264 plugin for Linux

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Maire Reavy [:mreavy]]

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.

Comment 1

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

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

Comment 2

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

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.

Comment 3

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

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.

Comment 4

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

Attachment: bug1012951-gmp-sandbox-wip0.diff

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.

Comment 5

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

Attachment: bug1012951-gmp-sandbox-wip1.diff

Shiny new work-in-progress.  Starts the sandbox before the library is loaded.

Comment 6

[Maire Reavy [:mreavy]]

Finally getting around to updating the bug component to security (which is more accurate).

Comment 7

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

Attachment: bug1012951-gmp-sandbox-hg0.diff

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

Comment 8

[Tim Abraldes [:TimAbraldes] [: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.

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

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

Attachment: bug1012951-gmp-sandbox-hg1.diff

Rebase.  Carrying over r+es.

Comment 11

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

Attachment: bug1012951-gmp-sandbox-hg2.diff

…and with this trivial change, it passes the recently added H.264 tests on try: https://tbpl.mozilla.org/?tree=Try&rev=f1d674e3e736

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d50d7e88f35e

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d50d7e88f35e

Comment 14

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

I'll request uplift once this has had some time on m-c.

Comment 15

[Ed Morley [:emorley]]

Backed out for causing bug 1046165 (not your fault; test coverage was down).

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/99e1f3c50a12

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/99e1f3c50a12

Comment 18

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

Attachment: 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.

Comment 19

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

Attachment: bug1012951-gmp-sandbox-hg3.diff

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

Comment 20

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

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

[Guillaume Destuynder [:kang] [this account is no longer active]]

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

Comment 22

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

Bug 1034138 means that the sched_getaffinity soft-fail needs to be unifdef'ed, it looks like.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b60e4395f141

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b60e4395f141

Comment 25

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

Attachment: Patch for uplift to Aurora

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/90e489c8fc4b