Closed
Bug 1130150
Opened 10 years ago
Closed 10 years ago
AudioGUM thread can access freed SourceMediaStream under heavy load
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: jesup, Assigned: jesup)
References
Details
(Keywords: csectype-uaf, regression, sec-high, Whiteboard: [adv-main37+])
Attachments
(2 files)
3.48 KB,
patch
|
roc
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.49 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
8 simultaneous pc_tests (16 PeerConnections); had a failure in the AudioGUM runnable in AppendToTrack because *this was freed memory. (a5) Linux64, debug nightly. More details to come.
Assignee | ||
Comment 1•10 years ago
|
||
This is the AudioGUM runnable: RUN_ON_THREAD(mThread, WrapRunnable(mSources[i], &SourceMediaStream::AppendToTrack,... mSources is nsTArray<SourceMediaStream*> mSources. Normally, the mSources values are held alive by the owning DOMUserMediaStream for the lifetime of mSources, but with the dispatch for AudioGUM, a raw ptr can be accessed on another thread independent of the MediaEngineWebRTCAudioSource's lifetime. Introduced with bug 979716, which landed in 33 and was uplifted to 32. Solution likely is simply to make mSources an nsTArray<nsRefPtr<...>> instead (and for good measure, the video mSources as well. traffic on mSources is low. Probably very hard to exploit (witness how hard I had to stress my linux box to hit this, with a ridiculous amount of memory traffic occurring), but it is a UAF.
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox38:
--- → affected
status-firefox-esr31:
--- → unaffected
Keywords: csectype-uaf
Assignee | ||
Comment 2•10 years ago
|
||
Note: due to how WrapRunnable constructs dynamic runnables, the first arg/local member is an nsRefPtr<SourceMediaStream>, and so has it's own reference
Assignee | ||
Updated•10 years ago
|
Attachment #8560292 -
Flags: review?(roc)
Attachment #8560292 -
Flags: review?(roc) → review+
Updated•10 years ago
|
Blocks: 979716
Keywords: regression,
sec-high
Assignee | ||
Comment 3•10 years ago
|
||
Trivial rebase to Aurora/37
Assignee | ||
Updated•10 years ago
|
Attachment #8565981 -
Attachment description: mSources update → mSources update (Aurora/37 version)
Attachment #8565981 -
Flags: review+
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8560292 [details] [diff] [review] mSources update [Security approval request comment] How easily could an exploit be constructed based on the patch? Pretty hard. It only indirectly exposes that something should be refcounted. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No Which older supported branches are affected by this flaw? See above: 32 and later If not all supported branches, which bug introduced the flaw? Bug 979716, which landed in 33 and was uplifted to 32. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Almost no risk; trivial backport (did Aurora and put it here, but it's trivial) How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions.
Attachment #8560292 -
Flags: sec-approval?
Attachment #8560292 -
Flags: approval-mozilla-aurora?
Comment 5•10 years ago
|
||
I could approve this for Trunk and Aurora but we're about to ship 36. The question is whether this needs to wait until the middle of the next cycle (like another three weeks or so) as a sec-high that is present on release branches. Randell, do you have any thoughts? It looks like the overall risk of us zero daying ourselves is low.
Flags: needinfo?(rjesup)
Assignee | ||
Comment 6•10 years ago
|
||
I do think the overall risk from this is low, given what has to be done to provoke it. In addition, the risk of someone figuring it out from the patch is low. They can notice it's a sec bug, and that it made something refptr'd, but leveraging that to an exploit would be difficult. On the other hand, the risk of leaving it until later in the cycle is also low. There's little risk to landing later given the patch (worst risk would be a leak, but that's unlikely and easy to find/fix).
Flags: needinfo?(rjesup)
Updated•10 years ago
|
Whiteboard: [sec-approval for 3/10 landing]
Comment 7•10 years ago
|
||
Comment on attachment 8560292 [details] [diff] [review] mSources update giving a sec-approval+ for landing on trunk on March 10 then.
Attachment #8560292 -
Flags: sec-approval? → sec-approval+
Updated•10 years ago
|
Updated•10 years ago
|
status-firefox39:
--- → affected
tracking-firefox39:
--- → +
Comment 8•10 years ago
|
||
Comment on attachment 8560292 [details] [diff] [review] mSources update 37 is now Beta. Need this on Beta as well.
Attachment #8560292 -
Flags: approval-mozilla-beta?
Updated•10 years ago
|
Attachment #8560292 -
Flags: approval-mozilla-beta?
Attachment #8560292 -
Flags: approval-mozilla-beta+
Attachment #8560292 -
Flags: approval-mozilla-aurora?
Attachment #8560292 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Rank: 1
Priority: -- → P1
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d065f47c16d https://hg.mozilla.org/releases/mozilla-aurora/rev/c4e811a11277 https://hg.mozilla.org/releases/mozilla-beta/rev/21f52f25675a
Whiteboard: [sec-approval for 3/10 landing]
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d065f47c16d
Target Milestone: --- → mozilla39
Comment 11•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/21f52f25675a https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/c368f8342bee https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/d88a08e20ef5
status-b2g-v1.4:
--- → unaffected
status-b2g-master:
--- → fixed
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 12•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/c368f8342bee https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/d88a08e20ef5
status-b2g-v2.0M:
--- → fixed
status-b2g-v2.1S:
--- → fixed
Updated•10 years ago
|
Whiteboard: [adv-main37+]
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
•