Closed Bug 955911 Opened 6 years ago Closed 6 years ago

Heap-use-after-free in mozilla::MediaStream::ChangeExplicitBlockerCount

Categories

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

x86_64
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox27 --- unaffected
firefox28 + verified
firefox29 --- verified
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: inferno, Assigned: padenot)

Details

(Keywords: csectype-uaf, sec-critical)

Attachments

(4 files)

Attached file Testcase
==25265==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000109790 at pc 0x7f93a8898469 bp 0x7f9389bd6310 sp 0x7f9389bd6308
READ of size 8 at 0x614000109790 thread T36 (MediaStreamGrph)
    #0 0x7f93a8898468 in mozilla::MediaStream::ChangeExplicitBlockerCount(int)::Message::Run() content/media/MediaStreamGraph.cpp:1656
    #1 0x7f93a886f10e in mozilla::MediaStreamGraphImpl::RunThread() content/media/MediaStreamGraph.cpp:1127
    #2 0x7f93a8899529 in mozilla::(anonymous namespace)::MediaStreamGraphInitThreadRunnable::Run() content/media/MediaStreamGraph.cpp:1350
    #3 0x7f93a4f7abcb in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:634
    #4 0x7f93a4e56ae1 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
    #5 0x7f93a57733e1 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:301
    #6 0x7f93a56e38d3 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:226
    #7 0x7f93a4f779a3 in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:258
    #8 0x7f93b1dd7407 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:205
    #9 0x43bf60 in __asan::AsanThread::ThreadStart(unsigned long) _asan_rtl_
    #10 0x7f93b6367e99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308
    #11 0x7f93b54763fc in ?? /build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112
0x614000109790 is located 336 bytes inside of 392-byte region [0x614000109640,0x6140001097c8)
freed by thread T36 (MediaStreamGrph) here:
    #0 0x433c25 in __interceptor_free _asan_rtl_
    #1 0x7f93a888f5af in nsTArray_Impl<nsRefPtr<mozilla::MediaStream>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) content/media/MediaStreamGraph.h:291
previously allocated by thread T0 here:
    #0 0x433d85 in malloc _asan_rtl_
    #1 0x7f93af3f1408 in moz_xmalloc memory/mozalloc/mozalloc.cpp:52
Thread T36 (MediaStreamGrph) created by T0 here:
    #0 0x414c9e in __interceptor_pthread_create _asan_rtl_
    #1 0x7f93b1dd3378 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:445
    #2 0x7f93b1dd2ec7 in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:528
Shadow bytes around the buggy address:
  0x0c28800192a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c28800192b0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c28800192c0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c28800192d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c28800192e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c28800192f0: fd fd[fd]fd fd fd fd fd fd fa fa fa fa fa fa fa
  0x0c2880019300: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c2880019310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2880019320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2880019330: 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa
  0x0c2880019340: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap right redzone:    fb
  Freed heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==25265==ABORTING
roc, do you have any suggestions for who might be able to look at this?  Thanks.
Flags: needinfo?(roc)
padenot maybe
Assignee: nobody → padenot
Flags: needinfo?(roc)
abhishek: What version did that ASAN report come from?
abhishek: What version did that ASAN report come from?
(lost the needinfo in a collision with roc)
Flags: needinfo?(inferno)
Severity: normal → critical
(In reply to Randell Jesup [:jesup] from comment #5)
> abhishek: What version did that ASAN report come from?

Clang revision r188888 and firefox revision 20140104081712
Flags: needinfo?(inferno)
Attached patch r=Splinter Review
So, this happens, because we have a setup like this:

- We have a MediaElement, whose source is being captured to a MediaStream.
- We abort its loading (src is replaced or something).
- This schedules a ::Destroy on the MediaStream

#0  mozilla::MediaStream::Destroy (this=0x7fffca817f00) at /home/padenot/src/trees/mozilla-central/content/media/MediaStreamGraph.cpp:1764
#1  0x00007ffff193644f in mozilla::MediaDecoder::DecodedStreamData::~DecodedStreamData (this=0x7fffc8473260, __in_chrg=<optimized out>)
    at /home/padenot/src/trees/mozilla-central/content/media/MediaDecoder.cpp:210
#2  0x00007ffff1949ce0 in nsAutoPtr<mozilla::MediaDecoder::DecodedStreamData>::assign (this=0x7fffca904a18, newPtr=0x0) at ../../dist/include/nsAutoPtr.h:45
#3  0x00007ffff19457ed in nsAutoPtr<mozilla::MediaDecoder::DecodedStreamData>::operator= (this=0x7fffca904a18, rhs=0x0) at ../../dist/include/nsAutoPtr.h:115
#4  0x00007ffff1936879 in mozilla::MediaDecoder::DestroyDecodedStream (this=0x7fffca904980) at /home/padenot/src/trees/mozilla-central/content/media/MediaDecoder.cpp:281
#5  0x00007ffff193740a in mozilla::MediaDecoder::Shutdown (this=0x7fffca904980) at /home/padenot/src/trees/mozilla-central/content/media/MediaDecoder.cpp:462
#6  0x00007ffff1883980 in mozilla::dom::HTMLMediaElement::ShutdownDecoder (this=0x7fffcc735000)
    at /home/padenot/src/trees/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:584
#7  0x00007ffff1883a39 in mozilla::dom::HTMLMediaElement::AbortExistingLoads (this=0x7fffcc735000)
    at /home/padenot/src/trees/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:601
[...]

Then, at roughly the same time,
MediaDecoder::UpdateStreamBlockingForStateMachinePlaying gets called, and
MediaStream::ChangeExplicitBlockerCount gets queued up [1], because we are not on
the main thread, and so we can't directly send a message to the graph.

Then, in the same MessageBlock, we have
- A destroy message
- A change explicit blocker count message

hence the use after free.

[1]: http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp#303
Attachment #8359283 - Flags: review?(roc)
fixd on central https://hg.mozilla.org/mozilla-central/rev/367027357fb5
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Paul: how far back does this bug go? A severe (or unrated) security patch needs sec-approval before landing: https://wiki.mozilla.org/Security/Bug_Approval_Process

Given the simple patch and code comments giving the triggering condition we should get this patch landed on other affected branches ASAP.
status-b2g18: --- → ?
status-b2g-v1.2: --- → ?
status-b2g-v1.3: --- → ?
tracking-b2g18: --- → ?
Flags: needinfo?(paul)
Daniel, I just tested current release, current beta, current aurora, and current nightly (that has the fix from this bug), using ASAN builds for each , and I can't repro. I would expect this bug to be a fallout from the recent big changes in the HTMLMediaElement/MediaStreamGraph ingration roc did a couple weeks ago.
Flags: needinfo?(paul)
Decoder: please run this testcase through your tinderbox-build bisector and see if we can pin down a regression range that would tell us the affected releases.
Flags: sec-bounty?
(In reply to Paul Adenot (:padenot) from comment #12)
> Daniel, I just tested current release, current beta, current aurora, and
> current nightly (that has the fix from this bug), using ASAN builds for each
> , and I can't repro. I would expect this bug to be a fallout from the recent
> big changes in the HTMLMediaElement/MediaStreamGraph ingration roc did a
> couple weeks ago.

Is there a tinderbox build that you /can/ repro on? Maybe you don't have it set up correctly. The only big changes I see from roc in MediaStreamGraph that you might be talking about is from bug 943461, but that didn't land until after this bug was filed.
Flags: needinfo?(paul)
The patch is in the middle of code that's quite old, would it be unsafe to just take it on old branches in any case? Or do we really need to nail down the regression range to do so safely?
Daniel, I didn't build myself, I grabbed the ASAN builds from tinderbox.
Flags: needinfo?(paul)
Waiting on confirmation if the previous releases are impacted to actually track this. Also just as an fyi, we go-to-build with one of last beta's today.
(In reply to Paul Adenot (:padenot) from comment #16)
> Daniel, I didn't build myself, I grabbed the ASAN builds from tinderbox.

I've got a windows box and can't use those ASAN builds. Comment 12 tells me which builds do NOT have the bug, but not which builds do. I assume you must have reproduced the crash at some point but you didn't say you did--it's possible you diagnosed this in the debugger without reproducing. If you didn't reproduce the crash at least once then I'm less confident in relying on your statement that Aurora didn't crash.

I'd be more confident if we could pin this down to the regressing bug, but roc's changes to this area seem to be either /after/ Abhishek reported the problem (bug 943461) or long enough ago that Aurora should have had the problem too. Probably there's some other bug you know about that I couldn't find, which is what I was asking for in comment 14
Flags: needinfo?(paul)
I'll do a mozregression run today to pinpoint exactly the changeset that introduced the crash. I could certainly repro in an ASAN build (that I built myself) 100% of the time before I fixed this, so that should not be too hard (the range being rather small).
Flags: needinfo?(paul)
Without having figuring out a regression range, this should not have been checked in trunk per policy with this rating.

https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: sec-bounty? → sec-bounty+
(In reply to Paul Adenot (:padenot) from comment #19)
> I'll do a mozregression run today to pinpoint exactly the changeset that
> introduced the crash. I could certainly repro in an ASAN build (that I built
> myself) 100% of the time before I fixed this, so that should not be too hard
> (the range being rather small).

NI on paul here to see if he has an update here. We have already gone to build with our final Release build for Fx27.0 but we could still uplift this on aurora if this impacts that branch.
Flags: needinfo?(paul)
Interestingly enough, I can't repro at all, using the gecko revision from comment 7, and I'm sure I could repro most of the time before.
Flags: needinfo?(paul)
Clearing tracking for now , please renominate if needed, in case we need to fix this on Fx28.
Does this affect Firefox 28? Did it affect 27?
Flags: needinfo?(inferno)
(In reply to Al Billings [:abillings] from comment #25)
> Does this affect Firefox 28? Did it affect 27?

I dont have the Firefox 28, 27 builds to verify this (i just build from trunk). Might want to check with Paul who fixed this.
Flags: needinfo?(inferno)
Matt, can you try reproducing this on older builds so we can figure out if this affects 28 or if it affected 27?
Flags: needinfo?(mwobensmith)
We mainly care about 28 here though but it needs to be a Linux ASAN build and you'll probably need to build it by hand.
Crashes 28, but not 27.
Flags: needinfo?(mwobensmith)
Thanks, Matt.
Can we get a patch for Beta for this?
Flags: needinfo?(paul)
Al, the attached patch applies cleanly on 28.
Flags: needinfo?(paul)
Comment on attachment 8359283 [details] [diff] [review]
r=

Approved for Beta (28). Get 'er in!
Attachment #8359283 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/84f6530e4f9c

I talked to Paul on IRC about backports to the older branches. While the patch applies cleanly all the way down to b2g18, the changes that actually made this function buggy were introduced in 28, so the patch won't actually be doing anything useful on them.
Matt, can you please evaluate whether this needs QA testing before we release Firefox 28?
Flags: needinfo?(mwobensmith)
Crashes FF29, 2013-12-19.
Verified fixed on FF28, FF29 2014-03-03.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mwobensmith)
Group: core-security
You need to log in before you can comment on or make changes to this bug.