Closed Bug 1077274 Opened 6 years ago Closed 6 years ago

Dead object dereference if <video> GC'd before page closes

Categories

(Core :: WebRTC: Audio/Video, defect)

33 Branch
defect
Not set
normal

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)

Attached file stream_crash.html
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.
Do you have a stack trace?
#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
#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.
MediaTrackListListener keeps a raw pointer to MediaTrackList object, and nothing ever clears that
raw pointer.
Summary: Null dereference if <video> GC'd before page closes → Dead object dereference if <video> GC'd before page closes
Attached patch Clean up tracklists (obsolete) — Splinter Review
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
[Tracking Requested - why for this release]:
sec-crit UAF with reproducible pattern

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bbab49e96695
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: nobody → rjesup
OS: Mac OS X → All
Hardware: x86 → All
Version: 32 Branch → 33 Branch
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+
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.
Attached file stream_crash.html
Nastier testcase that verifies that other streams stay healthy if you remove one.

Crashes locally without the patch
From inspection the regressing code was added in Bug 744896, which is mozilla33.
Verified NOT in 32.0.1.

Regression from bug 744896 which landed in 33, as jib indicates
Blocks: 744896
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=430086ea589e 
for the linux debug versions (forward class ref was needed)
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?
final patch as try'd
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?
Keywords: regression
Attachment #8499806 - Flags: sec-approval?
Attachment #8499806 - Flags: review+
Attachment #8499806 - Flags: approval-mozilla-beta?
Attachment #8499806 - Flags: approval-mozilla-aurora?
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.
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+
https://hg.mozilla.org/mozilla-central/rev/527003361b21
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1078017
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.
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
Group: core-security
You need to log in before you can comment on or make changes to this bug.