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

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

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

Comment 1

[Karl Tomlinson (:karlt)]

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.

Comment 2

[Karl Tomlinson (:karlt)]

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.

Comment 3

[Karl Tomlinson (:karlt)]

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.

Comment 4

[Karl Tomlinson (:karlt)]

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

Comment 5

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

Bump - karl, where does this stand?

Comment 6

[Karl Tomlinson (:karlt)]

Since changes in bug 1062661, security rating is downgraded to small-null-offset deref.

Comment 7

[Bobby Holley (:bholley)]

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.

Comment 8

[Matt Woodrow (:mattwoodrow)]

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

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: shutdown-decoder

Comment 10

[Karl Tomlinson (:karlt)]

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?

Comment 11

[Karl Tomlinson (:karlt)]

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

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: shutdown-decoder v2

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: 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.

Comment 14

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eed7ad9fbb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c4ddf14434

Comment 15

[Jean-Yves Avenard [:jya]]

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.

Comment 16

[Jean-Yves Avenard [:jya]]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1eed7ad9fbb7
https://hg.mozilla.org/mozilla-central/rev/88c4ddf14434

Comment 18

[Karl Tomlinson (:karlt)]

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.

Comment 19

[Karl Tomlinson (:karlt)]

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

Comment 20

[Ralph Giles (:rillian)]

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.

Comment 21

[Ralph Giles (:rillian)]

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

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 23

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

(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?

Comment 24

[Karl Tomlinson (:karlt)]

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

Comment 25

[Benjamin Kerensa [:bkerensa]]

Does this impact ESR31?

Comment 26

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

We don't have MSE in ESR31 per comment 24.