Closed
Bug 1177501
Opened 9 years ago
Closed 9 years ago
MediaTimer can allow mThread to be destroyed before Dispatch unwinds
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox39 | --- | wontfix |
firefox40 | + | fixed |
firefox41 | + | fixed |
firefox42 | + | unaffected |
firefox-esr31 | --- | unaffected |
firefox-esr38 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-v2.2r | --- | unaffected |
b2g-master | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: sec-high, Whiteboard: [post-critsmash-triage][adv-main40+])
Attachments
(1 file)
990 bytes,
patch
|
jya
:
review+
lmandel
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
This is the source of the crashes I was seeing in bug 1175768.
Assignee | ||
Comment 1•9 years ago
|
||
Introduced in bug 1135424, so technically 39 is affected, but it's probably much harder to trigger without bug 1175768.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8626285 -
Flags: review?(jyavenard)
Updated•9 years ago
|
Attachment #8626285 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8626285 [details] [diff] [review] Hold a strong ref to mThread. v1 [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily - it demonstrates what the problem is, but triggering and exploiting it is hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Ish. Which older supported branches are affected by this flaw? 39, though harder to trigger. If not all supported branches, which bug introduced the flaw? bug 1135424. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch should apply verbatim. How likely is this patch to cause regressions; how much testing does it need? Very safe.
Attachment #8626285 -
Flags: sec-approval?
Comment 4•9 years ago
|
||
Comment on attachment 8626285 [details] [diff] [review] Hold a strong ref to mThread. v1 Review of attachment 8626285 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaTimer.cpp @@ +40,5 @@ > { > nsCOMPtr<nsIRunnable> task = NS_NewNonOwningRunnableMethod(this, &MediaTimer::Destroy); > + // Hold a strong reference to the thread so that it doesn't get deleted in > + // Destroy(), which may run completely before the stack if Dispatch() begins > + // to unwind. Do you mean MediaTimer::Destroy() could run before MediaTimer::DispatchDestroy() returns? Why is that a problem?
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #4) > Comment on attachment 8626285 [details] [diff] [review] > Hold a strong ref to mThread. v1 > > Review of attachment 8626285 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaTimer.cpp > @@ +40,5 @@ > > { > > nsCOMPtr<nsIRunnable> task = NS_NewNonOwningRunnableMethod(this, &MediaTimer::Destroy); > > + // Hold a strong reference to the thread so that it doesn't get deleted in > > + // Destroy(), which may run completely before the stack if Dispatch() begins > > + // to unwind. > > Do you mean MediaTimer::Destroy() could run before Yes - specifically, mThread can be released, and possibly freed. > MediaTimer::DispatchDestroy() returns? Specifically, before the call to nsThreadPool::Dispatch returns. > Why is that a problem? Because nsThreadPool::Dispatch accesses |this| while unwinding the callstack, and |this| may have been deleted by that point.
Comment 6•9 years ago
|
||
Sorry, I still can't see the problem. It is safe to read |this|. But it is not safe to access the members of MediaTimer after it is deleted, right? Did member access happen after deletion?
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #6) > Sorry, I still can't see the problem. It is safe to read |this|. But it is > not safe to access the members of MediaTimer after it is deleted, right? Did > member access happen after deletion? This is what happens: (1) Thread A does mThread->Dispatch(deletionTask) (2) We end up deep in nsThreadPool::Dispatch, and stick the task in the queue of the thread. (3) We context-switch to the the thread of mThread, _before_ returning from ::Dispatch. (4) We run the task, and delete the MediaTimer. (5) Deleting the MediaTimer destroys the nsCOMPtr for mThread, which invokes SharedThreadPool::Release. (6) The SharedThreadPool refcount drops to zero, and we delete it. (7) Deleting the SharedThreadPool destroys the nsCOMPtr for mPool/mEventTarget, which invokes nsThreadPool::Release. (8) The nsThreadPool refcount drops to zero, and we delete it. (9) We context-switch back to thread A. (10) We now need to return from nsThreadPool::Dispatch. This method continues to do more work before returning, and accesses members of the deleted nsThreadPool. (11) Boom. Does that make sense? I'm happy to explain more as needed - it's tricky! :-)
Comment 8•9 years ago
|
||
Thanks for the detailed explaination. :) This is super complicated and I am still trying to figure out which member of nsThreadPool is accessed after deletion. (1) Thread A calls nsThreadPool::Dispatch with a NS_DISPATCH_NORMAL flag. (2) Since it is a normal dispatch, it calls PutEvent() and return. No member is accessed after PutEvent() returns on thread A. (3) mEvents.PutEvent() is called inside the monitor in PutEvent(). There is no way to run deletionTask before thread A releases the monitor. (4) If spawnThread is false, PutEvent() returns immediately. No member is accessed. (5) The only place for UAF to occur is https://hg.mozilla.org/mozilla-central/file/bbc26cc168c7/xpcom/threads/nsThreadPool.cpp#l111 where nsThreadPool is deleted on another thread before it acquires the monitor again on thread A. However it means deletionTask is executed despite spawnThread is true. This seems impossible to me.
Comment 9•9 years ago
|
||
Sec-approval? comments imply this is 39 and up but flags say 38 is affected. Is 38 affected? I'm giving sec-approval+ after talking to Bobby in person.
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → ?
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
Updated•9 years ago
|
Attachment #8626285 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ac7b53802a
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/10ac7b53802a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 12•9 years ago
|
||
Bobby, could you fill the uplift request to aurora and beta? Thanks
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] PTO => July 10th from comment #12) > Bobby, could you fill the uplift request to aurora and beta? Thanks This is already on aurora, right? I'll fill out a request for beta now.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8626285 [details] [diff] [review] Hold a strong ref to mThread. v1 Approval Request Comment [Feature/regressing bug #]: bug 1135424 [User impact if declined]: security [Describe test coverage new/current, TreeHerder]: none [Risks and why]: extremely safe [String/UUID change made/needed]: none
Attachment #8626285 -
Flags: approval-mozilla-beta?
Comment 15•9 years ago
|
||
Comment on attachment 8626285 [details] [diff] [review] Hold a strong ref to mThread. v1 Small fix that has been on 41 for a couple of weeks. Beta+ 42 is also marked as affected but given that this landed while 41 was on m-c I take it this is a mistake. Unless I hear otherwise, 42 status should be cleared.
Attachment #8626285 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Updated•9 years ago
|
Comment 16•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/05d720b741cf
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-v2.2r:
--- → unaffected
status-b2g-master:
--- → fixed
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40+]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•