1333576 - Crash in mozilla::MediaDecoder::NotifySuspendedStatusChanged

Status: RESOLVED FIXED

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

Description

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1333576_fix.patch

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.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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).

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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 %

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 6

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

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2de648638139a829f41180e6b72d6968f593612c

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2de648638139https://hg.mozilla.org/mozilla-central/rev/2de648638139

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8830191 [details] [diff] [review]
1333576_fix.patch

sec-high fix for beta52 and aurora53

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/589165fd4e972e0476c0845e35201601b674cdb7

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2be2a6d8c05

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

JW - any idea what the cause of the failures before 52 were?  Any chance this will fix them as well?

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

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.