crash in mozilla::GMPVideoDecoder::GMPInitDone

RESOLVED FIXED in Firefox 46

Status

()

defect
P1
critical
Rank:
5
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: cpearce)

Tracking

(5 keywords)

46 Branch
mozilla48
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 wontfix, firefox46blocking fixed, firefox47+ fixed, firefox48+ fixed, firefox-esr38 unaffected, firefox-esr4546+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main46+][adv-esr45.1+], crash signature)

Attachments

(1 attachment)

Reporter

Description

3 years ago
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
Reporter

Comment 1

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

Comment 2

3 years ago
[Tracking Requested - why for this release]:
This is 2.7% of all Firefox 46.0b1 crashes, and therefore by far a topcrash.
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.
Assignee: nobody → cpearce
See Also: → 1203763
Rank: 5
Priority: -- → P1
Group: media-core-security
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.

Updated

3 years ago
Keywords: topcrash-win
Sec-critical, recent regression, tracking
#3 topcrash in beta 46 so far.  I agree, being the very top crashes and sec-critical should probably make this a release blocker.
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.
(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.
Posted patch PatchSplinter Review
* 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.
Attachment #8730519 - Flags: review?(gsquelart)
Attachment #8730519 - Flags: review?(gsquelart) → review+
Duplicate of this bug: 1256523
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.
Attachment #8730519 - Flags: sec-approval?
Group: core-security
sec-approval+ for trunk. Please make and nominate branch patches as well.
Attachment #8730519 - Flags: sec-approval? → sec-approval+
Remember to request uplift.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/49aa9281700c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.
Flags: needinfo?(cpearce)
Attachment #8730519 - Flags: approval-mozilla-esr45?
Attachment #8730519 - Flags: approval-mozilla-beta?
Attachment #8730519 - Flags: approval-mozilla-aurora?
Comment on attachment 8730519 [details] [diff] [review]
Patch

Sec-crit, Aurora47+, Beta46+, ESR45.1.0+
Attachment #8730519 - Flags: approval-mozilla-esr45?
Attachment #8730519 - Flags: approval-mozilla-esr45+
Attachment #8730519 - Flags: approval-mozilla-beta?
Attachment #8730519 - Flags: approval-mozilla-beta+
Attachment #8730519 - Flags: approval-mozilla-aurora?
Attachment #8730519 - Flags: approval-mozilla-aurora+
No crashes reported in builds built roughly after when this landed. Looking good so far...
Group: media-core-security → core-security-release
Assignee

Updated

3 years ago
Duplicate of this bug: 1258604
Assignee

Updated

3 years ago
Blocks: 1259397
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+][adv-esr45.1+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.