1106596 - (CVE-2015-0813) heap-use-after-free at AppendElements

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Aki Helin]

Attachment: ff-uaf-AppendElements.mp3

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

Comment 1

[Aki Helin]

Attachment: ff-uaf-AppendElements.html

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Anthony, is there someone on your team that can work on this? (Can you recreate it?)

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Maire - does it make sense for padenot to look into this?

Comment 4

[Maire Reavy [:mreavy]]

This looks like it should go to Padenot or Karlt.

Paul -- Can you take this one?

Comment 5

[Paul Adenot (:padenot)]

Attachment: trace | asan_symbolize.py | c++filt

This looks like a missing addref somewhere, we are trying to push freed data to the AudioStream circular buffer.

Comment 6

[Paul Adenot (:padenot)]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Matt: Can you take this bug?

Comment 8

[Bobby Holley (:bholley)]

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

[Matt Woodrow (:mattwoodrow)]

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.

Comment 10

[Bobby Holley (:bholley)]

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?

Comment 11

[Aki Helin]

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

[Paul Adenot (:padenot)]

Attachment: 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

Comment 13

[Bobby Holley (:bholley)]

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

[Bobby Holley (:bholley)]

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.

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

This isn't a padenot bug anymore: Apparently it's rillian or edwin

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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

[Bobby Holley (:bholley)]

(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

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Can we make it fail gracefully for now and fix it in a follow up bug?

Comment 21

[Ralph Giles (:rillian)]

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

[Randell Jesup [:jesup] (needinfo me)]

rillian: are you available to look at this now?

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

During CritSmash, assigning to Ralph and emailing for confirmation.

Comment 24

[Ralph Giles (:rillian)]

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.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

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?

Comment 26

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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...)

Comment 29

[Bobby Holley (:bholley)]

Edwin, does this means that our invariant is that we should never trigger a second preroll?

Comment 30

[Edwin Flores [inactive from 2016-12-01] [:eflores] [: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.

Comment 31

[Daniel Veditz [:dveditz]]

"fixed" is probably a better resolution, so in case the fix gets backed out we'll remember it was preventing an exploitable hole.

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 33

[Daniel Veditz [:dveditz]]

Taking the fix in Fx37 is certainly the security team's recommendation. We might need this on ESR31, too.

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 35

[Lawrence Mandel [:lmandel] (use needinfo)]

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

[Benjamin Kerensa [:bkerensa]]

Drive-By Comment: Per Comment 34 Tracking for 37+ and marking affected ESR

Comment 37

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

No action needed on this bug. Clearing ni?.

Comment 38

[Al Billings [:abillings - ex-MoCo]]

This missed the upcoming ESR31 release?

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

(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.

Comment 40

[Benjamin Kerensa [: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.

Comment 41

[Al Billings [:abillings - ex-MoCo]]

Are we not getting an ESR31 patch for this? I thought we were hoping to respin ESR31 with this fix.

Comment 42

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 43

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(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.

Comment 44

[Al Billings [:abillings - ex-MoCo]]

Ah, thanks.

Comment 45

[Bogdan Maris, Desktop QA]

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?

Comment 46

[Aki Helin]

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.

Comment 47

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Thanks Aki! Updating status flags.