329 bytes, text/html
2.29 KB, patch
|Details | Diff | Splinter Review|
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?
(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.
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)
(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.
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
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.
You need to log in before you can comment on or make changes to this bug.