Closed
Bug 1077274
Opened 10 years ago
Closed 10 years ago
Dead object dereference if <video> GC'd before page closes
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
VERIFIED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox32 | --- | unaffected |
firefox33 | + | verified |
firefox34 | + | verified |
firefox35 | + | verified |
firefox-esr31 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | fixed |
b2g-v2.2 | --- | fixed |
People
(Reporter: abr, Assigned: jesup)
References
Details
(Keywords: csectype-uaf, regression, sec-critical)
Attachments
(3 files, 1 obsolete file)
1022 bytes,
text/html
|
Details | |
1.68 KB,
text/html
|
Details | |
5.28 KB,
patch
|
jesup
:
review+
dveditz
:
approval-mozilla-aurora+
dveditz
:
approval-mozilla-beta+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
In trying to fix some usability issues in Loop, I found a very strange and trivial-to-trigger crash that affects Beta through m-c. I cannot replicate the issue on Firefox 32. I've isolated the behavior to the following conditions: 1) Call gUM with {video:true} (doesn't matter if it's real or fake media) 2) Attach the media stream to a <video> tag 3) Remove the <video> tag from the DOM and forget all references to its DOM node 4) Wait for (or manually trigger) GC 5) Close the page This reliably triggers a crash in DOMMediaStream::NotifyMediaStreamTrackEnded. Most usually, the crash is a null deref inside nsTArray, although I've seen occasional variations on the stack while trying to isolate a minimal set of triggering conditions. This leads me to believe that the issue can sometimes be a wild pointer read (and maybe write?) under the right circumstances, so I think we need to keep it SS until we understand the root cause and can rule out exploitability. The attached file will reliably crash Firefox 33 and up.
Comment 1•10 years ago
|
||
Do you have a stack trace?
Comment 2•10 years ago
|
||
#0 0x00007f3c357e16ed in nanosleep () from /lib64/libc.so.6 #1 0x00007f3c357e1584 in sleep () from /lib64/libc.so.6 #2 0x00007f3c30c6cb6d in ah_crap_handler (signum=11) at /home/smaug/mozilla/hg/inbound2/toolkit/xre/nsSigHandlers.cpp:89 #3 0x00007f3c30c49f31 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7fffe6a39ab0, context=0x7fffe6a39980) at /home/smaug/mozilla/hg/inbound2/profile/dirserviceprovider/nsProfileLock.cpp:190 #4 0x00007f3c31aa403f in AsmJSFaultHandler (signum=11, info=0x7fffe6a39ab0, context=0x7fffe6a39980) at /home/smaug/mozilla/hg/inbound2/js/src/asmjs/AsmJSSignalHandlers.cpp:978 #5 <signal handler called> #6 0x00007f3c2cd145af in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length (this=0x7f3bf89398b0) at ../../dist/include/nsTArray.h:329 #7 0x00007f3c2facee4b in mozilla::dom::MediaTrackList::GetTrackById (this=0x7f3bf8939860, aId=...) at /home/smaug/mozilla/hg/inbound2/content/media/MediaTrackList.cpp:83 #8 0x00007f3c2facedca in mozilla::dom::MediaTrackListListener::NotifyMediaTrackEnded (this=0x7f3bf86c15f8, aId=...) at /home/smaug/mozilla/hg/inbound2/content/media/MediaTrackList.cpp:40 #9 0x00007f3c2fa7bccb in mozilla::DOMMediaStream::NotifyMediaStreamTrackEnded (this=0x7f3bf8b499a0, aTrack=0x7f3bf860ff20) at /home/smaug/mozilla/hg/inbound2/content/media/DOMMediaStream.cpp:485 #10 0x00007f3c2fa9c807 in mozilla::DOMMediaStream::StreamListener::TrackChange::Run (this=0x7f3bf3572d00) at /home/smaug/mozilla/hg/inbound2/content/media/DOMMediaStream.cpp:97 #11 0x00007f3c2ce3ca68 in nsThread::ProcessNextEvent (this=0x7f3c3565dc70, aMayWait=false, aResult=0x7fffe6a3a2ee) at /home/smaug/mozilla/hg/inbound2/xpcom/threads/nsThread.cpp:830 #12 0x00007f3c2ce8b697 in NS_ProcessNextEvent (aThread=0x7f3c3565dc70, aMayWait=false) at /home/smaug/mozilla/hg/inbound2/xpcom/glue/nsThreadUtils.cpp:265
Comment 3•10 years ago
|
||
#6 0x00007f3c2cd145af in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length (this=0x7f3bf89398b0) at ../../dist/include/nsTArray.h:329 329 size_type Length() const { return mHdr->mLength; } (gdb) p mHdr $1 = (Header *) 0x5a5a5a5a5a5a5a5a Doesn't look good.
Comment 4•10 years ago
|
||
MediaTrackListListener keeps a raw pointer to MediaTrackList object, and nothing ever clears that raw pointer.
Updated•10 years ago
|
Summary: Null dereference if <video> GC'd before page closes → Dead object dereference if <video> GC'd before page closes
Assignee | ||
Comment 5•10 years ago
|
||
I'd elide comments if we're not landing this across all affected branches at once. Still not sure it's not on 32 or older; I'm going to build a crueler test
Assignee | ||
Updated•10 years ago
|
Keywords: sec-critical
Assignee | ||
Comment 6•10 years ago
|
||
[Tracking Requested - why for this release]: sec-crit UAF with reproducible pattern https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bbab49e96695
status-b2g-v2.0:
--- → ?
status-b2g-v2.1:
--- → affected
status-firefox32:
--- → ?
status-firefox33:
--- → affected
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox-esr31:
--- → ?
tracking-firefox33:
--- → ?
tracking-firefox34:
--- → ?
tracking-firefox35:
--- → ?
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8499693 [details] [diff] [review] Clean up tracklists perhaps better long-term would be fixing this to not need such an explicit clear (WeakReference, or strong references (including new ones from the media element) plus cycle handling (which in my mind is almost as bad as explicit disconnects, or maybe worse because it's so arcane/non-obvious). r? to roc as well, but no guarantee he's around to look.
Attachment #8499693 -
Flags: review?(roc)
Attachment #8499693 -
Flags: review?(jib)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rjesup
OS: Mac OS X → All
Hardware: x86 → All
Version: 32 Branch → 33 Branch
Comment 8•10 years ago
|
||
Comment on attachment 8499693 [details] [diff] [review] Clean up tracklists Review of attachment 8499693 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits ::: content/html/content/src/HTMLMediaElement.cpp @@ +2865,5 @@ > if (container) { > GetSrcMediaStream()->AddVideoOutput(container); > } > > + // Note: we must mSrcStream->DisconnectTrackListListeners(AudioTracks(), VideoTracks()); before "must call" and wordwrap comment more @@ +2879,5 @@ > } > > void HTMLMediaElement::EndSrcMediaStreamPlayback() > { > + mSrcStream->DisconnectTrackListListeners(AudioTracks(), VideoTracks()); Since EndSrcMediaStreamPlayback is never called with !mSrcStream, it seems OK to call AudioTracks() and VideoTracks() without worrying they'll accidentally create XxxTrackList objects just to remove them. I just find sibling member-function use like this a little hard to follow. I would add an NS_ASSERTION(mSrcStream) ahead of this line, or... @@ +2885,1 @@ > MediaStream* stream = GetSrcMediaStream(); ... move the new code below this existing GetSrcMediaStream() call which does the assert implicitly. ::: content/media/MediaTrackList.h @@ +27,5 @@ > */ > class MediaTrackListListener > { > public: > + friend class mozilla::DOMMediaStream; MediaTrackListListener.Remove() might have been cleaner but slower, so yeah works for me.
Attachment #8499693 -
Flags: review?(jib) → review+
Comment 9•10 years ago
|
||
We need to determine if this impacts Firefox 31 ESR and B2G 2.0. Adam said in the description that he can't reproduce on 32, which makes it seem like this is new in 33 but I would like to know for certain before marking 32 and older releases as unaffected.
Assignee | ||
Comment 10•10 years ago
|
||
Nastier testcase that verifies that other streams stay healthy if you remove one. Crashes locally without the patch
Comment 11•10 years ago
|
||
From inspection the regressing code was added in Bug 744896, which is mozilla33.
Comment 12•10 years ago
|
||
That is http://hg.mozilla.org/mozilla-central/rev/6e7b48d0357c
Assignee | ||
Comment 13•10 years ago
|
||
Verified NOT in 32.0.1. Regression from bug 744896 which landed in 33, as jib indicates
Blocks: 744896
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 14•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=430086ea589e for the linux debug versions (forward class ref was needed)
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8499693 [details] [diff] [review] Clean up tracklists (And request for aurora/beta) [Security approval request comment] How easily could an exploit be constructed based on the patch? not super-hard if you can exploit a through-UAF function call. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Something of a bullseye; I could removed the MUST call this before that! comments Which older supported branches are affected by this flaw? 33 and later If not all supported branches, which bug introduced the flaw? Bug 744896 (add audioTracks() and videoTracks() to HTMLMediaElements) Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I'll port the patch; backports should be trivial. How likely is this patch to cause regressions; how much testing does it need? This should be pretty safe; biggest concern would be issues with removing the right entries from the list, which was why I created a testcase with 6 <video>s run from one stream.
Attachment #8499693 -
Flags: sec-approval?
Attachment #8499693 -
Flags: approval-mozilla-beta?
Attachment #8499693 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Keywords: csectype-uaf
Assignee | ||
Comment 16•10 years ago
|
||
final patch as try'd
Assignee | ||
Updated•10 years ago
|
Attachment #8499693 -
Attachment is obsolete: true
Attachment #8499693 -
Flags: sec-approval?
Attachment #8499693 -
Flags: review?(roc)
Attachment #8499693 -
Flags: approval-mozilla-beta?
Attachment #8499693 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Keywords: regression
Assignee | ||
Updated•10 years ago
|
Attachment #8499806 -
Flags: sec-approval?
Attachment #8499806 -
Flags: review+
Attachment #8499806 -
Flags: approval-mozilla-beta?
Attachment #8499806 -
Flags: approval-mozilla-aurora?
Comment 17•10 years ago
|
||
Comment on attachment 8499806 [details] [diff] [review] Clean up tracklists [Triage Comment] sec-approval+ dveditz, and we need this on beta/aurora too: a=dveditz I don't think we're going to get any more betas so let's get this on aurora/nightly ASAP to get as much testing as possible.
Updated•10 years ago
|
Attachment #8499806 -
Flags: sec-approval?
Attachment #8499806 -
Flags: sec-approval+
Attachment #8499806 -
Flags: approval-mozilla-beta?
Attachment #8499806 -
Flags: approval-mozilla-beta+
Attachment #8499806 -
Flags: approval-mozilla-aurora?
Attachment #8499806 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/527003361b21 https://hg.mozilla.org/releases/mozilla-aurora/rev/247712164e37 https://hg.mozilla.org/releases/mozilla-beta/rev/f0253d7268bb
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/527003361b21
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 20•10 years ago
|
||
Reproduced the original issue several times using the following: - using both poc's from comment #0 and comment #10 - Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0b9/ Went through the following verification: fx35 - Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-06-03-02-02-mozilla-central/ - Win 8 x64 (VM): PASSED - Ubuntu 14.0.4.1 x64 (VM): PASSED - OSX 10.9.5 x64: PASSED fx34: - Build Used: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-06-00-40-57-mozilla-aurora/ - Win 8 x64 (VM): PASSED - Ubuntu 14.0.4.1 x64 (VM): PASSED - OSX 10.9.5 x64: PASSED For all of the above channels, the following test cases were used: - Opened both poc several times in a normal window/tab - Opened both poc several times in a private window/tab - Opened both poc several times in a e10s window/tab's (only on m-c) I'll finish verification with fx33 once a build with the fix becomes available. Currently, 33.0b9 still has the issue.
Comment 21•10 years ago
|
||
Reproduced the original issue several times using the following: - using both poc's from comment #0 and comment #10 - http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/33.0b9-candidates/build1/ Went through the following verification: - Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/33.0-candidates/ - Win 8 x64 (VM): PASSED - Ubuntu 14.0.4.1 x64 (VM): PASSED - OSX 10.9.5 x64: PASSED Used the same test cases from comment #20.
Status: RESOLVED → VERIFIED
Updated•10 years ago
|
status-b2g-v2.2:
--- → fixed
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•