race conditions in TrackBuffer::mInitializationPromise

RESOLVED FIXED in Firefox 37

Status

()

P2
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: karlt, Assigned: jya)

Tracking

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

37 Branch
mozilla39
csectype-uaf, sec-high
Points:
---

Firefox Tracking Flags

(firefox37+ fixed, firefox38+ fixed, firefox39 fixed, firefox-esr31 unaffected)

Details

(Whiteboard: [adv-main37+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
AFAICS TrackBuffer::mInitializationPromise.RejectIfExists() may be called on
the main thread via SourceBuffer::Abort() while also being called on an
initialization task queue thread from InitializeDecoder().

This could lead to a null deref if MediaPromiseHolder::mPromise has been
cleared, or use-after-free calling Reject() or Release() on a deleted mPromise.

Perhaps the RejectIfExists() can be proxied to the main thread.
(Assignee)

Comment 1

4 years ago
Only change required is TrackBuffer::AbortAppendData() to hold the monitor before calling reject.

if abort() got to run first, InitializeDecoder will immediately exit , after that it holds the lock.

The only other time reject can be called is in TrackBuffer::AppendData and this can never be called while TrackBuffer::InitializeDecoder hasn't completed.
(Reporter)

Updated

4 years ago
Summary: race conditions in TrackBuffer::RemoveDecoder() may lead to null deref → race conditions in TrackBuffer::mInitializationPromise
(Assignee)

Comment 2

4 years ago
Created attachment 8565766 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

Implement solution discussed earlier. Detect if abort() was called while the trackbuffer was in the middle of calling readmetadata and initalizing decoders
Attachment #8565766 - Flags: review?(karlt)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
(Reporter)

Comment 3

4 years ago
Comment on attachment 8565766 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

> TrackBuffer::AbortAppendData()
> {
>   DiscardCurrentDecoder();
>   // The SourceBuffer would have disconnected its promise.
>   // However we must ensure that the MediaPromiseHolder handle all pending
>   // promises.
>+  ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
>   mInitializationPromise.RejectIfExists(NS_ERROR_ABORT, __func__);
> }

The early exit on (mCurrentDecoder != aDecoder) below is enough to make this
unnecessary, right?  DiscardCurrentDecoder() unsets mCurrentDecoder in the
monitor (on the main thread), so testing mCurrentDecoder in the monitor on the
initialization queue is sufficient to avoid using mInitializationPromise at
the wrong time.

>+  if (mCurrentDecoder != aDecoder || mInitializationPromise.IsEmpty()) {

Is the mInitializationPromise.IsEmpty() test required?
Please remove if not.
I assume we use Reject() instead of RejectIfExists() below to clarify that it will exist and there is no race.
mInitializationPromise is Ensure()d outside of the monitor in AppendData(), so
I feel uncomfortable testing it on this thread.

We have just marked the resource not-ended.  If aDecoder != mCurrentDecoder,
then I guess it should be ended?  Should the AppendData() call above instead be after this (mCurrentDecoder != aDecoder) early return?
Attachment #8565766 - Flags: review?(karlt)
(Reporter)

Comment 4

4 years ago
(In reply to Karl Tomlinson (:karlt) from comment #3)
> I assume we use Reject() instead of RejectIfExists() below to clarify that
> it will exist and there is no race.

I mean, I assume we *could* use ...
(Assignee)

Comment 5

4 years ago
Created attachment 8566918 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

applying comments. You're right, we could be using Reject() safely in place of RejectIfExists; as we are guaranteed that if we reach that point the promise holder holds a promise reference. Also, the only place where the promise would be created (in AppendData), will only ever be called until the initialization process have completed; or been cancelled (in which case InitializeDecoder would have iterrupted earlier).
Attachment #8566918 - Flags: review?(karlt)
(Assignee)

Updated

4 years ago
Attachment #8565766 - Attachment is obsolete: true
(Reporter)

Comment 6

4 years ago
Comment on attachment 8566918 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

> TrackBuffer::AbortAppendData()
> {
>   DiscardCurrentDecoder();
>   // The SourceBuffer would have disconnected its promise.
>   // However we must ensure that the MediaPromiseHolder handle all pending
>   // promises.
>+  ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
>   mInitializationPromise.RejectIfExists(NS_ERROR_ABORT, __func__);
> }

Please remove this change now that mutually exclusive access to mInitializationPromise is protected by the state of mCurrentDecoder, which is always modified in the monitor (on main thread), and read off-main-thread in the monitor.
Attachment #8566918 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/8c2b291548ad
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Group: media-core-security
Does this need to land on any other branches?
Flags: needinfo?(jyavenard)
(Assignee)

Comment 9

4 years ago
Yes, including beta 37
Flags: needinfo?(jyavenard) → needinfo?(giles)
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox-esr31: --- → unaffected
Comment on attachment 8566918 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes with video playback, possible exploitation by malicious content.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This is a small and straightforward fix, preventing null dereference and adding a lock. It's had two weeks to setting on central, so regression risk is low.
[String/UUID change made/needed]: None
Flags: needinfo?(giles)
Attachment #8566918 - Flags: approval-mozilla-beta?
Attachment #8566918 - Flags: approval-mozilla-aurora?
Attachment #8566918 - Flags: approval-mozilla-beta?
Attachment #8566918 - Flags: approval-mozilla-beta+
Attachment #8566918 - Flags: approval-mozilla-aurora?
Attachment #8566918 - Flags: approval-mozilla-aurora+
tracking-firefox37: --- → +
tracking-firefox38: --- → +
Whiteboard: [adv-main37+]

Updated

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