878478 - Heap-buffer-overflow READ in mozilla::dom::AudioBufferSourceNodeEngine::CopyFromInputBuffer

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

Tested on:

OS: Ubuntu 12.04

Firefox: ASAN opt-build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1370082293/

ASAN-report:

==13806== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fb8aa796e50 at pc 0x7fb8ce920d22 bp 0x7fb8a2494070 sp 0x7fb8a2494068
READ of size 1 at 0x7fb8aa796e50 thread T28
    #0 0x7fb8ce920d21 in mozilla::dom::AudioBufferSourceNodeEngine::CopyFromInputBuffer(mozilla::AudioChunk*, unsigned int, unsigned long, unsigned long, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/content/media/webaudio/AudioBufferSourceNode.cpp:174
    #1 0x7fb8ce91f16b in mozilla::dom::AudioBufferSourceNodeEngine::ProduceAudioBlock(mozilla::AudioNodeStream*, mozilla::AudioChunk const&, mozilla::AudioChunk*, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/content/media/webaudio/AudioBufferSourceNode.cpp:397
    #2 0x7fb8ce88704a in mozilla::AudioNodeStream::ProduceOutput(long, long) /builds/slave/m-cen-l64-asan-000000000000000/build/content/media/AudioNodeStream.cpp:428
    #3 0x7fb8ce8f5a1e in mozilla::AudioNodeStream::SampleRate() const /builds/slave/m-cen-l64-asan-000000000000000/build/content/media/MediaStreamGraph.cpp:967
    #4 0x7fb8ce9089a5 in mozilla::(anonymous namespace)::MediaStreamGraphThreadRunnable::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/content/media/MediaStreamGraph.cpp:1214
    #5 0x7fb8d0fc9cd2 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238
.
.
.

Comment 1

[Christoph Diehl [:posidron]]

Yet again we are crashing at the same memcpy() with a different testcase as we did already in bug 876338. Is there really no possible way to sanitize those arguments before we make this particular memcpy() call?

Comment 2

[(no longer active)]

(In reply to Christoph Diehl [:cdiehl] from comment #1)
> Yet again we are crashing at the same memcpy() with a different testcase as
> we did already in bug 876338. Is there really no possible way to sanitize
> those arguments before we make this particular memcpy() call?

No, there is no silver bullet.  Sorry.

Comment 3

[(no longer active)]

So here's what happens here.  We first set a buffer, then we set it to looping.  Now, since loopEnd is not set, we set it to the length of the first buffer.  Then we set loopEnd, but this time loopEnd is so small that loopStartTicks and loopEndTicks will both end up being 0, hence in this condition check <http://mxr.mozilla.org/mozilla-central/source/content/media/webaudio/AudioBufferSourceNode.cpp?force=1#677> we decide to not change the loop arguments, and then we go ahead and set a buffer with a smaller size, which means that loopEnd will now be too large of an offset...

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f66088121bcb

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f66088121bcb

Comment 7

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> We first set a buffer, then we set it to looping. [...]
> and then we go ahead and set a buffer with a smaller size, which
> means that loopEnd will now be too large of an offset...

What are we doing in the loop? Reading or writing? If we're reading, what do we do with the potentially bogus values we just read?

Comment 9

[(no longer active)]

(In reply to Daniel Veditz [:dveditz] from comment #8)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> > We first set a buffer, then we set it to looping. [...]
> > and then we go ahead and set a buffer with a smaller size, which
> > means that loopEnd will now be too large of an offset...
> 
> What are we doing in the loop? Reading or writing?

We're reading from it.

> If we're reading, what do
> we do with the potentially bogus values we just read?

To the best of my knowledge, we just do floating point computations on it, not making branching decisions.  Which means that we can crash if we get a math exception, but not jump to the wrong address.

Comment 10

[Daniel Veditz [:dveditz]]

So if you manage not to crash the browser (heap feng shui) you might be able to interpret memory contents by analyzing the end of a generated sound? Could be interesting, but a much less severe problem than arbitrary code execution.