==6341== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60720003ee10 at pc 0x7f7360156c1d bp 0x7f7339f8aa10 sp 0x7f7339f8aa08 READ of size 16 at 0x60720003ee10 thread T23 #0 0x7f7360156c1c in soundtouch::TDStretchSSE::calcCrossCorr(float const*, float const*) const src/media/libsoundtouch/src/sse_optimized.cpp:125 #1 0x7f7360155727 in soundtouch::TDStretch::seekBestOverlapPositionFull(float const*) src/media/libsoundtouch/src/TDStretch.cpp:301 #2 0x7f7360155d1b in soundtouch::TDStretch::processSamples() src/media/libsoundtouch/src/TDStretch.cpp:259 #3 0x7f7360152d4a in soundtouch::SoundTouch::putSamples(float const*, unsigned int) src/media/libsoundtouch/src/FIFOSamplePipe.h:88 #4 0x7f735dcd7a39 in mozilla::BufferedAudioStream::GetTimeStretched(void*, long) src/content/media/AudioStream.cpp:1038 #5 0x7f735dcd7d55 in mozilla::BufferedAudioStream::DataCallback(void*, long) src/content/media/AudioStream.cpp:1063 #6 0x7f7360158f3f in cubeb_run_thread src/media/libcubeb/src/cubeb_alsa.c:301 #7 0x418f1a in __asan::AsanThread::ThreadStart() #8 0x7f7366958e99 in start_thread #9 0x7f7365c6fcbc in 0x60720003ee10 is located 0 bytes to the right of 3088-byte region [0x60720003e200,0x60720003ee10) allocated by thread T22 (Media Audio) here: #0 0x413bc2 in malloc #1 0x7f7363e591e8 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54 Thread T23 created by T22 (Media Audio) here: #0 0x4113d4 in __interceptor_pthread_create #1 0x7f7360157875 in cubeb_init src/media/libcubeb/src/cubeb_alsa.c:685 #2 0x7f735dcd56fd in mozilla::BufferedAudioStream::Init(int, int, mozilla::dom::AudioChannelType) src/content/media/AudioStream.cpp:144 #3 0x7f735dcacacf in mozilla::MediaDecoderStateMachine::AudioLoop() src/content/media/MediaDecoderStateMachine.cpp:1011 Thread T22 (Media Audio) created by T21 (Media Decode) here: #0 0x4113d4 in __interceptor_pthread_create #1 0x7f73659450ef in _PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:393 #2 0x7f7365944b4a in PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:476 #3 0x7f735f8c1554 in nsThread::Init() src/xpcom/threads/nsThread.cpp:331 Thread T21 (Media Decode) created by T20 (Media State) here: #0 0x4113d4 in __interceptor_pthread_create #1 0x7f73659450ef in _PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:393 #2 0x7f7365944b4a in PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:476 #3 0x7f735f8c1554 in nsThread::Init() src/xpcom/threads/nsThread.cpp:331 Thread T20 (Media State) created by T0 here: #0 0x4113d4 in __interceptor_pthread_create #1 0x7f73659450ef in _PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:393 #2 0x7f7365944b4a in PR_CreateThread src/nsprpub/pr/src/pthreads/ptthread.c:476 #3 0x7f735f8c1554 in nsThread::Init() src/xpcom/threads/nsThread.cpp:331 Shadow bytes around the buggy address: 0x1c0e40007d70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c0e40007d80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c0e40007d90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c0e40007da0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x1c0e40007db0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x1c0e40007dc0: 00 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c0e40007dd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c0e40007de0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c0e40007df0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c0e40007e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x1c0e40007e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Heap righ redzone: fb Freed Heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack partial redzone: f4 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 ASan internal: fe ==6341== ABORTING

This is caused by us trying to set the number of channels to something greater than 2 on a release build. This is actually caught by an assert in debug mode. In release, because we don't assert, we are writing somewhere strange, hence the heap overflow. This patch is the minimum we should do to avoid crashing, that is, refuse to set the playbackRate if we are playing a file that has more than 2 channels. I'll file a bug to implement arbitrary channels time stretching in soundtouch, it should not be too difficult, but considering that 3+ channels files are quite rare, I will not do it right now.

This is not necessary per se, but if someone sets the playback rate below the element in the chain, at least we won't crash.

And a crashtest.

Comment on attachment 720696 [details] [diff] [review] r= use NS_SUCCEEDED rather than comparing with NS_OK directly.

Comment on attachment 720691 [details] [diff] [review] r= I have little idea what I'm doing by setting the flag here, please tell me if I'm doing it wrong. (basically, last time I checked in a patch from a security-sensitive bug, and people told me that I should follow the process, and the wiki says to set sec-approval? and move on, so I'm doing that). [Security approval request comment] How easily could an exploit be constructed based on the patch? No idea. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, the crash is caused by loading a media file that has more than 2 channels in an audio tag, and setting the playbackRate property. So the crash test does exactly that. Which older supported branches are affected by this flaw? Aurora, Beta If not all supported branches, which bug introduced the flaw? Bug 495040 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Easy to backport, will do if needed. How likely is this patch to cause regressions; how much testing does it need? The problem and the fix are obvious, I'm not worried about regressions.

Comment on attachment 720696 [details] [diff] [review] r= (See comment 8)

Comment on attachment 720697 [details] [diff] [review] r= (See comment 8)

Before I give sec-approval for this, we need to know if Release Management will take this on Beta and Aurora, since it affects them. It would be really good to have a security rating for this issue. That's usually step one for a security bug.

(In reply to Al Billings [:abillings] from comment #11) > Before I give sec-approval for this, we need to know if Release Management > will take this on Beta and Aurora, since it affects them. > > It would be really good to have a security rating for this issue. That's > usually step one for a security bug. Given the risk evaluation in comment 8 and the fact that this is a regression, we'd like to take the fix on beta asap.

I've given sec-approval+ for this to go into trunk. Paul, please nominate for Aurora and Beta and prepare patches there as well.

Pushed a folded patch to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa7493d1dcf

Comment on attachment 723423 [details] [diff] [review] Rebased for Aurora [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 495040 User impact if declined: Crash when setting the |playbackRate| property and playing a media file that has more that 2 channels Testing completed (on m-c, etc.): has a crashtest, landed on m-i Risk to taking this patch (and alternatives if risky): not risky, obvious bug and trivial fix String or UUID changes made by this patch: none

Comment on attachment 723423 [details] [diff] [review] Rebased for Aurora (sorry, bzexport obsoleted the patch automatically) [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 495040 User impact if declined: Crash when setting the |playbackRate| property and playing a media file that has more that 2 channels Testing completed (on m-c, etc.): has a crashtest, landed on m-i Risk to taking this patch (and alternatives if risky): not risky, obvious bug and trivial fix String or UUID changes made by this patch: none

Comment on attachment 723430 [details] [diff] [review] Rebased for Beta [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 495040 User impact if declined: Crash when setting the |playbackRate| property and playing a media file that has more that 2 channels Testing completed (on m-c, etc.): has a crashtest, landed on m-i Risk to taking this patch (and alternatives if risky): not risky, obvious bug and trivial fix String or UUID changes made by this patch: none

Comment on attachment 723423 [details] [diff] [review] Rebased for Aurora Approving to land to Aurora once this has been merged successfully to mozilla-central.

Comment on attachment 723430 [details] [diff] [review] Rebased for Beta Approving for Beta now, so it can be landed today as soon as this has been merged successfully to mozilla-central.

https://hg.mozilla.org/releases/mozilla-aurora/rev/2677c3f3ebbf https://hg.mozilla.org/releases/mozilla-beta/rev/9cbb8e7e65df

Backed out on beta for bustage. https://hg.mozilla.org/releases/mozilla-beta/rev/ef87b1a2f8d3 https://tbpl.mozilla.org/php/getParsedLog.php?id=20545320&tree=Mozilla-Beta VideoFrameContainer.cpp ../../../../content/media/MediaDecoderStateMachine.cpp:987:9: error: unknown type name 'audioStream'; did you mean 'AudioStream'? if (audioStream->SetPreservesPitch(preservesPitch) != NS_OK) { ^~~~~~~~~~~ AudioStream ../../../../content/media/AudioSegment.h:16:7: note: 'AudioStream' declared here class AudioStream; ^ ../../../../content/media/MediaDecoderStateMachine.cpp:987:20: error: expected unqualified-id if (audioStream->SetPreservesPitch(preservesPitch) != NS_OK) { ^ ../../../../content/media/MediaDecoderStateMachine.cpp:987:20: error: variable declaration in condition must have an initializer ../../../../content/media/MediaDecoderStateMachine.cpp:994:11: error: unknown type name 'audioStream'; did you mean 'AudioStream'? if (audioStream->SetPlaybackRate(playbackRate) != NS_OK) { ^~~~~~~~~~~ AudioStream ../../../../content/media/AudioSegment.h:16:7: note: 'AudioStream' declared here class AudioStream; ^ ../../../../content/media/MediaDecoderStateMachine.cpp:994:22: error: expected unqualified-id if (audioStream->SetPlaybackRate(playbackRate) != NS_OK) { ^ ../../../../content/media/MediaDecoderStateMachine.cpp:994:22: error: variable declaration in condition must have an initializer 6 errors generated. make[7]: *** [MediaDecoderStateMachine.o] Error 1 make[7]: *** Waiting for unfinished jobs.... make[6]: *** [media_libs] Error 2

Sorry about that. https://hg.mozilla.org/releases/mozilla-beta/rev/681e92032e24