1122218 - (CVE-2015-2729) Out-of-Bounds Read in AudioParamTimeline::AudioNodeInputValue

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Holger Fuhrmannek]

Attachment: testcase.html

The asan report was generated with this build: 

https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1421077535/firefox-37.0a1.en-US.linux-x86_64-asan.tar.bz2

Asan Log:

==32128==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6160008fd890 at pc 0x7f298eb25e71 bp 0x7f2934856310 sp 0x7f2934856308
READ of size 4 at 0x6160008fd890 thread T69 (threaded-ml)
    #0 0x7f298eb25e70 in AudioNodeInputValue /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/webaudio/AudioParam.cpp:139
    #1 0x7f298eb6a48c in GetValueAtTime<long> /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/webaudio/AudioParamTimeline.h:93
    #2 0x7f298eb69c53 in ComputeCustom /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/webaudio/OscillatorNode.cpp:243
    #3 0x7f298eb232df in ProcessInput /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/webaudio/AudioNodeStream.cpp:492
    #4 0x7f298e9b4016 in ProduceDataForStreamsBlockByBlock /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/media/MediaStreamGraph.cpp:1229
...

Comment 1

[Daniel Veditz [:dveditz]]

Paul: what's the implications of reading from a bogus stream? This one value doesn't seem terrible, but if the stream is truncated what other bad data could other parts of the code read or do bad things to?

Comment 2

[Paul Adenot (:padenot)]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> Paul: what's the implications of reading from a bogus stream? This one value
> doesn't seem terrible, but if the stream is truncated what other bad data
> could other parts of the code read or do bad things to?

No serious implication, this will just feed weird audio into the graph. That said, I'll certainly look into this one.

Comment 3

[Andrew McCreight [:mccr8]]

I guess it seems like maybe this could be bad because somebody could read out random memory which could expose data about pointers?  Can the actual garbage data be extracted somehow?

Comment 4

[Paul Adenot (:padenot)]

No, content cannot get back the value reliably, as far as I know.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Dropping tracking since it's sec-moderate but please re-nom if the severity changes. 
Is 39 likely to also be affected?

Comment 6

[Paul Adenot (:padenot)]

Yes, 39 will be affected.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Fix off-by-one error when computing oscillator rendering range. r=

baku recently added asserts in the AudioParam code that made pinpointing this
easier.

Below is a slightly easier to understand testcase:

> var ac = new OfflineAudioContext(1, 256, 44100);
> var osc = ac.createOscillator();
> // to go in ComputeCustom
> osc.type = "square";
> // to go sample the param value in UpdateParametersIfNeeded
> osc.frequency.setValueAtTime(440, 0);
>
> // First block should be silent, sound should start exactly at the second
> // block
> osc.start(128 / ac.sampleRate);
>
> ac.startRendering();

For the first block, mozilla::dom::OscillatorNodeEngine::FillBounds returns
`start = 128` and `end = 128`, meaning no samples have to be computed, and the
block should be entirely silent, and that should have been caught by the
ComputeSilence case above.

It's now caught when trying to update the parameters:
"Assertion failure: aCounter < WEBAUDIO_BLOCK_SIZE" in GetValuesAtTime, because
we're trying to sample 129 values of a 128 elements array, so it's "only"
an out-of-bound read of exactly one element, nothing terrible.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> I guess it seems like maybe this could be bad because somebody could read
> out random memory which could expose data about pointers?  Can the actual
> garbage data be extracted somehow?

One adjacent set of four bytes may be interpreted as a float for a frequency for one sample of a time domain signal, which can be read from content.
It may be possible for content to determine some information about the original bytes.  However, there may be loss of precision and the inverse is not unique.

Comment 9

[Paul Adenot (:padenot)]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta/esr38/b2g37/b2g34 approval when you get a chance.

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8604666 [details] [diff] [review]
Fix off-by-one error when computing oscillator rendering range. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): initial Web Audio API landing in Firefox 25
User impact if declined: maybe a way to infer the content of a very small portion of memory (four bytes at a time)
Testing completed: test added, baked on central for a while
Risk to taking this patch (and alternatives if risky): low, this is a well understood off-by-one error. Alternative is to leave the bug in.
String or UUID changes made by this patch: none

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8604666 [details] [diff] [review]
Fix off-by-one error when computing oscillator rendering range. r=

Test, improve stability, taking it.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dce717706040

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/befc7b294081

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/0b2496be7da1

Comment 17

[Josh Cheng [:josh]]

Comment on attachment 8604666 [details] [diff] [review]
Fix off-by-one error when computing oscillator rendering range. r=

Approving as this increases stability. Please NI if this causing any side effect.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/574026262484
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/574026262484

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6ae516ab1f40