Closed
Bug 1472925
Opened 6 years ago
Closed 6 years ago
Potential UaF of MSG from CompleteAudioContextOperations
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P2)
Tracking
()
People
(Reporter: karlt, Assigned: karlt)
References
Details
(Keywords: csectype-uaf, regression, sec-high, Whiteboard: [post-critsmash-triage][adv-main62+][adv-esr60.2+])
Attachments
(3 files)
4.36 KB,
patch
|
padenot
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
4.18 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
4.12 KB,
patch
|
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1471953 +++ On ThreadedDriver thread: 1. MediaStreamGraphInitThreadRunnable::Run() dispatches AsyncCubebOperation::SHUTDOWN to an AsyncCubebTask thread. https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/media/GraphDriver.cpp#194 2. ThreadedDriver::RunThread() can hand the graph back to the main thread. On the main thread: 3. MediaStreamGraphShutDownRunnable::Run() waits for shutdown of the ThreadedDriver thread and then deletes the graph. https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/media/MediaStreamGraph.cpp#1466 On the AsyncCubebTask thread: 4. CompleteAudioContextOperations tries to use the graph and calls a virtual function on a stale reference. https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/media/GraphDriver.cpp#510 https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/media/MediaStreamGraph.cpp#4125 This is a very unlikely sequence of events, but I don't see anything to guarantee it won't happen. Detected by resetting GraphDriver::mGraphImpl when no longer safe to use https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebb91a780f7eb6be87903ae68659d7f482682fbf&selectedJob=186125558
Assignee | ||
Updated•6 years ago
|
Summary: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:296:27 in get → Potential UaF of MSG from CompleteAudioContextOperations
Assignee | ||
Comment 1•6 years ago
|
||
This should work provided nothing tries to wake up the MSG again. I think we're safe from that because mPendingUpdateRunnables will not be run. https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc2db90dbbd2b8c20cef220d7bf3fad5fd17c172&selectedJob=186138137
Attachment #8989339 -
Flags: review?(padenot)
Updated•6 years ago
|
Attachment #8989339 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 2•6 years ago
|
||
Comment on attachment 8989339 [details] [diff] [review] keep a strong reference to MediaStreamGraph from GraphDriver [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. We don't have any evidence that this crash can happen in practice, but the existence of bug 1471953 is evidence that similar races can have unexpected winners. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The patch identifies the security problem. It does not identify exactly where the stale pointer is referenced, but https://bugzilla.mozilla.org/attachment.cgi?id=8989335 would identify one case if landed at the same time. Which older supported branches are affected by this flaw? All branches since 40 may be affected. If not all supported branches, which bug introduced the flaw? Possibly bug 1094764. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch would need to be modified for other branches, but this would be easy. There is a low risk of introducing a leak, but automated tests should catch if there is such a problem. How likely is this patch to cause regressions; how much testing does it need? Automated testing is sufficient.
Attachment #8989339 -
Flags: sec-approval?
Comment 3•6 years ago
|
||
sec-approval+ for trunk. Can I get Beta and ESR60 patches made and nominated as well, please?
status-firefox61:
--- → wontfix
status-firefox62:
--- → affected
status-firefox63:
--- → affected
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → affected
tracking-firefox62:
--- → +
tracking-firefox63:
--- → +
tracking-firefox-esr60:
--- → 62+
Updated•6 years ago
|
Attachment #8989339 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 4•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0ddea9ad624286eb863e8419e144d5cbae4d58
Assignee | ||
Comment 5•6 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Possibly bug 1094764. [User impact if declined]: Potential sec-high. [Is this code covered by automated tests?]: The code is exercised by automated tests, but we do not have a way to cause the race to be run in a way that reproduces the bug. [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: There is a low risk of introducing a leak, but automated tests should catch if there is such a problem. [Why is the change risky/not risky?]: It changes object lifetimes. [String changes made/needed]: None.
Attachment #8990798 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 6•6 years ago
|
||
[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high. User impact if declined: Potential sec-high. Fix Landed on Version: 63 Risk to taking this patch (and alternatives if risky): There is a low risk of introducing a leak, but automated tests should catch if there is such a problem. String or UUID changes made by this patch: None
Attachment #8990799 -
Flags: approval-mozilla-esr60?
![]() |
||
Comment 7•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a0ddea9ad62
Group: media-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 8•6 years ago
|
||
From talking with Karl on irc, sounds like this fixes both this and bug 1471953. Let's uplift this for beta and esr60 since it already landed on trunk.
Comment 9•6 years ago
|
||
Comment on attachment 8990798 [details] [diff] [review] beta patch Fix for UAF, let's uplift for beta 8.
Attachment #8990798 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 10•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/47b9b8ecc3f1
Updated•6 years ago
|
Keywords: regression
Comment 11•6 years ago
|
||
Comment on attachment 8990799 [details] [diff] [review] esr60 patch Approved for ESR 60.2 also.
Attachment #8990799 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 12•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/511fca97fb60
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main62+][adv-esr60.2+]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•