race conditions in TrackBuffer::RemoveDecoder() may lead to null deref

RESOLVED FIXED in Firefox 37

Status

()

Core
Audio/Video
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: karlt, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug, {csectype-dos, sec-low})

36 Branch
mozilla38
csectype-dos, sec-low
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox36 disabled, firefox37 fixed, firefox38 fixed, firefox-esr31 disabled)

Details

Attachments

(2 attachments, 1 obsolete attachment)

TrackBuffer::RemoveDecoder() may be called from the initialization task queue
from TrackBuffer::InitializeDecoder().

Under the parent decoder monitor, on the initialization task queue, it then
reduces the length of mDecoders and may modify mCurrentDecoder.

mCurrentDecoder is null-checked without the monitor on the main thread in
AppendDataToCurrentResource() and Detach(), leading to possible null deref.

mDecoders is iterated on the main thread without the monitor in EvictData(),
EvictBefore(), Dump().

When mDecoders length is reduced but still > 0, memory is not reallocated, and
elements are merely shifted, potentially appearing as multiple copies of a
decoder, currently with no major side effects afaik.

When mDecoders length is reduced to zero, the memory is freed, potentially
leading to UAF.

Disabled in RELEASE_BUILD
(Reporter)

Comment 1

3 years ago
I think the preferable models here are that mCurrentDecoder is accessed only
on the main thread and mDecoders is modified only on the main thread.

Given TrackBuffer::RemoveDecoder() dispatches an event to the main thread
anyway, I think that would be quite achievable.
(Reporter)

Comment 2

3 years ago
Re the spec, errors should not be reported and buffered ranges should not be
modified outside of "updating" intervals.

That probably requires delaying the updateend event until after
InitializeDecoder() has run, if it is going to run.
However, we don't want to delay the updateend if ReadMetadata will not
unblock.

Spec compliance may be outside the scope of this bug.
(Reporter)

Comment 3

3 years ago
Attachment 8516267 [details] [diff] intends to remove use of mDecoders in EvictData() and
EvictBefore(), which would remove the UAF security exposure as Dump() is called only from a debugger I assume.
Group: media-core-security
(Reporter)

Comment 4

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #2)
> Re the spec, errors should not be reported and buffered ranges should not be
> modified outside of "updating" intervals.

Bug 1097252 covers that.
Bump - karl, where does this stand?
Flags: needinfo?(karlt)
Priority: -- → P1
(Reporter)

Comment 6

3 years ago
Since changes in bug 1062661, security rating is downgraded to small-null-offset deref.
Depends on: 1062661
Flags: needinfo?(karlt)
Keywords: csectype-uaf, sec-high → csectype-dos, sec-low
Assignee: nobody → bobbyholley
Anthony, is there a reason this should still be p1? I'm pretty sure the worst we'd get would be a crash, and I"d imagine we'd be seeing crash bugs about it if that were happening.
Flags: needinfo?(ajones)
Flags: needinfo?(ajones)
Priority: P1 → P2
Assignee: bobbyholley → matt.woodrow
(Assignee)

Comment 8

3 years ago
(In reply to Karl Tomlinson (back 19 Jan :karlt) from comment #2)
> Re the spec, errors should not be reported and buffered ranges should not be
> modified outside of "updating" intervals.
> 
> That probably requires delaying the updateend event until after
> InitializeDecoder() has run, if it is going to run.
> However, we don't want to delay the updateend if ReadMetadata will not
> unblock.
> 
> Spec compliance may be outside the scope of this bug.

I think ideally we'd accumulate bytes in the 'input buffer' (as per the spec) until we have an entire init segment, and only then append them to a SourceBufferResource and initialize a decoder.

That way we know ReadMetadata will complete without blocking, and we can delay updateend until it finishes.
(Assignee)

Comment 9

3 years ago
Created attachment 8549337 [details] [diff] [review]
shutdown-decoder
Attachment #8549337 - Flags: review?(karlt)
(Reporter)

Updated

3 years ago
Summary: race conditions in TrackBuffer::RemoveDecoder() may lead to null deref or UAF → race conditions in TrackBuffer::RemoveDecoder() may lead to null deref
(Reporter)

Updated

3 years ago
See Also: → bug 1123492
(Reporter)

Comment 10

3 years ago
Comment on attachment 8549337 [details] [diff] [review]
shutdown-decoder

This is what I had in mind in comment 1.  I think this is still the right
approach for mCurrentDecoder, but some things have changed re access of
mDecoders, so this patch either needs to hold the mutex, or probably better leave mDecoders behavior as is, for now.

TrackBuffer::Buffered() reads mDecoders under parent decoder mutex.
This is no longer main-thread only since bug 1091008.
The function can't switch to mInitializedDecoders because ranges are
currently determined on main thread, perhaps before initialization has
happened.  Fixing bug 1097252 would probably change this, but currently
modifying mDecoders will require holding the mutex.

Some other notes, which don't require action here:

  Since https://hg.mozilla.org/mozilla-central/diff/96b86345fe30/dom/media/mediasource/TrackBuffer.cpp
  ContinueShutdown() writes to mDecoders off main thread, under mutex from
  parent decoder task queue.  I think this could use mInitializedDecoders
  instead.

  SetCDMProxy seems to be only called from HTMLMediaElement, so I assume that
  is main thread.

I think ideally mDecoders would be a main thread array (or perhaps even
removed) and mInitializedDecoders would be the array accessed off main thread,
but we are not at that stage yet and all mDecoders access is now under mutex
except for ResetDecode(), as described in bug 1123492, so I don't think there
is a need to change the mDecoders operation in RemoveDecoder() now.  Given
moving the mDecoders RemoveElement() to ReleaseDecoderTask now would require
holding the mutex again, I'd lean towards keeping it for now in
RemoveDecoder(), where the mutex is already held.  I can't think of any other
unwanted effects from doing the RemoveElement() in ReleaseDecoderTask, but
there may be more risk with current uses of mDecoders.

>+    // TODO: This can result in a change of buffered
>+    // regions outside of update/updateend events.

Can you reference bug 1097252 here, if you keep this comment, please?
Attachment #8549337 - Flags: review?(karlt) → review-
(Reporter)

Comment 11

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> I think ideally we'd accumulate bytes in the 'input buffer' (as per the
> spec) until we have an entire init segment, and only then append them to a
> SourceBufferResource and initialize a decoder.
> 
> That way we know ReadMetadata will complete without blocking, and we can
> delay updateend until it finishes.

That could be made to work, but involves two different pathways agreeing on
what makes an init segment.  It may however be the easiest way to do things in
the current framework.
(Assignee)

Comment 12

3 years ago
Created attachment 8552136 [details] [diff] [review]
shutdown-decoder v2
Attachment #8549337 - Attachment is obsolete: true
Attachment #8552136 - Flags: review?(karlt)
(Reporter)

Updated

3 years ago
Attachment #8552136 - Flags: review?(karlt) → review+
(Reporter)

Comment 13

3 years ago
Created attachment 8562362 [details] [diff] [review]
document that mCurrentDecoder is modified on main thread only

I would like to say *accessed* on main thread only, but
https://hg.mozilla.org/mozilla-central/rev/3c4dc1b74b7f indroduced some
off-main-thread reads.  I'm not clear how racey the reads are nor whether they
are just optimizations or required for correct behavior.

MediaSourceDecoder::Shutdown() does Detach() before initiating shutdown on the
decode task queue.
Attachment #8562362 - Flags: review?(matt.woodrow)
(Assignee)

Updated

3 years ago
Attachment #8562362 - Flags: review?(matt.woodrow) → review+
(Reporter)

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eed7ad9fbb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c4ddf14434
Flags: in-testsuite-
Comment on attachment 8552136 [details] [diff] [review]
shutdown-decoder v2

Review of attachment 8552136 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/TrackBuffer.cpp
@@ +655,4 @@
>    {
>      ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
>      mInitializedDecoders.RemoveElement(aDecoder);
>      mDecoders.RemoveElement(aDecoder);

That seems wrong to me...
the code rely on mCurrentDecoder to be null to know when to abort its initalization.
DiscardCurrentDecoder would have cleared mCurrentDecoder; and now we don't only much later.

As such, we would have initialization of the decoder that will complete when previously it wouldn't.

I haven't followed the problem entirely, but I think the entire problem could have been avoided if we only deleted mInitializedDecoders during shutdown. Remove mCurrentDecoder from mDecoders. And mCurrentDecoder would have been deleted in the normal TrackBuffer destructor which will happen on the main thread when the sourcebuffer is deleted.
I should add that now mCurrentDecoder may be used by the MediaSourceReader only once it's been initialized. The risk of race there has been completely removed already.
https://hg.mozilla.org/mozilla-central/rev/1eed7ad9fbb7
https://hg.mozilla.org/mozilla-central/rev/88c4ddf14434
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Reporter)

Comment 18

3 years ago
After considering comment 15, jya and I concluded that it was
actually not necessary for TrackBuffer::RemoveDecoder() to clear
mCurrentDecoder.  Instead SourceBuffer (and the parts of TrackBuffer called
from SourceBuffer, on the main thread) and the mInitializationPromise handling
can manage mCurrentDecoder.

I suspect there are some existing issues with handling appends after an
append error (without any abort()), but such issues exist (perhaps in slightly different forms) whether not mCurrentDecoder is cleared before the promise rejection handler runs.
(Reporter)

Comment 19

3 years ago
(In reply to Karl Tomlinson (:karlt) from comment #18)
> After considering comment 15, jya and I concluded that it was
> actually not necessary for TrackBuffer::RemoveDecoder() to clear
> mCurrentDecoder.

bug 1132328.
status-firefox37: --- → affected
Comment on attachment 8552136 [details] [diff] [review]
shutdown-decoder v2

Requesting 37 uplift of both patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes from bad/malicious web content. Less consistent testing and harder to backport later fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This change is specific to MSE, so risk of regression outside that feature is minimal.
[String/UUID change made/needed]: None.
Attachment #8552136 - Flags: approval-mozilla-aurora?
Pushed to aurora with pre-approval from lmandel.

https://hg.mozilla.org/releases/mozilla-aurora/rev/82ab77166a5f
https://hg.mozilla.org/releases/mozilla-aurora/rev/6595ccb73af6
status-firefox37: affected → fixed
Comment on attachment 8552136 [details] [diff] [review]
shutdown-decoder v2

As stated, this bug was pre-approved to land with a set of changes for MSE. Marking the approval after the fact.
Attachment #8552136 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8562362 - Flags: approval-mozilla-aurora+
Group: media-core-security
(In reply to Karl Tomlinson (:karlt) from comment #0)

> Disabled in RELEASE_BUILD

Is this true and this isn't present in final Firefox builds in "affected" versions?
Flags: needinfo?(karlt)
(Reporter)

Comment 24

3 years ago
(In reply to Al Billings [:abillings] from comment #23)
> Is this true and this isn't present in final Firefox builds in "affected"
> versions?

Yes, MSE is not enabled in versions prior to 37 and so they are not affected by this bug.
Flags: needinfo?(karlt)
status-firefox36: --- → disabled

Does this impact ESR31?
Flags: needinfo?(abillings)
(Reporter)

Updated

3 years ago
status-firefox-esr31: --- → disabled
We don't have MSE in ESR31 per comment 24.
Flags: needinfo?(abillings)

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.