Crash in mozilla::MediaDecoder::NotifySuspendedStatusChanged

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jesup, Assigned: jwwang)

Tracking

({crash, csectype-uaf, sec-high})

45 Branch
mozilla54
x86
Windows
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox51 unaffected, firefox52+ fixed, firefox53+ fixed, firefox54+ fixed)

Details

(Whiteboard: [post-critsmash-triage], crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is 
report bp-5d936421-6b13-4d72-bc77-47ae42170124.
=============================================================

Note: this is not the same as bug 1312994 or bug 1311904

This is a wild/UAF ptr (probably mOwner, or maybe this) in MediaDecoder::NotifySuspendedStatusChanged()

Variations on this (calling stack varies) go back to at least 37(!). See https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3AMediaDecoder%3A%3ANotifySuspendedStatusChanged&date=%3E%3D2017-01-17T21%3A56%3A00.000Z&date=%3C2017-01-24T21%3A56%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#reports

seems to be Windows only; didn't look exhaustively.
Assignee: nobody → jwwang
Priority: -- → P1
See Also: → 1333644
http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/dom/html/HTMLMediaElement.h#768

We should shut down the existing decoder before updating mDecoder otherwise the decoder will never be shut down.
Attachment #8830191 - Flags: review?(gsquelart)
Attachment #8830191 - Flags: review?(gsquelart) → review+
Please request sec approval and request/recommend uplifts that are appropriate (including recommendation if it should be in a 51.x point release if any; if so request mozilla-release approval).
This bug is regressed by bug 1302656 which is landed to 52.

The search shows the number of reports increase significantly since 52.0a1.

https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3AMediaDecoder%3A%3ANotifySuspendedStatusChanged&date=%3E%3D2016-07-25T16%3A03%3A04.000Z&date=%3C2017-01-25T16%3A03%3A04.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=version&_sort=-date&page=3#aggregations

Version
X
18 	37.0.1 	1 	0.32 %
7 	37.0.2 	4 	1.28 %
19 	37.0b7 	1 	0.32 %
14 	38.0 	2 	0.64 %
6 	38.0.1esr 	6 	1.92 %
10 	38.0.5 	3 	0.96 %
20 	38.0b4 	1 	0.32 %
21 	38.0b6 	1 	0.32 %
4 	38.2.0esr 	21 	6.71 %
15 	39.0 	2 	0.64 %
22 	39.0.3 	1 	0.32 %
23 	39.0a2 	1 	0.32 %
11 	40.0 	3 	0.96 %
12 	40.0.2 	3 	0.96 %
8 	40.0.3 	4 	1.28 %
24 	41.0.2 	1 	0.32 %
25 	41.0b1 	1 	0.32 %
26 	42.0 	1 	0.32 %
5 	43.0.1 	8 	2.56 %
27 	43.0.3 	1 	0.32 %
13 	43.0b1 	3 	0.96 %
28 	43.0b9 	1 	0.32 %
9 	44.0b1 	4 	1.28 %
16 	50.0.2 	2 	0.64 %
29 	50.1.0 	1 	0.32 %
17 	51.0b3 	2 	0.64 %
3 	52.0a1 	28 	8.95 %
1 	52.0a2 	151 	48.24 %
2 	53.0a1 	54 	17.25 %
30 	54.0a1 	1 	0.32 %
Blocks: 1302656
Comment on attachment 8830191 [details] [diff] [review]
1333576_fix.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very difficult. This patch just ensures some cleanup before discarding a decoder object.

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

Which older supported branches are affected by this flaw? 52 and later versions.

If not all supported branches, which bug introduced the flaw? 1302656

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

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely for the change is quite simple. TreeHerder should be enough for testing.
Attachment #8830191 - Flags: sec-approval?
Duplicate of this bug: 1334089
Changing Firefox 51 to unaffected since you say in the sec-approval request that this is 52 and higher and that matches the time on the bug that caused this.

sec-approval+ for trunk.

You'll want to make and nominate beta and aurora patches as well so we can avoid shipping this issue to the public.
Attachment #8830191 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2de648638139https://hg.mozilla.org/mozilla-central/rev/2de648638139
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8830191 [details] [diff] [review]
1333576_fix.patch

Approval Request Comment
[Feature/Bug causing the regression]:1302656
[User impact if declined]:crash or UAF
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:no
[Why is the change risky/not risky?]:The change is simple and Treeherder doesn't disagree.
[String changes made/needed]:none
Attachment #8830191 - Flags: approval-mozilla-aurora?
Attachment #8830191 - Flags: approval-mozilla-beta?
Comment on attachment 8830191 [details] [diff] [review]
1333576_fix.patch

sec-high fix for beta52 and aurora53
Attachment #8830191 - Flags: approval-mozilla-beta?
Attachment #8830191 - Flags: approval-mozilla-beta+
Attachment #8830191 - Flags: approval-mozilla-aurora?
Attachment #8830191 - Flags: approval-mozilla-aurora+
Group: media-core-security → core-security-release
Duplicate of this bug: 1333644
Duplicate of this bug: 1319001
Duplicate of this bug: 1326294
Duplicate of this bug: 1319900
Duplicate of this bug: 1334094
JW - any idea what the cause of the failures before 52 were?  Any chance this will fix them as well?
Flags: needinfo?(jwwang)
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=mozilla%3A%3AMediaDecoder%3A%3ANotifySuspendedStatusChanged#aggregations

38.2.0esr 	10 	7.41 %
52.0a2 	        4       2.96 %
52.0b1 	        120 	88.89 %
53.0a1 	        1 	0.74 %

10 reports from 38.2.0esr. 38.2.0esr is too ancient to debug. However, I believe crashes before 52 are from different causes.
Flags: needinfo?(jwwang)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.