Closed Bug 1012949 Opened 10 years ago Closed 10 years ago

Sandbox the OpenH264 plugin for Mac

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34
Tracking Flags:
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: mreavy, Assigned: areinald.bug)

References

(Blocks 1 open bug,
URL
)

Details

(Whiteboard: aurora uplift note: see comment #110 [openh264-uplift?])

Attachments

(11 files, 9 obsolete files)

Interpose library for observing calls to sandbox_init...
7.07 KB, application/x-tar
Details
Stack traces of calls to GMP exports
11.13 KB, text/plain
Details
plugin-container.crash
2.25 KB, text/plain
Details
bugzilla-1012949-2.patch
2.71 KB, patch
Details | Diff | Splinter Review
plugin-container.sb.log
1.88 KB, text/x-log
Details
Read sandbox rules from a text file instead of hard-coded in source.
2.72 KB, patch
Details | Diff | Splinter Review
Patch rev3
10.16 KB, patch
Details | Diff | Splinter Review
Even more factorized version of the rules. Should be modified to include 10.6 compatibility.
548 bytes, text/plain
Details
sandbox rules syntax.txt
1.27 KB, text/plain
Details
Patch rev6 (respond to jesup's comments)
11.91 KB, patch
jesup
: review+
areinald.bug
: review+
ted
: review+
Details | Diff | Splinter Review
Patch for uplift to aurora
11.97 KB, patch
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
For security reasons we want to sandbox the OpenH264 plugin (which runs in separate process).  This bug handles the Mac OS. This is a spin off from Bug 985252.
OS: Windows 7 → Mac OS X
Hardware: x86_64 → All
From https://bugzilla.mozilla.org/show_bug.cgi?id=985252#c13 (comment by Steven Michaud):

I've been asked to take a look at maybe writing the Mac implementation.  Before I do anything else, I need to understand far more about Mac sandboxing than I do, including the changes/additions to our code and infrastructure it's likely to require.  The first thing I noticed is this quote from https://developer.apple.com/library/mac/documentation/Security/Conceptual/AppSandboxDesignGuide/AboutAppSandbox/AboutAppSandbox.html:

"The elements of App Sandbox are container directories, entitlements, user-determined permissions, privilege separation, and kernel enforcement."

The bit about "entitlements" means we'll need to change how Mac Firefox distros are signed.  I hope these changes won't require any changes to our signing infrastructure ... but we can't count on that.  So my first task will be to find out exactly how "entitlements" are involved here, and what changes they'll require to how we sign Mac Firefox distros.

I'll open new bugs as needed, and report their numbers here.

(By the way, I *haven't* (yet) promised to actually write the Mac implementation, or to finish the work by any deadline.)
Blocks: WebRTC-OpenH264
Blocks: 925570
Apple's "app sandbox" is only supported on OS X 10.7 and up:

https://developer.apple.com/library/mac/releasenotes/macosx/whatsnewinosx/Articles/MacOSX10_7.html

But we still support 10.6, and can't drop support anytime soon (about 25% of our users are still on OS X 10.6.X).  So, if we use the OpenH264 plugin at all on 10.6, we'll need to run it without a sandbox.

What are the implications of running without a sandbox?  Or to put this question another way, is the sandbox just something "nice to have" for the OpenH264 plugin, or is it an absolute requirement to be able to run the plugin securely?

Dan, please feel free to pass on my needinfo if you think that's appropriate.
Flags: needinfo?(dveditz)
(Expanding on comment #2)

Is the OpenH264 plugin no worse a security risk than NPAPI plugins, which we currently don't run in a sandbox?
iPhoto and Mail are examples of sandboxed apps from Apple.  For these (and any sandboxed app) you can get the details of their "entitlements" by using the following command in Terminal:

codesign -d -vvv --entitlements :- /Applications/[appname].app
Just discovered an older "sandbox" technology, supported on OS X 10.5 and up, which (apparently) doesn't require us to change how we sign Firefox (or its components such as plugin-container.app):

http://www.chromium.org/developers/design-documents/sandbox/osx-sandboxing-design

Chrome (apparently) uses this, and so does Safari.  Neither of those apps uses entitlements in signatures, but both use lots of *.sb files scattered in various locations under /System/Library.

Furthermore the Flash plugin recently started using this technology in Safari:
http://blogs.adobe.com/security/2013/10/flash-player-sandbox-now-available-for-safari-on-mac-os-x.html

So this (older) technology isn't obsolete ... though it *is* much less well documented.  That's typical Apple, though -- saying the least about the most powerful (and useful) APIs :-(
From this point I'm going to concentrate on the sandbox technology mentioned in comment #6, and ignore the stuff mentioned in earlier comments.
Apparently we've covered some of this ground before.
See Also: → 387248
Apple's current (10.9) manpage on sandbox_init() says that it's deprecated, and that the "App Sandbox" should be used instead.

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/sandbox_init.3.html

The same is presumably true of the entire sandbox technology I mentioned in comment #6.

But as both Safari and Chrome continue to use this "deprecated" technology instead of the "App Sandbox" (even on OS X 10.9.X), I think that's a load of ... something that doesn't smell very nice :-)
I mentioned above that neither Chrome nor Safari uses Apple's "App Sandbox".  Instead they use an older (and much less well documented) technology described here:

http://www.chromium.org/developers/design-documents/sandbox/osx-sandboxing-design

But they use it in somewhat different ways.  Chrome uses sandbox_init().  But Safari (via WebKit) uses the otherwise undocumented sandbox_init_with_parameters():

extern "C" int sandbox_init_with_parameters(const char *profile, uint64_t flags,
                                            const char *const parameters[],
                                            char **errorbuf);

Both of these sandbox_init methods are implemented in /usr/lib/system/libsystem_sandbox.dylib (a component of libSystem.dylib), and are clearly closely related.  In fact (in libsystem_sandbox.dylib) sandbox_init() just calls sandbox_init_with_parameters().  The only difference seems to be that sandbox_init_with_parameters() can specify string "parameters" -- so it's more flexible (and powerful).

We could use either of these methods.  And I suspect it's wise to leave the decision open, for the time being.

Chrome's calls to sandbox_init() generally happen here:

http://src.chromium.org/viewvc/chrome/trunk/src/content/common/sandbox_mac.mm#l593

WebKit's calls to sandbox_init_with_parameters() generally happen here:

https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/mac/ChildProcessMac.mm#L158
https://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/mac/ChildProcessMac.mm#L170
Do we have a working OpenH264 plugin that we can test with?

If we don't, for the time being we can just play with sandboxing plugins (via plugin-container).  But eventually we'll need the OpenH264 plugin itself.
Flags: needinfo?(mreavy)
Above I concluded that we should use BSD-style sandboxing on the Mac, and not Apple's (relatively) new "App Sandbox".  Let me sum up my reasons in a single comment:

1) BSD-style sandboxing is available on all of our supported versions of OS X, but the App Sandbox is only available on OS X 10.7 and up.

2) The App Sandbox is turned on per-application by how the application is signed.  But BSD-style sandboxing is turned on dynamically, using sandbox_init() or sandbox_init_with_parameters().  If we use the App Sandbox we'll need to create a separate app (presumably a close copy of plugin-container) to run the OpenH264 plugin.  If we use BSD-style sandboxing we can use plugin-container for the OpenH264 plugin, for NPAPI plugins, and for any number of other things.  If we want to sandbox plugin-container we can call sandbox_init() or sandbox_init_with_parameters() with the appropriate parameters, and otherwise not.

Avoiding the App Sandbox also avoids any complications that may arise from having to change our Mac app-signing infrastructure.

3) Because sandbox_init() and sandbox_init_with_parameters() are called dynamically, we can decide when to call them.  All "resources" that were "open" before the call stay accessible.  Since we can arrange that access to certain resources is "grandfathered in", we should be able to make our sandbox rules simpler than they would otherwise be.

4) Though Apple has deprecated BSD-style sandboxing, the fact that Chrome and (especially) Safari/WebKit still use it mean that this is bullshit, and can be ignored.
Here's an interpose library you can use to log (and observe) the parameters of any call to sandbox_init() or sandbox_init_with_parameters() from any process that uses them.  interpose.mm in the library contains instructions for building and use.

This works just fine for Google Chrome (or Chromium).  But Safari is more complicated.  The problem is that the OS prevents the DYLD_INSERT_LIBRARIES environment variable from having any effect on apps that have entitlements in their signatures.  (Safari has a number of non-App-Sandbox entitlements.)

To get around this you need to create a copy of Safari.app, then re-sign it with your own signing certificate.  You need to make this new signature replace all the existing ones, as follows (for example):

sudo codesign -s "Steven Michaud" -f /Applications/SafariAlt.app

For this you need a code signing cert from Apple.

To get one you need an account (i.e. to create an Apple ID), as per:

https://developer.apple.com/register/index.action

Once you have this account, sign in, choose "Certificates, Identities and Profiles", choose Certificates under Mac Apps, then click on the "+" (Plus) button.  You'll be walked through using the Keychain Access utility to create a certificate request, plus all the other necessary steps.

As best I can tell, the only difference this re-signing makes to Safari is that it prompts you more often for access to things like your keychain.
(In reply to Steven Michaud from comment #2)
> What are the implications of running without a sandbox?  Or to put this
> question another way, is the sandbox just something "nice to have" for the
> OpenH264 plugin, or is it an absolute requirement to be able to run the
> plugin securely?

I have no reason to believe OpenH264 plugin is any less secure than an NPAPI plugin, but we are now making all non-Flash plugins click-to-play and Flash has implemented their own sandbox (or will soon if they haven't yet on Mac). In addition we will require a sandbox for the DRM plugin-like thing so one way or another we need to figure out how to make them work on every platform.

OpenH264 is slightly scarier in that we will be downloading it ourselves which makes us more responsible than we are for other plugins which the user installs themselves. In terms of capabilities a video encoder/decoder is less scary than complex things like Flash or Java which have the video encoder/decoder PLUS a whole code execution environment and networking stack.
Flags: needinfo?(dveditz)
(In reply to Steven Michaud from comment #11)
> Do we have a working OpenH264 plugin that we can test with?
> 
> If we don't, for the time being we can just play with sandboxing plugins
> (via plugin-container).  But eventually we'll need the OpenH264 plugin
> itself.

Yes:
- I have put up an initial patch for the WebRTC adapter (bug 999704)
that implements the browser side of the WebRTC integration.

- I pushed a branch of OpenH264 to github that implements the
OpenH264 side of the WebRTC integration
(https://github.com/ekr/openh264/tree/plugin4-merge)
(In reply to Daniel Veditz [:dveditz] from comment #15)
> (In reply to Steven Michaud from comment #2)
> > What are the implications of running without a sandbox?  Or to put this
> > question another way, is the sandbox just something "nice to have" for the
> > OpenH264 plugin, or is it an absolute requirement to be able to run the
> > plugin securely?
> 
> I have no reason to believe OpenH264 plugin is any less secure than an NPAPI
> plugin, but we are now making all non-Flash plugins click-to-play and Flash
> has implemented their own sandbox (or will soon if they haven't yet on Mac).
> In addition we will require a sandbox for the DRM plugin-like thing so one
> way or another we need to figure out how to make them work on every platform.
> 
> OpenH264 is slightly scarier in that we will be downloading it ourselves
> which makes us more responsible than we are for other plugins which the user
> installs themselves. In terms of capabilities a video encoder/decoder is
> less scary than complex things like Flash or Java which have the video
> encoder/decoder PLUS a whole code execution environment and networking stack.

As Dan says, we are going to need a sandbox for EME on Mac, so we might as well
do it now.
Flags: needinfo?(mreavy)
If we use BSD-style sandboxing, we can customize our sandbox rules per-plugin.  Seems like we should take advantage of this to create a set of rules specifically for the OpenH264 plugin.

Eric, do you have any general recommendations what those rules should contain?  For example, can we (in principle) disallow all file system and network access?
Flags: needinfo?(ekr)
For OpenH264 you should be allowing essentially everything that involves
affecting the system outside the process with the exception of the
IPC I/O to the parent process. It's just computation
Flags: needinfo?(ekr)
allowing -> disallowing, I assume :-)

OK, so we should plan to sandbox the OpenH264 plugin as tightly as possible.  The IPC I/O connections to the parent process can presumably be set up before we turn on sandboxing, so that they'll get "grandfathered in".
(In reply to Steven Michaud from comment #20)
> allowing -> disallowing, I assume :-)

Yes.


> OK, so we should plan to sandbox the OpenH264 plugin as tightly as possible.
> The IPC I/O connections to the parent process can presumably be set up
> before we turn on sandboxing, so that they'll get "grandfathered in".

That was my thought as well.
Huh, that didn't come out right. Ignore.


(In reply to Eric Rescorla (:ekr) from comment #21)
> Yes, that was my thought as well.(In reply to Eric Rescorla (:ekr) from
> comment #19)
> > For OpenH264 you should be allowing
> 
> yes essentially everything that involves
> > affecting the system outside the process with the exception of the
> > IPC I/O to the parent process. It's just computation
Do we really want libxul in the sandbox address space?
> Do we really want libxul in the sandbox address space?

I frankly don't know whether access to the address space is controllable -- whether we can limit access to only parts of it.

In any case, though, we'll get all of XUL in the client process if we use plugin-container to run the OpenH264 plugin, as seems to be the current plan.
(In reply to Steven Michaud from comment #25)
> In any case, though, we'll get all of XUL in the client process if we use
> plugin-container to run the OpenH264 plugin, as seems to be the current plan.

I don't see the same model working for EME.
Flags: needinfo?(hsivonen)
(In reply to Mike Hommey [:glandium] from comment #24)
> Do we really want libxul in the sandbox address space?

Other than aesthetic issues, what's wrong with this?
> I frankly don't know whether access to the address space is
> controllable -- whether we can limit access to only parts of it.

As best I can tell, there are no sandbox rules that allow us to do
this.  We *could* use a low-level method (vm_protect()) to change the
access permissions on parts of the address range (before we turn on
the sandbox), and once the sandbox was on vm_protect() would
presumably be unusable.  But, needless to say, this would be difficult
and error prone.
Note that we almost certainly don't have time to use something besides plugin-container to run the OpenH264 plugin, if we want support for sandboxing it to get into the FF33 release.
(In reply to Steven Michaud from comment #28)
> > I frankly don't know whether access to the address space is
> > controllable -- whether we can limit access to only parts of it.
> 
> As best I can tell, there are no sandbox rules that allow us to do
> this.  We *could* use a low-level method (vm_protect()) to change the
> access permissions on parts of the address range (before we turn on
> the sandbox), and once the sandbox was on vm_protect() would
> presumably be unusable.  But, needless to say, this would be difficult
> and error prone.

I'm trying to work through the threat model... What would the objective of
restricting access to the libxul code be?
> What would the objective of restricting access to the libxul code be?

We presumably don't want the OpenH264 plugin (or whatever black box it loads) calling into XUL, or into the system dylibs that XUL depends on (and loads).
(In reply to Steven Michaud from comment #31)
> > What would the objective of restricting access to the libxul code be?
> 
> We presumably don't want the OpenH264 plugin (or whatever black box it
> loads) calling into XUL, or into the system dylibs that XUL depends on (and
> loads).

How does this help? Can you walk me through what you think preventing
that access would stop people from doing.
> How does this help?

I frankly don't know :-)

No system is perfect, and as you get closer to the "edge" it gets harder and harder to make it more perfect.  So for any real system you need to consider not just its level of security, but whether it's worth the trouble to make it more secure than a given level.

Like you, I suspect it's not worth our trouble to prevent XUL from getting loaded into the client process.  But it's worth discussing, and I'm willing to listen to contrary arguments.
(In reply to Steven Michaud from comment #33)
> > How does this help?
> 
> I frankly don't know :-)
> 
> No system is perfect, and as you get closer to the "edge" it gets harder and
> harder to make it more perfect.  So for any real system you need to consider
> not just its level of security, but whether it's worth the trouble to make
> it more secure than a given level.
> 
> Like you, I suspect it's not worth our trouble to prevent XUL from getting
> loaded into the client process.  But it's worth discussing, and I'm willing
> to listen to contrary arguments.


Sorry, I'm trying to make a more general point.

The conventional reason to limit the code surface which is available for
calling is that if you are using w^x, then the attacker can't inject
their own shellcode but has to make use of shellcode that it is already
in the process. So, for instance, if you prevent the attacker from
calling system(), then they can't inject a shell command into the
writable address space and pass the address of that command to system().
One could imagine making a similar argument vis-a-vis XUL in that
it's full of functions that do dangerous stuff if you pass them the
wrong arguments.

Unfortunately for this story, it turns out that you don't need complete
functions, just code fragments, and these are supplied by any binary
of sufficient size [0]. I suspect that just OpenH264 at ~500ish K is
going to be plenty big for that.


[0] http://cseweb.ucsd.edu/~hovav/dist/geometry.pdf
BTW, why not just use the chromium sandbox? We're already using it for b2g and windows.
> the chromium sandbox?

What do you mean?  I'm already planning to use the *kind* of sandboxing that Chrome uses (and also Safari/WebKit).
I mean literally the chromium sandbox code. See security/sandbox.
There's currently no "mac" entry there.  And though I suppose we could add one, BSD-style sandboxing is actually extremely simple to use -- you just decide what rules you want and turn it on (with a call to sandbox_init() or sandbox_init_with_parameters()).

By the way, Windows uses plugin-container.exe when sandboxing Gecko Media Plugins -- see bug 985252 comment #16.  So if Windows does use the "chromium sandbox", it won't make any difference to that.

But I'll keep your suggestion in mind.  That may turn out to be the most convenient way to "wrap" calls to sandbox_init() or sandbox_init_with_parameters().
Note that we'll need to know what kind of plugin is being sandboxed before deciding what rules to use, and I don't know if that's convenient from security/sandbox.
(In reply to Mike Hommey [:glandium] from comment #26)
> (In reply to Steven Michaud from comment #25)
> > In any case, though, we'll get all of XUL in the client process if we use
> > plugin-container to run the OpenH264 plugin, as seems to be the current plan.
> 
> I don't see the same model working for EME.

With EME, we need the CDM host do some work before it drops privileges. I don't know plugin-container well enough to say if this special requirement precludes its use.

In any case, if we want the sandboxing code to be easy to audit for privacy, we should make the CDM host executable minimal instead of throwing all sort of unused code there.
Flags: needinfo?(hsivonen)
I thin(In reply to Henri Sivonen (:hsivonen) from comment #40)
> (In reply to Mike Hommey [:glandium] from comment #26)
> > (In reply to Steven Michaud from comment #25)
> > > In any case, though, we'll get all of XUL in the client process if we use
> > > plugin-container to run the OpenH264 plugin, as seems to be the current plan.
> > 
> > I don't see the same model working for EME.
> 
> With EME, we need the CDM host do some work before it drops privileges. I
> don't know plugin-container well enough to say if this special requirement
> precludes its use.
> 
> In any case, if we want the sandboxing code to be easy to audit for privacy,
> we should make the CDM host executable minimal instead of throwing all sort
> of unused code there.

Hmm. There are two sets of things that a user might be interested in here:

1. What resources the host process has already acquired that might be
a threat to the user. This is partly dependent on how much code is sitting
around, but it's sufficient to audit what code is actually executed, since
it's not subject to control by the CDM.

2. What new resources the CDM could acquire. This is simply a matter of
auditing the sandbox itself, since we assume that all the code running in
the host process is compromised.
Also, the CDM host should be small in size to make it easy for downstream users of the Firefox code base to ship our CDM host. I expect downstream users wouldn't want to ship our libxul in addition to theirs.
For what it's worth, I've now figured out how to get the OpenH264 GMP plugin to load (in the plugin-container process) in current m-c nightlies.  Since I went to some trouble, here's what I did.  Thanks, Robert Strong, for helping me on this.

1) Get the latest NASM source from http://www.nasm.us/ and build and install it.
2) Get the latest OpenH264 source from https://github.com/cisco/openh264.
3) Run the following commands to build OpenH264:
   a) make
   b) make gmp-bootstrap
   c) make plugin
4) Create a gmp-gmpopenh264 directory in /Library/Internet Plug-Ins/
5) Copy the following files to it from the OpenH264 build directory:
   gmpopenh264.info
   libgmpopenh264.dylib
6) On a machine that has a camera, load the following URL:
   http://mozilla.github.io/webrtc-landing/pc_test_h264.html
7) Click on the Start button
   After a few seconds, you will be prompted to share a camera and a microphone.  Do so.

You will find the gmp-gmpopenh264 plugin running in the plugin-container process.
I used an interpose library to log all the calls to any of the following functions exported by the OpenH264 plugin:

GMPInit
GMPGetAPI
GMPShutdown

I hope this will help me and others figure out how we load and use the OpenH264 plugin, in preparation for figuring out how to sandbox it.

Interestingly, GMPShutdown never gets called.  I tested with yesterday's m-c nightly.

(I can post the interpose library I used, if anyone's interested.)
(Following up comment #43)

Oops, I forgot some very important steps:

5a) Start Firefox, visit about:config, create the following boolean setting and set to true:

    media.peerconnection.video.h264_enabled

5b) Quit Firefox and restart it.
Based on our last sandboxing meeting, I believe Andre and Steven are working together to figure out the solution for Mac, and Andre is implementing the solution.
Assignee: nobody → areinald
Attached patch bugzilla-1012949-1.patch (obsolete) — — Splinter Review 
WIP. Though openh264 loads in Nightly, it doesn't in my local build. So this code is not called when debugging. Investigating...
Finally getting around to updating the bug component to security (which is more accurate).
Component: WebRTC → Security
(Following up comment #43)

The patch for bug 1032814 broke these steps.

I understand there are workarounds.  But for the time being I'm testing with builds made with the following revision at tip (which was landed just before the patch for bug 1032814):

http://hg.mozilla.org/mozilla-central/rev/1a4c6cb31743
I've now done a bit of testing (as per comment #49), compiling on different versions of OS X and with different SDKs.  In none of them does André's patch work :-(

The failure is (unfortunately) rather subtle.  The testcase continues running correctly.  But, if you look, you'll notice that the plugin-container process running the OpenH264 plugin has died (very shortly after it was launched).

This doesn't happen (things work as expected) if you change the call to sandbox_init() as follows (which specifies a single "allow everything" rule:

sandbox_init("(allow default)", 0, &errorbuf);

So pretty clearly the plugin-container process is running up against the sandbox and dying.  I don't yet know why.  kSBXProfileNoInternet and kSBXProfileNoWrite also fail, in the same way.

We'll need a lot of trial and error to figure out what's wrong, and what's best to do about it.  I imagine this will take at least a week.
One more thing:

+#ifdef XP_MACOSX
+#include <sandbox.h>
+#endif

You may need to change this as follows to avoid missing symbols errors:

+#ifdef XP_MACOSX
+extern "C" {
+#include <sandbox.h>
+}
+#endif
(Following up comment #50)

Forgot to mention that even when the plugin-container process dies, sandbox_init() does *not* return an error.
(In reply to Steven Michaud from comment #51)
> +#ifdef XP_MACOSX
> +extern "C" {
> +#include <sandbox.h>
> +}
> +#endif

That's not necessary as sandbox.h, starts with __BEGIN_DECLS and ends with __END_DECLS which are defined in sys/cdefs.h exactly as we expect. I'm not even sure how recursive extern "C" behaves.
(In reply to Steven Michaud from comment #50)
> The failure is (unfortunately) rather subtle.  The testcase continues
> running correctly.  But, if you look, you'll notice that the
> plugin-container process running the OpenH264 plugin has died (very shortly
> after it was launched).

That's very surprising : it mean that after the container dies, the main process falls back to reloading the plugin itself so that it keeps working !

> So pretty clearly the plugin-container process is running up against the
> sandbox and dying.  I don't yet know why.  kSBXProfileNoInternet and
> kSBXProfileNoWrite also fail, in the same way.

I can think of the container trying to access locked resources, and failing more or less elegantly because it can't. My own tests showed than an fopen placed immediately after the sandbox_init() call failed, returning NULL as expected. What I didn't expect was that such calls were performed later, failed, killed the container, and there was the above fallback.

> We'll need a lot of trial and error to figure out what's wrong, and what's
> best to do about it.  I imagine this will take at least a week.

Agree : we'll have to find which resources are accessed, by who (container or plugin), decide if that access is legitimate, if so put it in an exception list, and else, patch the calling code.

One week is very optimistic.
(In reply to Steven Michaud from comment #52)
> Forgot to mention that even when the plugin-container process dies,
> sandbox_init() does *not* return an error.

The sandbox initialization works as expected, making the test fopen return NULL. So sandbox_init() *should* *not* return an error.
(In reply to comment #53)

I got the missing symbols error building with the 10.6 SDK (on OS X 10.7).  This hack fixed the problem.  And I didn't get any errors (building with the hack in place) building on OS X 10.9.4 (with no SDK specified, which is more or less equivalent to building with the 10.9 SDK).

So we *could* use the "extern "C" block" hack.  But it'd be better to figure out exactly why it's sometimes needed, and only use it when necessary.  For this we'd need to check which SDK we're compiling with -- which will be easier when we move the Mac sandboxing code to security/sandbox/mac (which doesn't exist yet).

(In reply to comment #55)

I wasn't complaining :-)

I just wanted to make sure people understand that the sandbox is (probably) working as expected.
(In reply to comment #54)

Today I'll try to figure out exactly where the plugin-container process dies.
Bug 1040164 tracks the issue caused by reloading the plugin into the main process when the container crashes.
I just realized you need to run this bug's testcase
(http://mozilla.github.io/webrtc-landing/pc_test_h264.html) maximized.
Otherwise you see only one of its two frames.  Not knowing this has
caused me to waste hours of time.  Here are revised STR, so nobody
else has to suffer the same fate:

1) Current trunk code is incompatible with Cisco's current OpenH264
   plugin.  This is likely to remain true for the next week or so.

   Until this problem is resolved, you need to test (on the Firefox
   side) with the 2014-07-10 m-c nightly, or with a build from a tree
   with the following revision at tip:

   http://hg.mozilla.org/mozilla-central/rev/1a4c6cb31743

   On the Cisco/OpenH264 side, you should (probably) test with code as
   of 2014-07-10.

2) Get the latest NASM source from http://www.nasm.us/ and build and
   install it.

3) Get the latest OpenH264 source from
   https://github.com/cisco/openh264.

4) Run the following commands to build OpenH264:
   a) make
   b) make gmp-bootstrap
   c) make plugin

5) Create a gmp-gmpopenh264 directory in /Library/Internet Plug-Ins/

6) Copy the following files to it from the OpenH264 build directory:
   gmpopenh264.info
   libgmpopenh264.dylib

7) Start Firefox, visit about:config, create the following boolean
   setting and set to true: media.peerconnection.video.h264_enabled

8) Quit Firefox and restart it.

9) On a machine that has a camera, load the following URL and use the
   zoom button to maximize its window:

   http://mozilla.github.io/webrtc-landing/pc_test_h264.html

   You should see two identical "media playback" frames -- one to the
   left and one to the right.

10) Click on the Start button.

   After a few seconds, you will be prompted to share a camera and a
   microphone.  Do so.

   If the test "works" (if you have the OpenH264 plugin running in a
   plugin-container process), you will see camera output in both
   frames -- in the little upper-left box in the right-hand frame, and
   in the bigger box in the left-hand frame.

   If the test "fails" (if you don't have the OpenH264 plugin running
   in a plugin-container process, because it died or never started),
   you will only see camera output in the right-hand frame (in the
   little box).
The openh264 plugin does work with M-C and latest github/cisco/openh264

I don't see in your instructions the setting of MOZ_GMP_PATH which is now required since the landing of https://bugzilla.mozilla.org/show_bug.cgi?id=1032814

Are you doing that?

e.g. - export MOZ_GMP_PATH=/Library/Internet Plug-Ins/gmp-gmpopenh264
The reason I suggested not using the latest code (for either Firefox or the OpenH264 plugin) is that, besides the patch for bug 1032814, I understand there are several patches in the pipeline, each of which will also break the OpenH264 plugin API/ABI.  I don't know how many, and I don't have all the bug numbers.

So I figure it's best to stop updating either tree until the dust has cleared, in the next week or two.
Understood.  The GMP-API changing bug is here - https://bugzilla.mozilla.org/show_bug.cgi?id=1038615
I'm in a meeting right now where Maire kicked it out to FF34.  So we hope to have things for FF33 all in tomorrow and I should be able to tell the Moz build team that the plugin code is ready for FF33 then.
> Today I'll try to figure out exactly where the plugin-container process dies.

By using my interpose library to hook calls to _exit(), I've found that the plugin-container process does return early -- it calls _exit(1).  But this happens in start(), just after main() returns '1'.  Presumably this happens when XRE_InitChildProcess() does an error return, here:

https://hg.mozilla.org/mozilla-central/annotate/f77a9f825427/ipc/app/MozillaRuntimeMain.cpp#l147

But that doesn't tell us much :-(

I'll keep digging tomorrow.
I've made some progress.

We were starting the sandbox too early -- before IPC was fully set up.  But if you postpone starting it until just before receiving the first "message" (from the parent) in GMPChild, the plugin-container process no longer dies.  And if, further, you allow shmem read/writes (using the following rulset), our single testcase actually seems to work!

(version 1)
(deny default)
(allow ipc-posix-shmem)

But things are still a mess.  For example, even with André's original patch, we're starting the sandbox after calling GMPInit in the OpenH264 plugin (which we shouldn't be doing).  And I haven't yet had a chance to test calling this at a different time, after the sandbox has been started.

I'll try to neaten things up next week, and post a working (though still very preliminary) patch.
One extremely annoying "feature" of Apple's ruleset parsing engine that I forgot to mention:

If your ruleset has any syntax mistakes, sandbox_init() will load it without error, and the sandbox will default to "allow everything".

And note that "(version 1)" must appear at the top of all rulesets.  So my following command from comment #50 used bad syntax, and *that's* why it caused the sandbox to allow everything:

sandbox_init("(allow default)", 0, &errorbuf);
I thought I would share the fact that syntax errors ARE reported by sandbox_init(). For instance, the following code:

if (sandbox_init(rules, 0, &errorbuf)) {
  if (errorbuf) {
    printf("Sandbox initialization failed: %s\n", errorbuf);
    sandbox_free_error(errorbuf);
  }
  free((void *)rules);
  return false;
}

Given the following rules:

(version 1)
(debug deny)
(deny default)
(allow ipc-posix-shmem)

Will write:

Sandbox initialization failed: line 4: unbound variable: ipc-posix-shmem

to the terminal.
Attached file plugin-container.crash — 
This is the stack trace of a crashing plugin-container process, when the sandbox is enabled. It's likely caused by a failed attempt to use a resource. But I haven't found yet where the sandbox log was written though my set of rules included a (debug deny) statement. The Console.app reports nothing happening (while it monitors many log files, notably system.log).
That's a WIP again, the rules.sb file is fetched from a fixed path, so I don't have to recompile in order to experiment new rules, just to re-launch Firefox. Also, a mistake in testing for sandbox_init() return value was corrected.
Attachment #8453833 - Attachment is obsolete: true
I'm still working on my own preliminary patch, which starts the sandbox in GMPChild::OnChannelConnected() -- which is called after IPC setup has completed.

But I also misread the error return from sandbox_init(), and was thrown off by that.  Among other things, my comment #65 is wrong:

(Following up comment #65)

sandbox_init() *does* return an error when you use illegal syntax.
I haven't yet figured out where the "(debug deny)" is written, either.  That will probably require some reverse engineering.

But here's another, extremely useful rule:

(trace output_file_name)

This catches every sandbox failure, ignores it, and writes a rule to the output file that would allow the "failed" access.
Attached patch Preliminary patch (obsolete) — — Splinter Review 
Here's the patch I've been working on.

It's not meant to be applied to current trunk.  Instead you should "hg update -r 1a4c6cb31743" on current trunk, then apply the patch.  For best results you should also apply the two patches for bug 1020090 (without them you'll get weird errors caused by running up against the system limit on the number currently open file descriptors).

This patch starts the sandbox after IPC is guaranteed to have been fully set up (in the call to GMPChild::OnChannelConnected()).

It also solves the "plugin initialization code" problem -- it starts the sandbox before the plugin binary gets loaded.  This was surprisingly easy.

My ruleset mostly comes from the output of (trace output_file_name), mentioned in comment #70.  But I also borrowed some lines from Chromium code.  Further testing may require us to add a few more (or to modify some existing ones), but I think this ruleset is already largely complete.  It's *much* simpler than it would be if we didn't wait until IPC is fully set up before starting the sandbox.

More work remains to be done.  In particular I'm unhappy with having to call MOZ_CRASH if the plugin fails to load.  But I haven't yet been able to find a better solution.
This is the log produced by a "deny all" ruleset, when the sandbox is started as in my last patch (ie after the plugin is loaded). A few seconds later, the plugin-container crashes.
(In reply to Steven Michaud from comment #70)
> I haven't yet figured out where the "(debug deny)" is written, either.  That
> will probably require some reverse engineering.

It's written in /var/log/system.log
But there is another trick I not exactly sure about: the place where (debug deny) is written matters. So I had a hard time figuring out the correct order of statements. Plus I expected a (debug allow) to work, but it doesn't.

So your (trace output_file_name) is more useful.
(In reply to Steven Michaud from comment #71)
> Created attachment 8461302 [details] [diff] [review]
> Preliminary patch
> 
> This patch starts the sandbox after IPC is guaranteed to have been fully set
> up (in the call to GMPChild::OnChannelConnected()).
> 
> It also solves the "plugin initialization code" problem -- it starts the
> sandbox before the plugin binary gets loaded.  This was surprisingly easy.

I think your approach is way better than mine for the above reason.

Though sandboxing the container process before the library is loaded may not be useful in the particular case of openh264 because it's open source, it's definitely preferable in the general case.
Here's my preliminary patch from comment #71 updated to current trunk.

And here's a tryserver build I just started (no tests yet):
https://tbpl.mozilla.org/?tree=Try&rev=8f5e12bf407a

Testing it has gotten a lot simpler:

1) The machine you test on must have a camera.  On it load the
   following URL and use the zoom button to maximize its window:

   http://mozilla.github.io/webrtc-landing/pc_test_h264.html

   You should see two identical "media playback" frames -- one to the
   left and one to the right.

2) Click on the Start button.

   After a few seconds, you will be prompted to share a camera and a
   microphone.  Do so.

   If the test "works" (if you have the OpenH264 plugin running in a
   plugin-container process), you will see camera output in both
   frames -- in the little upper-left box in the right-hand frame, and
   in the bigger box in the left-hand frame.

   If the test "fails" (if you don't have the OpenH264 plugin running
   in a plugin-container process, because it died or never started),
   you will only see camera output in the right-hand frame (in the
   little box).

We still have only the one testcase.  I updated Josh's patch from bug
957928 comment #32 to current trunk, but failed to get it working.  In
principle this would (I think) allow us to test the OpenH264 plugin
with any HTML5 video.

I also understand Chris Pearce is working on testcases that will use a
different GMP plugin -- https://github.com/cpearce/gmp-clearkey.  I
hope to learn more about this later.
Attachment #8461302 - Attachment is obsolete: true
> I also understand Chris Pearce is working on testcases that will use
> a different GMP plugin -- https://github.com/cpearce/gmp-clearkey.
> I hope to learn more about this later.

The work for this is being done at bug 1044742.
Attached patch Preliminary patch rev1 (obsolete) — — Splinter Review 
Here's a revision of my preliminary patch that finally builds on the tryserver :-)  I had to work around a problem with the /usr/include/sandbox.h file on our build servers (there's something wrong with it, on which I'll open a bug tomorrow).  I also had to fiddle with some of the escape characters in the large 'rules' string.  (It compiled fine locally, but not on the tryserver.)

Now I've started another set of tryserver builds, this time with tests!  Let's see how we do:
https://tbpl.mozilla.org/?tree=Try&rev=05fcac80114c
Attachment #8465183 - Attachment is obsolete: true
Attached patch Preliminary patch rev2 (obsolete) — — Splinter Review 
Yet another revision.  Sigh.

And another set of tryserver builds:
https://tbpl.mozilla.org/?tree=Try&rev=ac9de9df25d9
Attachment #8465928 - Attachment is obsolete: true
The tryserver tests went pretty well.  There's only one test that consistently fails, and then only on OS X 10.6:

/tests/dom/media/tests/mochitest/test_peerConnection_basicH264Video.html

I'll try to figure out what's going on.
That test was added by bug 1040346 for the "GMP fake plugin".  So I guess I need to ensure that the sandbox rules work with that plugin, in addition to the OpenH264 plugin.
> So I guess I need to ensure that the sandbox rules work with that plugin, in addition to the
> OpenH264 plugin.

Actually no.

Even the http://mozilla.github.io/webrtc-landing/pc_test_h264.html testcase crashes the GMP plugin when run on OS X 10.6 (with my current patch).  I need to figure out why *that* happens.
(Following up comment #81)

Turns out some of the rules in our current set have unrecognized syntax on OS X 10.6.  There's also one rule that needs to be present when we're running on 10.6 and not on other versions of OS X.

So we'll have to use different rulesets on 10.6 and other versions.

It's messy and inconvenient to do this kind of version checking in cross-platform code (like GMPChild.cpp/h).  So next week I'll move the Mac-specific sandbox code to security/sandbox/mac -- where we can call it from GMPChild.cpp/h.
One thing to consider: We could land a first rev of Mac sandboxing that doesn't support 10.6, especially if it meant getting sandboxing for more recent revs (10.7+) sooner.
It will make a difference of at most a day or two.
To be applied last over patch 8465944.
Replaces 8467161.
Attachment #8467161 - Attachment is obsolete: true
(In reply to André Reinald from comment #85)
> Created attachment 8467157 [details]
> This is the set of sandbox rules I've come up with till now. Based on lsof
> and (debug deny) rule output. Hoping to tighten it even more.

The rules in patch 8465944 seem to be tailored for being set before even the plugin-container is started.

As we designed the sandboxing of the Mac plugin-container, the process itself is already loaded and started, all linked libs are loaded, and the ipc channel is already opened (which involves sem and shm). So we can remove the corresponding (allow) rules.
Attached patch Patch rev3 — — Splinter Review 
Here's my next revision.

I moved the Mac-specific code to security/sandbox/mac and introduced some version-specific customizations to the sandbox ruleset.

I'm using Andre's rules, not the ones I wrote originally.  They work, and weren't hard to adapt to use on OS X 10.6.  They're also neater, and I like them better.

But I'd really like to know how you got them, Andre.  Clearly you didn't just use (trace "trace.sb"), as I did.  Please do write up your work on them in some detail in a comment here.  It will give a better understanding of how these rules work on OS X.

I'd have started another set of tryserver builds, but they're down :-(
Attachment #8465944 - Attachment is obsolete: true
(In reply to Steven Michaud from comment #89)
> But I'd really like to know how you got them, Andre.

Comments 72 and 73 indicate how I added the (allow) rules.
It's based on (debug deny) and grep /var/log/system.log

For the removals it's just trial and error.
A summary of document at http://blog.0x0lab.org/2010/03/macosx-sandbox/
This still needs changes.  For example I need to update it with Andre's new ruleset (after testing that on all platforms).  But we're getting close.  The mochitest that failed on 10.6 last time has passed.

Tryserver run:
https://tbpl.mozilla.org/?tree=Try&rev=aeb1c1bea6c9
Attached patch Patch rev5 (obsolete) — — Splinter Review 
Andre:  I found that the 10.6 sandbox appeared not to read your (allow mach-lookup ...) and (allow file-read* ...) rules correctly, so I went back to the old syntax (from your previous ruleset).  I also found that dropping the (allow iokit-open ...) rule appears to cause no trouble anywhere, so I went ahead with this.

Randell:  I'm specifically asking you to review the changes to GMPChild.cpp/h.  But feel free to also comment on the rest of the patch.

The tryservers are down.  I'll start another build when then come back up.
Attachment #8469307 - Attachment is obsolete: true
Attachment #8469477 - Flags: review?(rjesup)
Attachment #8469477 - Flags: review?(areinald)
Comment on attachment 8469477 [details] [diff] [review]
Patch rev5

Andre:  I also dropped your (debug deny) rule.  I assume you didn't mean to leave it in.
Comment on attachment 8469477 [details] [diff] [review]
Patch rev5

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

::: content/media/gmp/GMPChild.cpp
@@ +31,5 @@
> +  #if defined(XP_LINUX)
> +  #include "mozilla/Sandbox.h"
> +  #elif defined(XP_MACOSX)
> +  #include "mozilla/Sandbox.h"
> +  #endif

nit: standard style here is # aligned on left edge, though I might prefer this style myself

@@ +60,5 @@
> +
> +  nsAutoCString pluginBinaryPath(mPluginPath.c_str());
> +  nsAutoCString leaf(Substring(pluginBinaryPath,
> +                               pluginBinaryPath.RFindChar('/') + 5,
> +                               pluginBinaryPath.Length()));

don't we have better ways?  (though perhaps not from the child).

From GMPService.cpp/GMPParent.cpp: (snipped)
  nsresult rv = NS_NewLocalFile(aDirectory, false, getter_AddRefs(directory));

  nsresult rv = aPluginDir->GetLeafName(leafname);
  MOZ_ASSERT(leafname.Length() > 4);
  mName = Substring(leafname, 4);

::: security/sandbox/mac/moz.build
@@ +8,5 @@
> +    'Sandbox.h',
> +]
> +
> +SOURCES += [
> +    'Sandbox.mm',

need a build peer at least in theory (moz.configs and configure.in)
Attachment #8469477 - Flags: review?(rjesup) → review+
How's this, Randell?
Attachment #8469477 - Attachment is obsolete: true
Attachment #8469477 - Flags: review?(areinald)
Attachment #8469562 - Flags: review?(rjesup)
Comment on attachment 8469562 [details] [diff] [review]
Patch rev6 (respond to jesup's comments)

Ted, I'm asking you to review the build system changes.  Feel free to pass the review along if you think that's appropriate.
Attachment #8469562 - Flags: review?(ted)
Attachment #8469562 - Flags: review?(areinald)
Attachment #8469562 - Flags: review?(rjesup) → review+
Comment on attachment 8469562 [details] [diff] [review]
Patch rev6 (respond to jesup's comments)

Snuck in another set of tryserver builds:
https://tbpl.mozilla.org/?tree=Try&rev=85ff83372184
Comment on attachment 8469562 [details] [diff] [review]
Patch rev6 (respond to jesup's comments)

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

::: content/media/gmp/GMPChild.cpp
@@ +32,3 @@
>  #include "mozilla/Sandbox.h"
> +#elif defined(XP_MACOSX)
> +#include "mozilla/Sandbox.h"

Any reason not to combine this in the other #if?

::: security/sandbox/moz.build
@@ +8,5 @@
>  
>      DIRS += ['linux']
>  
> +elif CONFIG['OS_ARCH'] == 'Darwin':
> +

The blank line here and above are weird, can you get rid of them?
Attachment #8469562 - Flags: review?(ted) → review+
> Any reason not to combine this in the other #if?

Not really.  I'll do that when I land the patch.

> The blank line here and above are weird, can you get rid of them?

I just copied the formatting of the existing "if CONFIG['OS_ARCH'] == 'Linux':" section.  I suppose I could get rid of the blank lines in both sections.
Comment on attachment 8469562 [details] [diff] [review]
Patch rev6 (respond to jesup's comments)

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

::: security/sandbox/mac/Sandbox.mm
@@ +35,5 @@
> +  "    (regex #\"^/(private/)?var($|/)\")\n"
> +  "    (regex #\"\\.app/Contents/MacOS/plugin-container\\.app/Contents/\")\n"
> +  "    (literal \"/usr/share/icu/icudt51l.dat\")\n"
> +  "    (literal \"%s\"))\n";
> +

I'd rather specify the path to the current plugin-container Contents/ directory than use a partial path as I did in last regex.
That would involve an additional %s I guess. We can think of it as an improvement, and make a follow-up bug.

I also wonder about the overlapping with line 25 (allow file-read-data (literal "pluginPath")),
and line 38 (allow file-read* (literal "pluginBinaryPath")).
(In reply to André Reinald from comment #103)
> I also wonder about the overlapping with line 25 (allow file-read-data
> (literal "pluginPath")), and line 38 (allow file-read* (literal "pluginBinaryPath")).

If I can grab a working 10.6 VM image (I didn't manage to set up one myself), I can tweak the rules further (and make sure they work on 10.6), as it seems 10.6 has major differences in the rules interpreter, and the needs to access files.
Andre, let's save further changes to the sandbox rules for later.  I need to get this patch landed today :-)

As soon as I get back from my vacation, I'll find out how to get the full path to the plugin-container binary.

> I also wonder about the overlapping with line 25 (allow file-read-data
> (literal "pluginPath")), and line 38 (allow file-read* (literal "pluginBinaryPath")).

This just seems to be something that the 10.6 sandbox wants.  I don't see any harm in it.
Attachment #8469562 - Flags: review?(areinald) → review+
Comment on attachment 8469562 [details] [diff] [review]
Patch rev6 (respond to jesup's comments)

Landed directly on mozilla-central (since mozilla-inbound seems to be down for a while):
https://hg.mozilla.org/mozilla-central/rev/5e74101ce59e

Will watch the tree, but for now I'm marking this fixed.
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox34: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment on attachment 8469562 [details] [diff] [review]
Patch rev6 (respond to jesup's comments)

Most of the tests have run by now, and there've been no non-spurious test failures.

But note that I'll be gone next week, and won't be around until the week after that to fix problems that might show up.  So if something happens that isn't easy to fix, please just back this patch out and I'll deal with it when I come back.
Depends on: 1054965
Whiteboard: aurora uplift note: take bug 1054965 with this one
Whiteboard: aurora uplift note: take bug 1054965 with this one → aurora uplift note: take bug 1054965 with this one [openh264-uplift?]
We'll also need to uplift (at least) the following:

1) The Mac-specific parts of bug 1052033.
2) Bug 1056419
3) Bug 1056936 (once I'm done with it).
Revised list of things to uplift to 33 (provisional):

1) This bug's patch plus the Mac-specific parts of bug 1052033.
2) Bug 1040905
3) Bug 1054965
4) Bug 1055308
5) Bug 1056936
> Revised list of things to uplift to 33 (provisional):

I've now tested these all together on the aurora branch -- no problems.  So this list is final -- this is what we need to uplift to the 33 branch to implement GMP/OpenH264 on the Mac.
Whiteboard: aurora uplift note: take bug 1054965 with this one [openh264-uplift?] → aurora uplift note: see comment #110 [openh264-uplift?]
This includes the Mac-specific parts of the patch for bug 1052033.

It should be landed together with the other patches listed in comment #110.
Comment on attachment 8480791 [details] [diff] [review]
Patch for uplift to aurora

Approval Request Comment
[Feature/regressing bug #]: New feature
[User impact if declined]: FF 33 won't have Mac GMP sandboxing
[Describe test coverage new/current, TBPL]: Baking on m-c for several weeks with no unfixed problems.  Separate tests (with other patches from comment #110) done by me on aurora branch -- again no problems.
[Risks and why]: Moderate risk, but no more so than any other new feature.
[String/UUID change made/needed]: None
Attachment #8480791 - Flags: approval-mozilla-aurora?
Attachment #8480791 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Steven, is it worth adding that to the release notes?
Flags: needinfo?(smichaud)
> Steven, is it worth adding that to the release notes?

What release notes are you talking about?  Those for Firefox 33?

There should probably be something about adding support for the GMP plugin, OpenH264 and sandboxing on all platforms, but I'm not the best person to decide what the text should be.
Flags: needinfo?(smichaud)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: