Bug 1106596 (CVE-2015-0813)

heap-use-after-free at AppendElements

VERIFIED FIXED in Firefox 37

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: aki.helin, Assigned: eflores)

Tracking

({csectype-uaf, regression, sec-critical})

unspecified
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox37+ verified, firefox38+ verified, firefox39 verified, firefox40 verified, firefox-esr3137+ fixed)

Details

(Whiteboard: [adv-main37+][adv-esr31.6+] "fixed" by blacklisting flump3dec in bug 981869)

Attachments

(4 attachments)

Reporter

Description

5 years ago
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
Reporter

Comment 1

5 years ago
Component: DOM → Video/Audio
Anthony, is there someone on your team that can work on this? (Can you recreate it?)
Flags: needinfo?(ajones)
Flags: needinfo?(ajones)
Flags: needinfo?(ajones)
Maire - does it make sense for padenot to look into this?
Flags: needinfo?(mreavy)
This looks like it should go to Padenot or Karlt.

Paul -- Can you take this one?
Assignee: nobody → padenot
Flags: needinfo?(mreavy)
This looks like a missing addref somewhere, we are trying to push freed data to the AudioStream circular buffer.
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)
Matt: Can you take this bug?
Flags: needinfo?(cpearce) → needinfo?(matt.woodrow)
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.
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)
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

5 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.
Posted file mozconfig for asan
(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)
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.
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)
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)
This isn't a padenot bug anymore: Apparently it's rillian or edwin
Assignee: padenot → nobody
(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.
(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.
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...
Can we make it fail gracefully for now and fix it in a follow up bug?
I'm happy to take a look in January if Edwin doens't want to. I likely won't have time to before.
rillian: are you available to look at this now?
During CritSmash, assigning to Ralph and emailing for confirmation.
Assignee: nobody → giles
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)
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?
Flags: needinfo?(giles)
Group: media-core-security
Assignee: giles → edwin
Flags: needinfo?(giles)
Flags: needinfo?(edwin)
Status: NEW → ASSIGNED
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...)
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 981869
Flags: sec-bounty?
Edwin, does this means that our invariant is that we should never trigger a second preroll?
Flags: needinfo?(edwin)
(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)
"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
Group: media-core-security
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)
Taking the fix in Fx37 is certainly the security team's recommendation. We might need this on ESR31, too.
Flags: needinfo?(dveditz)
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)
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?
Drive-By Comment: Per Comment 34 Tracking for 37+ and marking affected ESR
Target Milestone: --- → mozilla38
No action needed on this bug. Clearing ni?.
Flags: needinfo?(edwin)
This missed the upcoming ESR31 release?
Flags: needinfo?(lmandel)
Flags: needinfo?(edwin)
Whiteboard: "fixed" by blacklisting flump3dec in bug 981869 → [adv-main37+] "fixed" by blacklisting flump3dec in bug 981869
(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)
(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)
Alias: CVE-2015-0813
Flags: needinfo?(edwin)
Are we not getting an ESR31 patch for this? I thought we were hoping to respin ESR31 with this fix.
Flags: needinfo?(edwin)
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)
(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)
Ah, thanks.
Flags: needinfo?(bkerensa)
Whiteboard: [adv-main37+] "fixed" by blacklisting flump3dec in bug 981869 → [adv-main37+][adv-esr31.6+] "fixed" by blacklisting flump3dec in bug 981869
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)
Thanks Aki! Updating status flags.

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.