Potential UAF in MediaSourceReader::ReadMetadata() mAudioTrack->Decoders()[0]->GetReader()

RESOLVED FIXED in Firefox 36

Status

()

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: karlt, Assigned: bholley)

Tracking

(Blocks: 1 bug, {csectype-uaf, sec-high})

36 Branch
mozilla37
csectype-uaf, sec-high
Points:
---

Firefox Tracking Flags

(firefox35 unaffected, firefox36 fixed, firefox37 fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 fixed)

Details

(Whiteboard: [b2g-adv-main2.2-])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
MediaSourceReader::ReadMetadata() is called on the decode task queue.

It calls mAudioTrack->Decoders()[0]->GetReader() without holding the decoder
monitor.

Decoders() returns TrackBuffer::mInitializedDecoders, the length of which may
be modified concurrently in TrackBuffer::RegisterDecoder() on the
initialization task queue TrackBuffer::mTaskQueue (with the decoder monitor
held).

Disabled in RELEASE_BUILD
(Reporter)

Comment 1

4 years ago
Possible solutions might include one of:

* Setting mAudioReader in MediaSourceReader::OnTrackBufferConfigured().

* Holding the lock.

* Using the decode task queue to replace TrackBuffer::mTaskQueue.
(Reporter)

Updated

4 years ago
Keywords: csectype-uaf
Group: media-core-security
(Assignee)

Comment 2

4 years ago
See also bug 1113282, which improves this code but doesn't deal with the fundamental problem. I'll see if I can have a look at this sometime next week.
(Assignee)

Comment 3

4 years ago
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #2)
> See also bug 1113282, which improves this code but doesn't deal with the
> fundamental problem. I'll see if I can have a look at this sometime next
> week.

Oh wait I see - this is about MediaSourceReader's ReadMetaData implementation, not the call made in TrackBuffer. The TrackBuffer stuff is sane-ish AFAICT.

It also looks like MediaSourceReader::ReadMetaData doesn't actually do anything expensive or blocking, so it should be fine to just hold the lock. I'll attach a patch.

Also, can someone explain to me why we have this separate task queue for initialization? Is this something we can get rid of now after the refactoring in bug 1104964, or is it necessary for the blocking ReadMetaData call?
(Assignee)

Updated

4 years ago
Assignee: nobody → bobbyholley
(Assignee)

Comment 4

4 years ago
Created attachment 8539430 [details] [diff] [review]
Hold the lock for the entire duration of MediaSourceReader::ReadMetaData. v1
Attachment #8539430 - Flags: review?(cajbir.bugzilla)
(Assignee)

Comment 6

4 years ago
Comment on attachment 8539430 [details] [diff] [review]
Hold the lock for the entire duration of MediaSourceReader::ReadMetaData. v1

Looks like Karl is back, so moving the review to him.
Attachment #8539430 - Flags: review?(cajbir.bugzilla) → review?(karlt)
(Reporter)

Comment 7

4 years ago
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3)
> Also, can someone explain to me why we have this separate task queue for
> initialization? Is this something we can get rid of now after the
> refactoring in bug 1104964, or is it necessary for the blocking ReadMetaData
> call?

I assumed it was to allow media from one SourceBuffer (or even the same SourceBuffer) to continue to play while ReadMetadata blocked waiting for JS to provide the rest of the initialization segment and the first frame.  It no longer waits for the first frame, and WebMReader::ReadMetadata should no longer block waiting for the end of the init segment, but there may still be an issue with MP4Reader and partial init segments.
(Reporter)

Updated

4 years ago
Attachment #8539430 - Flags: review?(karlt) → review+
(Assignee)

Comment 8

4 years ago
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #5)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=797d32cde2f4

For some reason this try run didn't trigger any jobs:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=70bd3c18ebc0
https://hg.mozilla.org/mozilla-central/rev/811adfa8d698
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox37: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Group: media-core-security
status-firefox36: --- → unaffected
status-firefox-esr31: --- → unaffected
Comment on attachment 8539430 [details] [diff] [review]
Hold the lock for the entire duration of MediaSourceReader::ReadMetaData. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing; users more like to see Flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This does affect normal video playback, but addresses a race, so any regressions it might cause are better than the alternative.
[String/UUID change made/needed]: none.
Attachment #8539430 - Flags: approval-mozilla-aurora?
Attachment #8539430 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → fixed
status-firefox35: --- → unaffected
status-firefox36: unaffected → fixed
Group: core-security
Whiteboard: [b2g-adv-main2.2-]
You need to log in before you can comment on or make changes to this bug.