Closed
Bug 1302817
Opened 8 years ago
Closed 8 years ago
Indexing error in MediaStreamGraphImpl::UnregisterCaptureStreamForWindow might cause cross-site access
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: q1, Assigned: padenot)
Details
(Keywords: reporter-external, sec-moderate, Whiteboard: [post-critsmash-triage])
Attachments
(1 file, 1 obsolete file)
1.09 KB,
patch
|
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
MediaStreamGraphImpl::UnregisterCaptureStreamForWindow (dom\media\MediaStreamGraph.cpp) incorrectly indexes an array, potentially causing it to leave in place an element that it should have deleted. 3820: void 3821: MediaStreamGraphImpl::UnregisterCaptureStreamForWindow(uint64_t aWindowId) 3822: { 3823: MOZ_ASSERT(NS_IsMainThread()); 3824: for (uint32_t i = 0; i < mWindowCaptureStreams.Length(); i++) { 3825: if (mWindowCaptureStreams[i].mWindowId == aWindowId) { The bug is that the following line removes element |i| (e.g., element 3), which causes element |i+1| (e.g., element 4) (if it exists) to be shifted down to element |i| (e.g., element 3). However, when control returns to line 3824, above, |i| is incremented (e.g., to 4), so the next time line 3825 examines an element of mWindowCaptureStreams, it doesn't test the first element that was shifted down (e.g., 3), and so line 3826 doesn't remove it if it should be removed. 3826: mWindowCaptureStreams.RemoveElementAt(i); 3827: } 3828: } 3829: } I have marked this as a security bug because I'm unsure whether it might allow, e.g., a site opened in a window that previously hosted a different site, to then use a stream object from the previous site.
Updated•8 years ago
|
Flags: sec-bounty?
Updated•8 years ago
|
Component: DOM: Device Interfaces → Audio/Video: MediaStreamGraph
Updated•8 years ago
|
Group: core-security → media-core-security
Assignee | ||
Comment 1•8 years ago
|
||
This is not enabled on any build and any platform. I'll write a fix soon, though.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → padenot
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8793666 -
Flags: review?(rjesup)
Updated•8 years ago
|
Attachment #8793666 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 3•8 years ago
|
||
Al, what is the policy for landing a fix like this, in code that is only reachable if you flip a pref, and has never been shipped ?
Flags: needinfo?(abillings)
(In reply to Paul Adenot (:padenot) from comment #2) > Created attachment 8793666 [details] [diff] [review] > Iterate backward The patch invokes undefined behavior by underflowing |i| on the last iteration; see C++11 s.5(4). Probably this causes an infinite loop on all real platforms... > for (uint32_t i = mWindowCaptureStreams.Length() - 1; i >= 0; i--) { ...because |i| is unsigned, it always satisfies the loop-control condition |i >= 0|.
Assignee | ||
Comment 5•8 years ago
|
||
True, fix coming up. That's what you get for submitting patches without running the unit tests :-).
One nice form for this kind of loop is: for (auto i = array.Length(); // use right type for |i| i > 0;) { --i; ... array.RemoveElementAt (i); ... }
Comment 7•8 years ago
|
||
I'll just mark this sec-other because we don't ship this anywhere.
Flags: needinfo?(abillings)
Keywords: sec-other
Comment 8•8 years ago
|
||
It could probably also be unhidden if you prefer.
Comment 9•8 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #3) > Al, what is the policy for landing a fix like this, in code that is only > reachable if you flip a pref, and has never been shipped ? The pref thing is an interesting wrinkle. Generally, if the bug is on more branches than just trunk, it needs sec-approval *if* it is rated sec-high or sec-critical. This is functionally disabled though. Realistically, it is probably best to be paranoid and if it had that rating, ask for sec-approval if the code lives on multiple branches. The idea then is that we may want to backport it to Aurora and Beta, for example.
Assignee | ||
Comment 10•8 years ago
|
||
Fixed.
Assignee | ||
Updated•8 years ago
|
Attachment #8793666 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8796124 [details] [diff] [review] patch [Security approval request comment] How easily could an exploit be constructed based on the patch? This could mean cross origin audio data leakage. This code is behind a pref (disabled by default on all releases), and uses a non-standard Web API. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Kind of, although because the feature is non-standard, it's a bit hard to understand the implications. Which older supported branches are affected by this flaw? All older branches have the code, but it's disabled everywhere. If not all supported branches, which bug introduced the flaw? N/A Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should apply cleanly to all branches. How likely is this patch to cause regressions; how much testing does it need?
Attachment #8796124 -
Flags: sec-approval?
Reporter | ||
Comment 12•8 years ago
|
||
FWIW:
> for (int32_t i = mWindowCaptureStreams.Length() - 1; i >= 0; i--) {
...implicitly casts the result of nsTArray::Length() (which currently isa size_t; see class nsTArray_base) to int. Technically this truncates on both 32 and 64 bit platforms. Currently this doesn't matter because the maximum number of elements in an nsTArray is limited to 2^31-1 (see struct nsTArrayHeader). If anyone raises that limit, this loop might no longer work properly.
Assignee | ||
Comment 13•8 years ago
|
||
That's fine, you would OOM long before 2^31-1 capture stream, because that means you would have 2^31-1 HTMLMediaElement or AudioContext playing.
Comment 14•8 years ago
|
||
Comment on attachment 8796124 [details] [diff] [review] patch Check it in!
Attachment #8796124 -
Flags: sec-approval? → sec-approval+
Updated•8 years ago
|
status-firefox49:
--- → wontfix
status-firefox50:
--- → disabled
status-firefox51:
--- → disabled
status-firefox52:
--- → disabled
status-firefox-esr45:
--- → disabled
tracking-firefox52:
--- → +
Assignee | ||
Updated•8 years ago
|
Rank: 35
Priority: -- → P3
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1858c9bbbc83
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 16•8 years ago
|
||
Why is this code behind a pref? Is it just for debugging? Something we abandoned and are going to remove? Or is it a potential future-feature?
Flags: needinfo?(padenot)
Assignee | ||
Comment 17•8 years ago
|
||
Yes, a potential future feature that got de-prioritized for now.
Flags: needinfo?(padenot)
Updated•8 years ago
|
Group: media-core-security → core-security-release
Updated•8 years ago
|
Flags: sec-bounty? → sec-bounty+
Keywords: sec-other → sec-moderate
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Group: core-security-release
Updated•4 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•