1378826 - Removing the last track from a recorder results in crash

Status: VERIFIED FIXED

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

Description

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: ProofOfConcept.html

Performing the following results in a use after free:
- Create a media recorder with a stream with a single video track
- Remove the track from the stream
- Stop the media recorder

See the attached proof of concept (beware, this will crash your FireFox). To recreate the bug start a recording then click the break button.

This looks like an issue with MediaRecorder clean up/notify track removed. The recorder will not clean up the listener on the video track due to the track being removed[1]. As such the listener will still be used even after the recorder is gone and has freed related structures.

[1]: https://searchfox.org/mozilla-central/source/dom/media/MediaRecorder.cpp#848

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I'm going to grab this. Current plan is that we can perform checks when notifying that a track is removed to avoid this.

Setting this to sec-high based on UAF. :jesup, can you change the level as you think appropriate?

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Remove direct track listeners for video tracks if MediaRecorder is notified of their removal

Comment 3

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

Please request security-approval to land, and consider asking for beta approval

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8884995 [details] [diff] [review]
Remove direct track listeners for video tracks if MediaRecorder is notified of their removal

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- The patch itself doesn't make clear that a UAF is involved, rather that a listener is not being removed. However, to invoke the scenario that this patch protects against is quite easy (as shown by the attached proof of concept), though not a normal usage of the MediaRecorder.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- I don't think so, aside from the current bug comment being a secure big (this one). I have omitted reference to the UAF from the patch comment and am explicitly holding off on adding a crash test until we're landed (to be done in a follow up bug).

Which older supported branches are affected by this flaw?
- 54, 55.

If not all supported branches, which bug introduced the flaw?
- The bug appears to have been introduced between 52 and 53. Issue may have been introduced earlier but become more obvious at that stage, however. Specific regression range and discussion at end of this comment.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- I believe the current patch will resolve the issue in older branches and does not require a separate, specific backport.

How likely is this patch to cause regressions; how much testing does it need?
- Low. The recording machinery the patch interacts with is complicated, but the patch itself is simple (less than 10 lines). I have manually tested this patch, and the MediaRecorder has automated test suites which provide some coverage for other regressions once this goes into nightly.

Approval Request Comment
[Feature/Bug causing the regression]: See regression range and discussion at end of comment.
[User impact if declined]: A use after free is exposed.
[Is this code covered by automated tests?]: No, I am holding off on adding a test to avoid painting a target. However, an automated crash test can be created from the proof of concept attached.
[Has the fix been verified in Nightly?]: Locally by myself.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, the steps detailed in the original comment will verify the fix. 
[List of other uplifts needed for the feature/fix]: The included patch.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: The change is small, but it does deal with complicated machinery (media recording and associated state).
[String changes made/needed]: None.

---

A bisection in mozregression show this being introduced in the range of: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=9ea8e4f2edae49b3e46b71130f2c4303eafdf1bc&full=1

However, after looking at those patches I wonder if the issue still exists before these changes. These changes add the use of a monitor which is where the crashes are happening in my test, but I do not believe they introduce the UAF.

That said, the versions mentioned above are certainly affected. I'm going to investigate further and will follow up should I find that esr 52 is impacted.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 8884995 [details] [diff] [review]
Remove direct track listeners for video tracks if MediaRecorder is notified of their removal

sec-approval=dveditz
a=dveditz to land on beta.

Comment 6

[Daniel Veditz [:dveditz]]

If this affects ESR 52 we'll want to hit the 52.3 release that ships when Firefox 55 does. fwiw I couldn't get the POC to crash on ESR 52, but I did get an InvalidState message once or twice.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1362c0928dc15e9a9cd6fa7ed9ee57a14365286e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1362c0928dc1

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

After more investigation I believe I'm reproing the crash on ESR[1]. I had to run through the proof of concept a fair number of times to do so (~10). It seems to be happening less often due to the ops performed on the freed object. Looking at the code some more I think it is the case that the regression range above is a red herring and that this has been around for much longer.

[1]: https://crash-stats.mozilla.com/report/index/e8e44331-1320-4ca8-b539-f79140170712

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8884995 [details] [diff] [review]
Remove direct track listeners for video tracks if MediaRecorder is notified of their removal

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sechigh
Fix Landed on Version: 56, uplifted to 55
Risk to taking this patch (and alternatives if risky): Low. The change is small, but it does deal with complicated machinery (media recording and associated state).
String or UUID changes made by this patch: None.

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

Comment 12

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

Comment on attachment 8884995 [details] [diff] [review]
Remove direct track listeners for video tracks if MediaRecorder is notified of their removal

We should take this fix on esr also for 52.3.0 since this is a sec-high issue and fixed for 55.

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

This is the first time I've been involved with an ESR uplift. Please advise should I need do anything more or if I've overlooked anything.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/98ff43fb228a

Comment 15

[Andrei Vaida [:avaida]]

Reproduced the crash on an affected build (54.0.1, 20170628075643, 90f18f9c15f7) [1] using the test case from Comment 0 on Windows 10 x64.

I can confirm the test case is no longer crashing on Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12 using:
    - 52.2esr (20170723091719, 3856be3d222f)
    - 55.0b10 (20170717063821, e26b1f5d635e)
    - 56.0a1 (2017-07-23, 20170723030206)

[1] bp-84570390-f785-48a9-92aa-bc69c1170724

Comment 16

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Add test for removal of video tracks during recording

Comment 17

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8899672 [details] [diff] [review]
Add test for removal of video tracks during recording

Follow up crash test now that this fix has been landed for some time. Going to request review, sec approval, then checkin as per initial fix. Please advise should a different procedure be followed.

Comment 18

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8899672 [details] [diff] [review]
Add test for removal of video tracks during recording

I am requesting sec approval with the primary goal of confirming enough time has passed that it is appropriate to land a test for this bug.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- It's clear from this test how to recreate the exploit, though further effort would be required to abuse the UAF.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- This is the just a follow up test. The comment is left somewhat vague to avoid mentioning security, but the nature of this test being a crash test does paint a target.

Which older supported branches are affected by this flaw?
- All supported branches are now patched via prior work on this bug.

If not all supported branches, which bug introduced the flaw?
- All supported branches should be immune to the tested bug.

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

How likely is this patch to cause regressions; how much testing does it need?
- This patch is a test that should not impact other tests or require special testing. Any issues/failures should be isolated to this test.

Comment 19

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

I don't think we can approve the landing of tests yet. The build has only been out for a short while and the uptake it pretty low, AFAIK. NI? Ritu and Liz to get a sense of what uptake on 55 is.

Comment 20

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

We will likely be increasing rollout to 55 once 55.0.3 is out. Best to wait till a little while after we get 55.0.3 out the door.

Comment 21

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Cheers. Will follow up after 55.0.3 has been out for awhile. What is the best way re-query around that time? Drop approval flag and raise again? needinfo?

Comment 22

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

You can just ni? me.

Comment 23

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

Comment on attachment 8899672 [details] [diff] [review]
Add test for removal of video tracks during recording

Clearing SA until later.

Comment 24

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Has a suitable amount of time passed that this would be safe to land?

Comment 25

[Daniel Veditz [:dveditz]]

Yeah, should be fine. Please set the "in-testsuite" flag to '+' when you do.

Comment 26

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da62db8d01c922953e0ddf6271352c07521bc6de
Bug 1378826 - Add test for removal of video tracks during recording. r=jesup

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da62db8d01c9