Closed Bug 1092025 Opened 6 years ago Closed 6 years ago

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

Categories

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

36 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
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

People

(Reporter: karlt, Assigned: bholley)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [b2g-adv-main2.2-])

Attachments

(1 file)

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
Possible solutions might include one of:

* Setting mAudioReader in MediaSourceReader::OnTrackBufferConfigured().

* Holding the lock.

* Using the decode task queue to replace TrackBuffer::mTaskQueue.
Keywords: csectype-uaf
Group: media-core-security
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.
(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: nobody → bobbyholley
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)
(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.
Attachment #8539430 - Flags: review?(karlt) → review+
(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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Group: media-core-security
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+
Group: core-security
Whiteboard: [b2g-adv-main2.2-]
You need to log in before you can comment on or make changes to this bug.