Closed Bug 874869 Opened 11 years ago Closed 11 years ago

SEGV on mozilla::AudioChannelsDownMix

Categories

(Core :: Web Audio, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox22 --- disabled
firefox23 - disabled
firefox24 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: attekett, Assigned: ehsan.akhgari)

References

Details

(4 keywords, Whiteboard: [adv-main24-])

Attachments

(1 file)

Tested on:

OS: Ubuntu 12.04

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

Repro-case:

<script>
var Context0= new AudioContext()
var Panner0=Context0.createPanner();
var BufferSource3=Context0.createBufferSource();
Panner0.channelCount=0;
BufferSource3.connect(Panner0);
BufferSource3.buffer=function(){
	var length=1;
	var Buffer=Context0.createBuffer(1,length,'44200');
	var bufferData= Buffer.getChannelData(0);
	for (var i = 0; i < length; ++i) { bufferData[i] = 255;};
	return Buffer;
}();

</script>

ASAN-output:(opt-build)

=================================================================
==2893== ERROR: AddressSanitizer: SEGV on unknown address 0x7f5551b7fcdc (pc 0x7f514ce8e0a6 sp 0x7f5125511300 bp 0x7f51255113f0 T26)
AddressSanitizer can not provide additional info.
    #0 0x7f514ce8e0a5 in mozilla::AudioChannelsDownMix(nsTArray<void const*> const&, float**, unsigned int, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioChannelFormat.cpp:175
    #1 0x7f514ce91d57 in mozilla::AudioNodeStream::ObtainInputBlock(mozilla::AudioChunk&, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioNodeStream.cpp:347
    #2 0x7f514ce92fe5 in mozilla::AudioNodeStream::ProduceOutput(long, long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioNodeStream.cpp:407
    #3 0x7f514cf00c83 in mozilla::MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock(unsigned int, long, long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:937
    #4 0x7f514cf12e05 in mozilla::(anonymous namespace)::MediaStreamGraphThreadRunnable::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:1163
    #5 0x7f514f5a4212 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238
.
.
.
Blocks: webaudio
Keywords: crash, testcase
OS: Linux → All
I can repro on a regular debug build.
Assignee: nobody → ehsan
BTW, I think this is a safe crash, because we try to read memory from someArray[channelCount - 1] when channelCount is 0.  I doubt that this is a security sensitive bug.
Attached patch Patch (v1)Splinter Review
Attachment #752733 - Flags: review?(roc)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> BTW, I think this is a safe crash, because we try to read memory from
> someArray[channelCount - 1] when channelCount is 0.  I doubt that this is a
> security sensitive bug.

Why should it be safe to read before someArray? Do you get the data back that is stored there?
There is second similar crash:

Repro-case:
<script>
var Context0= new AudioContext()
var BufferSource0=Context0.createBufferSource();
BufferSource0.connect(Context0.destination);

Context0.destination.channelCount=0;
BufferSource0.buffer=function(){
	var length=73020;
	var Buffer=Context0.createBuffer(1,length,Context0.sampleRate);
	var bufferData= Buffer.getChannelData(0);
	for (var i = 0; i < length; ++i) { bufferData[i] = Math.sin(i*(365))};
	return Buffer;
}();
Context0.destination.channelInterpretation="discrete";
</script>

ASAN-report:

==20656== ERROR: AddressSanitizer: SEGV on unknown address 0x7f953d869cdc (pc 0x7f9138b77c56 sp 0x7f9111a5b4c0 bp 0x7f9111a5b5b0 T28)
AddressSanitizer can not provide additional info.
    #0 0x7f9138b77c55 in mozilla::AudioChannelsUpMix(nsTArray<void const*>*, unsigned int, void const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioChannelFormat.cpp:91
    #1 0x7f9138b86c54 in mozilla::AudioSegment::WriteTo(mozilla::AudioStream*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioSegment.cpp:125
    #2 0x7f9138be85fa in mozilla::MediaStreamGraphImpl::PlayAudio(mozilla::MediaStream*, long, long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:803
    #3 0x7f9138beaf50 in mozilla::MediaStreamGraphImpl::RunThread() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:1031
.
.
.


btw, if you change Context0.destination.channelCount into small negative number, like -1, from this repro you will get oom.

ASAN-report:

==20539== WARNING: AddressSanitizer failed to allocate 0x0003fffffe10 bytes
out of memory: 0x00000003FFFFFE10 bytes requested
ASAN:SIGSEGV
=================================================================
==20539== ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7faab027b678 sp 0x7faa81e8f2f0 bp 0x7faa81e8f3a0 T25)
AddressSanitizer can not provide additional info.
    #0 0x7faab027b677 in mozalloc_abort(char const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/memory/mozalloc/mozalloc_abort.cpp:30
    #1 0x7faab027b3bc in moz_xmalloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/memory/mozalloc/mozalloc.cpp:56
.
.
.
(In reply to Christian Holler (:decoder) from comment #4)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > BTW, I think this is a safe crash, because we try to read memory from
> > someArray[channelCount - 1] when channelCount is 0.  I doubt that this is a
> > security sensitive bug.
> 
> Why should it be safe to read before someArray? Do you get the data back
> that is stored there?

What I meant was that _if_ we crash here, it's going to be safe, since that would be the result of attempting to read an unmapped page.
(In reply to Atte Kettunen from comment #5)
> There is second similar crash:
> 
> Repro-case:
> <script>
> var Context0= new AudioContext()
> var BufferSource0=Context0.createBufferSource();
> BufferSource0.connect(Context0.destination);
> 
> Context0.destination.channelCount=0;
> BufferSource0.buffer=function(){
> 	var length=73020;
> 	var Buffer=Context0.createBuffer(1,length,Context0.sampleRate);
> 	var bufferData= Buffer.getChannelData(0);
> 	for (var i = 0; i < length; ++i) { bufferData[i] = Math.sin(i*(365))};
> 	return Buffer;
> }();
> Context0.destination.channelInterpretation="discrete";
> </script>

Yeah, this bug happens when you set channelCount to 0.  You could construct any number of scenarios which would trigger this bug in this exact way.

Fix landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2718954757e1
Needed a clobber <https://hg.mozilla.org/integration/mozilla-inbound/rev/e52c6f7200b3> because the build system sucks (see bug 874923.)
Setting to sec-low per comment 2.
Keywords: sec-low
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> 
> What I meant was that _if_ we crash here, it's going to be safe, since that
> would be the result of attempting to read an unmapped page.

Okay, and what if we don't crash there? I don't see how this would lead to the conclusion that this is not a security problem or sec-low. The crash is not guaranteed, is it?
How do you guarantee that the memory before the array is unmapped?

What do you do with the object you do read should this not crash on an access violation?
Flags: needinfo?(ehsan)
Keywords: sec-low
(In reply to Daniel Veditz [:dveditz] from comment #11)
> How do you guarantee that the memory before the array is unmapped?

Comment 2 was poorly worded, and not well thought out.  I retract that comment.

> What do you do with the object you do read should this not crash on an
> access violation?

So here's what happens.  We use -1 as an index to the first array, and if that doesn't segfault, we use the read value + some non-zero offset from another array <http://mxr.mozilla.org/mozilla-central/source/content/media/AudioChannelFormat.cpp#173>.  If none of these segfault, we start filling an array using the values from the invalid pointer that we have read + a non-negative offset <http://mxr.mozilla.org/mozilla-central/source/content/media/AudioChannelFormat.cpp#185> and <http://mxr.mozilla.org/mozilla-central/source/content/media/AudioChannelFormat.cpp#190>, and if that doesn't segfault either, we're probably overwriting memory which doesn't belong to us.  But the things we write to the memory are floating point multiplications based on values read from garbage memory as well (plus the values coming from the previous node in the graph).  In theory, it's possible to craft everything in a way that you end up writing what you want to the memory address that you want, but it definitely requires some heroics being put into it.  Which is why I suggested in comment 2 that this is probably not a security bug, but I guess that theoretical possibility still exists.
Flags: needinfo?(ehsan)
That's quite odd.  I wonder if there is some bindings-related leak, as I can't see how this patch would cause leaks by itself.  Unless the test case happens to trigger leaks.
(In reply to Andrew McCreight [:mccr8] from comment #14)
> That's quite odd.  I wonder if there is some bindings-related leak, as I
> can't see how this patch would cause leaks by itself.  Unless the test case
> happens to trigger leaks.

The testcase is in the crashtest suite...
So I pushed the patch landed on inbound and retriggered mochitest-1 runs on Windows on it, and the leak did not happen again: https://tbpl.mozilla.org/?tree=Try&rev=bc1cd1f75c78

This shows that the leak was intermittent in fact, and the patch was prematurely backed out.  So I relanded it: https://hg.mozilla.org/integration/mozilla-inbound/rev/96b964d758c8
https://hg.mozilla.org/mozilla-central/rev/96b964d758c8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main24-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.