1256065 - crash in mozilla::GMPVideoDecoder::GMPInitDone

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is 
report bp-5597f645-ecac-4afb-a2c3-2160e2160312.
=============================================================

Currently at #6 in Top Crashers for Firefox 46.0b.

Potentially exploitable:
Crash Reason 	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address 	0xffffffffe5e5e5e5

All crashes are on Windows:
Operating System	Percentage
Windows 7	53.66%
Windows XP	39.02%
Windows Vista	4.07%
Windows 10	1.63%
Windows 8.1	1.63%

Seems to have started in v46:
Product	Version	Percentage
Firefox	47.0a1	58.54%
Firefox	46.0a2	18.70%
Firefox	48.0a1	15.45%
Firefox	45.0a2	3.25%
Firefox	47.0a2	2.44%
Firefox	45.0a1	0.81%
Firefox	46.0b1	0.81%

Stack:
mozilla::GMPVideoDecoder::GMPInitDone(GMPVideoDecoderProxy*, GMPVideoHost*)
mozilla::gmp::GetGMPContentParentForVideoDecoderDone::Done(mozilla::gmp::GMPContentParent*)
mozilla::gmp::RunCreateContentParentCallbacks::Run()
nsThread::ProcessNextEvent(bool, bool*)
NS_ProcessNextEvent(nsIThread*, bool)
mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)
MessageLoop::RunHandler()
MessageLoop::Run()
nsThread::ThreadFunc(void*)
_PR_NativeRunThread

Comment 1

[Mats Palmgren (inactive)]

Note that an older bug 1203763 with the same signature resolved as WFM might be related.

Comment 2

[Robert Kaiser]

[Tracking Requested - why for this release]:
This is 2.7% of all Firefox 46.0b1 crashes, and therefore by far a topcrash.

Comment 3

[Maire Reavy [:mreavy]]

This has the same signature as Bug 1203763.  This is not OpenH264; it's playback (EME). Chris -- This is marked as a regression, blocking Fx46 release and a top crasher. Can you take it and drive this to a fix?  Or find someone on your team to drive this to a fix?  Thanks.

Comment 4

[Maire Reavy [:mreavy]]

Chris -- FYI: Since this *may* be related to bug 1256064 (and it's also a regression that's blocking release), I made you the owner for both bugs to drive these forward.  Thanks.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Sec-critical, recent regression, tracking

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

#3 topcrash in beta 46 so far.  I agree, being the very top crashes and sec-critical should probably make this a release blocker.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

There are two problems here, caused by races in shutdown.

The first problem here is that GMP{Audio,Video}Decoder::mInitPromise is set to null in GMP{Audio,Video}Decoder::Shutdown() after it's rejected. So if the GMP decoder is shutdown after Init() is called, but before GMPInitDone() is called, mInitPromise can be null, and that's how we get the null pointer deref on mInitPromise.

The other case we see is the UAF in GMP{Audio,Video}Decoder::mConfig in GMPInitDone. This is because mConfig is a const reference to a struct probably held by the MediaFormatReader or demuxer, which must have been freed. We can just make a copy of the config.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #7)
> There are two problems here, caused by races in shutdown.

Note: I mean shutdown of the decoder, not shutdown of Firefox.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

* Check whether GMP{Audio,Video}Decoder::mInitPromise is empty and bail if so in GMPInitDone. This prevents null pointer deref.
* Make a copy of the MediaInfo passed to GMP{Audio,Video}Decoder, rather than keeping a reference to it. This means we can't get a UAF if the thing that owns the MediaInfo is destroyed before the GMP decoder is. That can happen because the callback object maintains a reference to the GMP decoder while we wait for a GMPInitDone callback.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8730519 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I think it could be hardish. Attacker would need to construct an MP4 file with valid audio/video stream metadata that populates the MediaInfo/AudioInfo/VideoInfo structs to create valid executable code, and then if unencrypted decoding using GeckoMediaPlugins is enabled, play the video inside a regular non-EME <video> element and spam shutting down and starting up.

Unencrypted decoding is enabled in Firefox 46 and later.

In theory an attacker could load this video inside an EME <video> as well, though we have no crash reports that I can find proving this is possible. We only have crash reports for the unencrypted video case AFAICT. EME is enabled in Firefox 42 and later.


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I think a skilled attacker could work backwards from the class/struct names to figure out what the flaw was. Figuring out that they could exploit this using unencrypted decoding and/or ClearKey EME would be a bit of a leap.


Which older supported branches are affected by this flaw?

46 is affected in the unencrypted video decoding case. 45 and ESR45 in theory are affected in the EME video case, though we've not seen any crash reports in the EME case.


If not all supported branches, which bug introduced the flaw?
N/A.


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Not yet. We need Release 45. ESR45, and Aurora, Beta, Central. Should be easy enough.


How likely is this patch to cause regressions; how much testing does it need?

Not likely to cause regressions. Basically adding null-check type guards. I'm not 100% sure I fixed all the cases, since I can't repro, so testing this not-too-late in the beta cycle would be good.

We can also land this in inconspicuously in bug 1256064 to make it less visible that it's an exploitable bug.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Please make and nominate branch patches as well.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49aa9281700cdacc521a7fa851abfc019831f1eb
Bug 1256065. r=gerald

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Remember to request uplift.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49aa9281700c

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8730519 [details] [diff] [review]
Patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-crit.
Fix Landed on Version: 48, mozilla-central.
Risk to taking this patch (and alternatives if risky): Fairly low, basically adding (if shutdown, bail) checks.
String or UUID changes made by this patch: None.


See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: Unencrypted decoding via Adobe GMP, and EME playback
[User impact if declined]: Crashes when using unencrypted decoding via Adobe EME plugin, and potentially if using regular EME.
[Describe test coverage new/current, TreeHerder]: Test included, and we have lots of EME mochitest, and specific test for unencrypted decoding via GMP.
[Risks and why]: Fairly low, basically adding (if shutdown, bail) checks.
[String/UUID change made/needed]: None.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730519 [details] [diff] [review]
Patch

Sec-crit, Aurora47+, Beta46+, ESR45.1.0+

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4a154a87655
https://hg.mozilla.org/releases/mozilla-beta/rev/48ca64d2b5b1
https://hg.mozilla.org/releases/mozilla-esr45/rev/ce0f864c5ecd

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

No crashes reported in builds built roughly after when this landed. Looking good so far...