Closed Bug 1122218 (CVE-2015-2729) Opened 9 years ago Closed 9 years ago

Out-of-Bounds Read in AudioParamTimeline::AudioNodeInputValue

Categories

(Core :: Web Audio, defect)

37 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- wontfix
firefox-esr38 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: hofusec, Assigned: padenot)

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [adv-main39+][adv-esr38.1+])

Attachments

(2 files)

Attached file 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
...
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?
Flags: needinfo?(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.
Flags: needinfo?(padenot)
Assignee: nobody → padenot
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?
No, content cannot get back the value reliably, as far as I know.
Dropping tracking since it's sec-moderate but please re-nom if the severity changes. 
Is 39 likely to also be affected?
Yes, 39 will be affected.
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.
Attachment #8604666 - Flags: review?(karlt)
Attachment #8604666 - Flags: review?(karlt) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/c48d5ce78376
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Please nominate this for Aurora/Beta/esr38/b2g37/b2g34 approval when you get a chance.
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
Flags: needinfo?(padenot)
Attachment #8604666 - Flags: approval-mozilla-esr38?
Attachment #8604666 - Flags: approval-mozilla-beta?
Attachment #8604666 - Flags: approval-mozilla-b2g37?
Attachment #8604666 - Flags: approval-mozilla-b2g34?
Attachment #8604666 - Flags: approval-mozilla-aurora?
Comment on attachment 8604666 [details] [diff] [review]
Fix off-by-one error when computing oscillator rendering range. r=

Test, improve stability, taking it.
Attachment #8604666 - Flags: approval-mozilla-esr38?
Attachment #8604666 - Flags: approval-mozilla-esr38+
Attachment #8604666 - Flags: approval-mozilla-beta?
Attachment #8604666 - Flags: approval-mozilla-beta+
Attachment #8604666 - Flags: approval-mozilla-aurora?
Attachment #8604666 - Flags: approval-mozilla-aurora+
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.
Attachment #8604666 - Flags: approval-mozilla-b2g37?
Attachment #8604666 - Flags: approval-mozilla-b2g37+
Attachment #8604666 - Flags: approval-mozilla-b2g34?
Attachment #8604666 - Flags: approval-mozilla-b2g34+
Whiteboard: [adv-main39+][adv-esr38.1+]
Alias: CVE-2015-2729
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.