Closed
Bug 1106596
(CVE-2015-0813)
Opened 10 years ago
Closed 10 years ago
heap-use-after-free at AppendElements
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: aki.helin, Assigned: eflores)
References
Details
(4 keywords, Whiteboard: [adv-main37+][adv-esr31.6+] "fixed" by blacklisting flump3dec in bug 981869)
Attachments
(4 files)
ASan spots a heap use after free whenever the attached page, which plays an mp3 file, is opened. This happens at least in Linux when using recent tinderbox builds.
To reproduce, download ff-uaf-AppendElements.{html,mp3} and $ firefox ff-uaf-AppendElements.html.
==3278==ERROR: AddressSanitizer: heap-use-after-free on address 0x6210006b7cff at pc 0x7f83ba18da6e bp 0x7f837e9f15b0 sp 0x7f837e9f15a8
READ of size 9216 at 0x6210006b7cff thread T58 (Media Audio)
#0 0x7f83ba18da6d in AppendElements /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/AudioStream.h:119
#1 0x7f83ba18a951 in PlayFromAudioQueue /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/AudioSink.cpp:323
#2 0x7f83ba18886b in AudioLoop /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/AudioSink.cpp:193
#3 0x7f83ba1ae020 in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/media/../../dist/include/nsThreadUtils.h:388
#4 0x7f83b62b0804 in ProcessNextEvent /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:830
#5 0x7f83b630f83a in NS_ProcessNextEvent /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:265
#6 0x7f83b6b2b089 in Run /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/glue/MessagePump.cpp:339
#7 0x7f83b6ad82dc in RunInternal /builds/slave/m-cen-l64-asan-000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:233
#8 0x7f83b62ad295 in ThreadFunc /builds/slave/m-cen-l64-asan-000000000000000/build/src/xpcom/threads/nsThread.cpp:350
#9 0x7f83c3948405 in _pt_root /builds/slave/m-cen-l64-asan-000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
#10 0x7f83c6e80181 in start_thread /build/buildd/eglibc-2.19/nptl/pthread_create.c:312 (discriminator 2)
#11 0x7f83c5f72fbc in clone /build/buildd/eglibc-2.19/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:111
![]() |
||
Updated•10 years ago
|
Component: DOM → Video/Audio
Updated•10 years ago
|
Keywords: csectype-uaf,
sec-critical
Comment 2•10 years ago
|
||
Anthony, is there someone on your team that can work on this? (Can you recreate it?)
Flags: needinfo?(ajones)
Updated•10 years ago
|
Flags: needinfo?(ajones)
Updated•10 years ago
|
Flags: needinfo?(ajones)
Comment 3•10 years ago
|
||
Maire - does it make sense for padenot to look into this?
Flags: needinfo?(mreavy)
Comment 4•10 years ago
|
||
This looks like it should go to Padenot or Karlt.
Paul -- Can you take this one?
Assignee: nobody → padenot
Flags: needinfo?(mreavy)
Comment 5•10 years ago
|
||
This looks like a missing addref somewhere, we are trying to push freed data to the AudioStream circular buffer.
Comment 6•10 years ago
|
||
This is probably caused by the Promise-y refactoring of the state machine, in which case, I know nothing about it, and I'm sure it's will only take a couple minutes to cpearce or bholley to find the cause. It looks like the MediaQueue does not addref the AudioData*, and then it gets freed? It's not clear how we want to fix this.
Flags: needinfo?(cpearce)
Comment 7•10 years ago
|
||
Matt: Can you take this bug?
Flags: needinfo?(cpearce) → needinfo?(matt.woodrow)
Comment 8•10 years ago
|
||
Looking at the logs, it looks like the free happens when the resolve runnable is destroyed. That runnable holds an nsRefPtr<AudioData> which it passes to MediaDecoderStateMachine::OnAudioDecoded(AudioData*).
The fact that the the AudioData gets destroyed when the runnable pops off the stack presumably means that the MDSM did not hold a strong reference to the audio data. But the fact that the AudioSink is hitting this when accessing the MDSM's mAudioQueue implies that the data did go into the queue (which should addref it). So what gives?
Matt, I'm happy to debug this, but I'd need to spin up a linux VM. If you've got one and want to jump on it, feel free. Otherwise, let me know and I'll dig in and fix it.
Comment 9•10 years ago
|
||
The UAF appears to be on the mAudioData member of AudioData, not the object itself (I'd expect us to have hit the error when dereferencing audio in PlayFromAudioQueue otherwise).
That would suggest that we're sharing the same array pointer between multiple AudioData objects, but I can't see how that would happen either.
I don't have a linux vm handy either, so feel free to take this.
Flags: needinfo?(matt.woodrow)
Comment 10•10 years ago
|
||
I'm having trouble reproducing.
padenot - I don't have much experience with ASAN builds. I've got an ubuntu VM, and I downloaded ASAN builsd from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/latest/ (and the non-debug version). I modified the testcase to have the proper audio filename (which it doesn't), and then navigated to the file. Nothing happens. Am I doing something dumb?
Flags: needinfo?(padenot)
Reporter | ||
Comment 11•10 years ago
|
||
Does it try to play the mp3? I downloaded the latest build and checked if this has been fixed accidentally or something, but the same thing happened here: firefox starts to play the mp3, and after less than a second the tab dies and ASAN report is printed to stderr. This does not happen if one triest to play just the mp3 file. I have an up-to-date server edition Ubuntu. There could be some difference in the external mp3 codecs used by Firefox.
Comment 12•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #10)
> I'm having trouble reproducing.
>
> padenot - I don't have much experience with ASAN builds. I've got an ubuntu
> VM, and I downloaded ASAN builsd from
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> central-linux64-asan-debug/latest/ (and the non-debug version). I modified
> the testcase to have the proper audio filename (which it doesn't), and then
> navigated to the file. Nothing happens. Am I doing something dumb?
That should work, although I always do my own builds, attached is a mozconfig that works on ubuntu.
Make sure to have clang and libvpx-dev installed:
> sudo aptitude build-dep firefox
> sudo aptitude install clang libvpx-dev
to get all the deps
Then, run it in gdb, and set a breakpoint at `__asan_report_error`, so gdb break right before asan kills the process. You can do:
> p __asan_describe_address(0xsomeaddress)
to get infos. You might need to use asan_symbolize.py [0] and c++filt (aptitude install it) to have readable stacks (they both take their input from stdin).
It was around 100% repro for me with this setup, but I could not repro with a tree that was around 1-2 weeks old, I had to update to current tip.
[0]: http://llvm.org/klaus/compiler-rt/raw/release_35/lib/asan/scripts/asan_symbolize.py
Flags: needinfo?(padenot)
Flags: needinfo?(ajones)
Updated•10 years ago
|
status-firefox37:
--- → affected
Keywords: regression
Comment 13•10 years ago
|
||
Figured it out - I needed to install gstreamer0.10-fluendo-plugins-mp3-partner, which is presumably something that most linux users have already done years ago, but christmas-easter VM users haven't.
Comment 14•10 years ago
|
||
So, the problem is that GStreamerReader::AudioPreroll gets invoked twice for one file - and receives different a different channel count each time (first 2, then 1). I think the second preroll is triggered because we seek. So anyway, we end up with AudioData entries in the MDSM's AudioQueue with 1 channel, but an AudioSink that thinks there are two channels, because we never re-initialized it.
When MediaDecoderStateMachine::DropAudioUpToSeekTarget encounters this 1-channel frame, it copies the buffer into a new (1-channeled) AudioData, and sticks that at the front of the AudioQueue. Then, AudioSink passes the data to its mAudioStream, and tells it there are 2 channels worth of data, whereas in reality there's only 1.
I'm not sure whether changing the number of channels is something we want to support or not - if so, we should update mInfo on the AudioSink, or even better, stop duplicating this state in three (!!!) different places (MDSM, MDR, and AudioSink).
Regardless, this is a bug in our gstreamer backend and not in any of the new asynchronous architecture. Back over to paul.
Flags: needinfo?(padenot)
Comment 15•10 years ago
|
||
Bobby says it's a gstreamer issue. It probably makes sense for Ralph or Edwin to look into it. You guys can figure out between you who wants to deal with this.
Flags: needinfo?(padenot)
Flags: needinfo?(giles)
Flags: needinfo?(edwin)
Comment 16•10 years ago
|
||
This isn't a padenot bug anymore: Apparently it's rillian or edwin
Assignee: padenot → nobody
Comment 17•10 years ago
|
||
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #14)
> I'm not sure whether changing the number of channels is something we want to
> support or not - if so, we should update mInfo on the AudioSink, or even
> better, stop duplicating this state in three (!!!) different places (MDSM,
> MDR, and AudioSink).
We need to support the number of channels changing for AAC streams; HE-AAC of some description can report that they have 1 channel, and then we end up getting 2 once we've decoded the first sample.
This shouldn't be necessary for MP3 however.
Comment 18•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) (PTO until 5 Jan) from comment #17)
> (In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect
> things) from comment #14)
> > I'm not sure whether changing the number of channels is something we want to
> > support or not - if so, we should update mInfo on the AudioSink, or even
> > better, stop duplicating this state in three (!!!) different places (MDSM,
> > MDR, and AudioSink).
>
>
> We need to support the number of channels changing for AAC streams; HE-AAC
> of some description can report that they have 1 channel, and then we end up
> getting 2 once we've decoded the first sample.
If we do do that, we need to be very careful, because we need to handle the case where we're processing a previously-appended audio sample whose baked-in channel count is different than the one currently used by the AudioSink etc.
Comment 19•10 years ago
|
||
We already handle this for the MP4Reader; we already delay the creation of the AudioSink's audio device until after the first sample has been decoded; the audio rate/num-channels can update MP4Reader::mInfo, and the state machine calls MediaDecoderReader::ReadUpdatedMetadata() after the first frame is decoded to ensure the channel count is accurate.
Maybe the GStreamerReader just needs to detect channel num changes and report them in ReadUpdatedMetadata()? I'm a bit iffy about that, as the channel count should not change for MP3...
Comment 20•10 years ago
|
||
Can we make it fail gracefully for now and fix it in a follow up bug?
Comment 21•10 years ago
|
||
I'm happy to take a look in January if Edwin doens't want to. I likely won't have time to before.
Comment 22•10 years ago
|
||
rillian: are you available to look at this now?
Updated•10 years ago
|
status-firefox38:
--- → affected
tracking-firefox38:
--- → +
Comment 23•10 years ago
|
||
During CritSmash, assigning to Ralph and emailing for confirmation.
Assignee: nobody → giles
Comment 24•10 years ago
|
||
Thanks for being executive. I guess Edwin and I were both hoping the other would step up.
I've been too busy with the MSE project to look at much else, but that's easing up. I'll see if I can reproduce tomorrow and find a quick fix.
Flags: needinfo?(giles)
Comment 25•10 years ago
|
||
If this is also affecting 37, we should track there too and try to get a fix into Aurora (unless there are string changes involved).
Ralph - any new info on your attempts to reproduce?
tracking-firefox37:
--- → +
Flags: needinfo?(giles)
Updated•10 years ago
|
Group: media-core-security
Updated•10 years ago
|
Assignee: giles → edwin
Flags: needinfo?(giles)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(edwin)
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 26•10 years ago
|
||
The main problem here is that the Fluendo MP3 plugin is total garbage. Blacklisting in bug 981869.
(must be the 10th time I've typed that this week...)
Assignee | ||
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Updated•10 years ago
|
Flags: sec-bounty?
Comment 29•10 years ago
|
||
Edwin, does this means that our invariant is that we should never trigger a second preroll?
Flags: needinfo?(edwin)
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #29)
> Edwin, does this means that our invariant is that we should never trigger a
> second preroll?
Ideally. However it doesn't particularly matter as long as the metadata returned is consistent, which should be the case given the same decoder and media.
Flags: needinfo?(edwin)
Comment 31•10 years ago
|
||
"fixed" is probably a better resolution, so in case the fix gets backed out we'll remember it was preventing an exploitable hole.
Flags: sec-bounty? → sec-bounty+
Resolution: DUPLICATE → FIXED
Whiteboard: "fixed" by blacklisting flump3dec in bug 981869
Updated•10 years ago
|
Updated•10 years ago
|
Group: media-core-security
Comment 32•10 years ago
|
||
Bug 981869 is marked as fixed but the uplift request was pulled. As 37 is affected, do we need to uplift the fix from bug 981869 to 37?
Flags: needinfo?(dveditz)
Comment 33•10 years ago
|
||
Taking the fix in Fx37 is certainly the security team's recommendation. We might need this on ESR31, too.
Comment 34•10 years ago
|
||
From the status flags on bug 981869 it looks like ESR 31 is indeed affected. Note that although bug 981869 is in the resolved state, it seems that there is some fallout that needs to be addressed before it is uplifted.
Edwin - I'm setting ni on you in both bugs so that you can comment here on whether we should expect the fix in 37 and comment on the feasibility of backporting to ESR 31.
Flags: needinfo?(edwin)
Comment 35•10 years ago
|
||
From Edwin's comment in bug 981869, we need the fix from bug 1133634 as well.
Edwin - Can you please get the approval requests on these bugs so that we can uplift before Monday?
Comment 36•10 years ago
|
||
Drive-By Comment: Per Comment 34 Tracking for 37+ and marking affected ESR
Updated•10 years ago
|
Target Milestone: --- → mozilla38
Assignee | ||
Comment 37•10 years ago
|
||
No action needed on this bug. Clearing ni?.
Flags: needinfo?(edwin)
Updated•10 years ago
|
Flags: needinfo?(edwin)
Updated•10 years ago
|
Whiteboard: "fixed" by blacklisting flump3dec in bug 981869 → [adv-main37+] "fixed" by blacklisting flump3dec in bug 981869
Comment 39•10 years ago
|
||
(In reply to Al Billings [:abillings] from comment #38)
> This missed the upcoming ESR31 release?
Ben - Is this correct? If so, we should consider respinning the ESR RC to pick up this fix tomorrow.
Flags: needinfo?(lmandel) → needinfo?(bkerensa)
Comment 40•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #39)
> (In reply to Al Billings [:abillings] from comment #38)
> > This missed the upcoming ESR31 release?
>
> Ben - Is this correct? If so, we should consider respinning the ESR RC to
> pick up this fix tomorrow.
Right if we can get an approval request from Edwin first thing tomorrow I would agree with respinning.
Flags: needinfo?(bkerensa)
Updated•10 years ago
|
Alias: CVE-2015-0813
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(edwin)
Comment 41•10 years ago
|
||
Are we not getting an ESR31 patch for this? I thought we were hoping to respin ESR31 with this fix.
Flags: needinfo?(edwin)
Comment 42•10 years ago
|
||
I also thought this fix was in build#2 but I don't see the changeset listed in the esr31 repo.
bkerensa - Can you confirm?
Flags: needinfo?(bkerensa)
Assignee | ||
Comment 43•10 years ago
|
||
(In reply to Al Billings [:abillings] from comment #41)
> Are we not getting an ESR31 patch for this? I thought we were hoping to
> respin ESR31 with this fix.
Yes. Bug 981869 and bug 1133634 have been uplifted to ESR31. There are no patches on this bug.
Flags: needinfo?(edwin)
Comment 44•10 years ago
|
||
Ah, thanks.
Updated•10 years ago
|
Flags: needinfo?(bkerensa)
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [adv-main37+] "fixed" by blacklisting flump3dec in bug 981869 → [adv-main37+][adv-esr31.6+] "fixed" by blacklisting flump3dec in bug 981869
Comment 45•10 years ago
|
||
I can`t reproduce the original issue since there are no asan builds older then 2015-02-28 thus I can`t verify if this is fixed.
Aki, would you mind verifying on Firefox 37 RC, Aurora 38 and ESR 31.6.0?
Flags: needinfo?(aki.helin)
Reporter | ||
Comment 46•10 years ago
|
||
Sorry for the delay - it took a while to get a setup and find an old version where the bug reproduces for reference.
FAIL: firefox-37.0a1.en-US.linux-x86_64-asan.tar.bz2 from january
OK: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/latest/firefox-40.0a1.en-US.linux-x86_64-asan.tar.bz2
OK: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/latest/firefox-39.0a2.en-US.linux-x86_64-asan.tar.bz2
OK: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/latest/firefox-38.0.en-US.linux-x86_64-asan.tar.bz2
OK: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-release-linux64-asan/latest/firefox-37.0.en-US.linux-x86_64-asan.tar.bz2
I didn't find suitable ESR asan tinderbox prebuilds, so that hasn't been tested.
Flags: needinfo?(aki.helin)
Comment 47•10 years ago
|
||
Thanks Aki! Updating status flags.
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•