Closed Bug 1276420 Opened 3 years ago Closed 3 years ago

Widevine plugin crashing on OS X due to -stdlib=libc++ and sandboxing interaction

Categories

(Core :: Audio/Video: Playback, defect, P1)

49 Branch
Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- fixed

People

(Reporter: haik, Assigned: haik)

References

Details

Attachments

(1 file, 2 obsolete files)

When trying to watching a streaming Amazon.com video using the Widevine plugin in Nightly on OS X 10.11, I'm hitting plugin-container crashes. This appears to be due to the fix for 1246743. With that fix backed out and Widevine enabled via "ac_add_options --enable-eme=widevine", I couldn't reproduce the problem.

The error displayed on Amazon.com (for now) is "Digital Rights Error" -- "Your web browser is missing a digital rights component. For further assistance, please contact Amazon Customer Service at www.amazon.com/videohelp and refer to error 7235."

The console app shows that the plugin sandbox is denying the load of /usr/lib/libstdc++.6.dylib which is a symlink to libstdc++.6.0.9.dylib. The stack is

  dyld::load()
  dyld::libraryLocator()
  ImageLoader::recursiveLoadLibraries()
  ImageLoader::link()
  dyld::link()
  dlopen()
  PR_LoadLibraryWithFlags()
  mozilla::gmp::GMPLoaderImpl::Load()
  mozilla::gmp::GMPChild::AnswerStartPlugin()
  mozilla::gmp::PGMPChild::OnCallReceived()
  mozilla::ipc::MessageChannel::DispatchInterruptMessage()
  mozilla::ipc::MessageChannel::DispatchMessage()
  mozilla::ipc::MessageChannel::OnMaybeDequeueOne()
  nsRunnableMethodImpl()
  mozilla::ipc::MessageChannel::DequeueTask::Run()
  MessageLoop::DeferOrRunPendingTask()
  MessageLoop::DoWork()
  base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
  MessageLoop::Run()
  XRE_InitChildProcess()
  content_process_main()
  start()

The attached patch adds a rule to the plugin sandbox allowing the library to be read. This fix needs a bit more analysis. Mainly I'm wondering if, now that we're dynamically linking libstdc++, there might be libstdc++ dependencies we should also allow, or if we should just allow all of /usr/lib.
Assignee: nobody → haftandilian
See Also: → 1246743
Version: unspecified → 49 Branch
This is sort of weird, because we should have been linking to libstdc++*.dylib before.  What library is that PR_LoadLibraryWithFlags call trying to load with and without the patch to bug 1246743?

Oh, here's a plausible scenario: the PR_LoadLibraryWithFlags call was always trying to load libstdc++, but before, libxul linked libstdc++, so we didn't have to do anything when a load was requested: the library had already been loaded into the address space.  With libc++ enabled, we're not linking libstdc++, so it's treated as a separate library.

Do we control whatever's trying to load libstdc++ dynamically?  I poked around a little bit in GMPChild, but I couldn't answer that question.  If so, could we make it load libc++ instead?
Flags: needinfo?(haftandilian)
(In reply to Nathan Froyd [:froydnj] from comment #1)
> This is sort of weird, because we should have been linking to
> libstdc++*.dylib before.  What library is that PR_LoadLibraryWithFlags call
> trying to load with and without the patch to bug 1246743?

This will be the Widevine CDM. This is in $FirefoxProfiledir/gmp-widevinecdm/1.4.8.*/libwidevinecdm.dylib.

> 
> Oh, here's a plausible scenario: the PR_LoadLibraryWithFlags call was always
> trying to load libstdc++, but before, libxul linked libstdc++, so we didn't
> have to do anything when a load was requested: the library had already been
> loaded into the address space.  With libc++ enabled, we're not linking
> libstdc++, so it's treated as a separate library.
> 
> Do we control whatever's trying to load libstdc++ dynamically?

libwidevinecdm.dylib is a dylib produced by Google. So no, we don't control it directly.

> I poked
> around a little bit in GMPChild, but I couldn't answer that question.  If
> so, could we make it load libc++ instead?

We could make GMPChild pre-load libstdc++ before starting the sandbox. It's simpler (in terms of lines of code) to just allow the load of libstdc++ in the sandbox rules IMO.
(In reply to Chris Pearce (:cpearce) from comment #2)
> (In reply to Nathan Froyd [:froydnj] from comment #1)
> > This is sort of weird, because we should have been linking to
> > libstdc++*.dylib before.  What library is that PR_LoadLibraryWithFlags call
> > trying to load with and without the patch to bug 1246743?
> 
> This will be the Widevine CDM. This is in
> $FirefoxProfiledir/gmp-widevinecdm/1.4.8.*/libwidevinecdm.dylib.

I tested this and, yes, that is what is being loaded.

> > Oh, here's a plausible scenario: the PR_LoadLibraryWithFlags call was always
> > trying to load libstdc++, but before, libxul linked libstdc++, so we didn't
> > have to do anything when a load was requested: the library had already been
> > loaded into the address space.  With libc++ enabled, we're not linking
> > libstdc++, so it's treated as a separate library.

Yeah, I agree.

Just for completeness, here's the OS X otool -L output (ldd equivalent) on the widevine dylib showing it loading libstdc++

  $ otool -L 1.4.8.893/libwidevinecdm.dylib 
  1.4.8.893/libwidevinecdm.dylib:
	@loader_path/libwidevinecdm.dylib
            (compatibility version 0.0.0, current version 0.0.0)
	/System/Library/Frameworks/ApplicationServices.framework/Versions/A/ApplicationServices
            (compatibility version 1.0.0, current version 45.0.0)
	/usr/lib/libstdc++.6.dylib (compatibility version 7.0.0, current version 56.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 169.3.0)

and here's an excerpt from dlopen(3) on OS X 10.11,

  If the file ... has not already been loaded
  into the current process, it is loaded and
  linked.  After being linked, if it contains any
  initializer functions, they are called, before
  dlopen() returns.

> > I poked
> > around a little bit in GMPChild, but I couldn't answer that question.  If
> > so, could we make it load libc++ instead?
> 
> We could make GMPChild pre-load libstdc++ before starting the sandbox. It's
> simpler (in terms of lines of code) to just allow the load of libstdc++ in
> the sandbox rules IMO.

How will updates to the widevine binary be handled? i.e., how will we qualify a new version before it gets picked up by Firefox release?

White listing widevinecdm.dylib dependencies (per the patch) or preloading them ourselves is the most conservative thing to do, but means new versions of the widevine dylib that have new dylib dependencies would break us again.

If we load widevinecdm.dylib before the sandbox rules are applied, we should be protected from sandboxing load issues with new dependencies needed by the widevine dylib in the future. However we have to trust the widevine dylib to not link with / execute anything problematic security-wise before the sandbox rules are applied.
Flags: needinfo?(haftandilian) → needinfo?(cpearce)
(In reply to Haik Aftandilian [:haik] from comment #3)
> How will updates to the widevine binary be handled? i.e., how will we
> qualify a new version before it gets picked up by Firefox release?

We test CDM updates locally on important sites that are known to use it, and then push them out to a pre-release version of Firefox only. Then we let that version of Firefox ride the trains, and the new CDM rides along with it. So we should have bake time on Beta for new CDM updates, and we test it locally before pushing it out.


> White listing widevinecdm.dylib dependencies (per the patch) or preloading
> them ourselves is the most conservative thing to do, but means new versions
> of the widevine dylib that have new dylib dependencies would break us again.
> 
> If we load widevinecdm.dylib before the sandbox rules are applied, we should
> be protected from sandboxing load issues with new dependencies needed by the
> widevine dylib in the future. However we have to trust the widevine dylib to
> not link with / execute anything problematic security-wise before the
> sandbox rules are applied.

We've previously tried hard to ensure that the sandbox has lowered privileges before running any code from the GMP binaries. I think we should try hard to maintain that, as the binaries aren't produced by us, and their code isn't inspectable by us. Given that we have the opportunity to test them before pushing them out, I think we can accept the overhead of maintaining the list of whitelisted dependencies to preload.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #4)
> We've previously tried hard to ensure that the sandbox has lowered
> privileges before running any code from the GMP binaries. I think we should
> try hard to maintain that, as the binaries aren't produced by us, and their
> code isn't inspectable by us. Given that we have the opportunity to test
> them before pushing them out, I think we can accept the overhead of
> maintaining the list of whitelisted dependencies to preload.

I'll get this patch reviewed and pushed to Nightly.

I've added the rule to the pluginSandboxRules list versus the Widevine-specific addendum rules because bug 1246743 will have affected all sandboxes and if libstdc++ was previously always linked it, it shouldn't be any less secure to allow it to be dlopen'd for other plugins.

The content sandbox already permits reads from /usr/lib.
Added correct regex to the pluginSandboxRules to match and allow /usr/lib/libstdc++.*dylib. We have to use \\+ to match a '+' and \\. to match a '.' because these are special regex characters. My intent with the regex is match files with names

  /usr/lib/libstdc++.(followed by anything that ends in "dylib")

Using a regex here because the filenames change when new versions of the dylib get installed because they include the version number. This matches the symlink and the minor version numbered dylib filenames. For example, libstdc++.6.dylib and libstdc++.6.0.9.dylib.

Tested on Nightly on OS X 10.11 watching videos on Amazon streaming use the Widevine plugin.

Try run:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e06e2f02d2bd
Attachment #8757609 - Attachment is obsolete: true
Attachment #8758492 - Flags: review?(gpascutto)
Attachment #8758492 - Flags: review?(gpascutto) → review+
Summary: Widevine plugin crashing on OS X 10.11 due to -stdlib=libc++ and sandboxing interaction → Widevine plugin crashing on OS X due to -stdlib=libc++ and sandboxing interaction
Keywords: checkin-needed
See Also: 1246743
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09dcec68c16f
Widevine plugin crashing on OS X due to -stdlib=libc++ and sandboxing interaction; r=gcp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/09dcec68c16f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.