Heap-buffer-overflow in soundtouch::TDStretchSSE::calcCrossCorr

RESOLVED FIXED in Firefox 20

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: inferno, Assigned: padenot)

Tracking

(4 keywords)

Trunk
mozilla22
x86_64
All
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox19 unaffected, firefox20+ fixed, firefox21+ fixed, firefox22+ fixed, firefox-esr17 unaffected, b2g18 unaffected, b2g18-v1.0.1 unaffected)

Details

(Whiteboard: [asan][adv-main20-])

Attachments

(8 attachments)

(Reporter)

Description

6 years ago
==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
(Reporter)

Comment 1

6 years ago
Posted file Zip Archive(Part 1)
(Reporter)

Comment 2

6 years ago
Posted file Zip Archive(Part 2)
(Reporter)

Comment 3

6 years ago
Posted file Zip Archive(Part 3)
Severity: normal → critical
Component: General → Video/Audio
Keywords: crash
Product: Firefox → Core
Whiteboard: [asan]
(Assignee)

Comment 4

6 years ago
Posted patch r=Splinter Review
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.
Attachment #720691 - Flags: review?(kinetik)
(Assignee)

Updated

6 years ago
Assignee: nobody → paul
(Assignee)

Comment 5

6 years ago
Posted patch r=Splinter Review
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.
Attachment #720696 - Flags: review?(kinetik)
(Assignee)

Comment 6

6 years ago
Posted patch r=Splinter Review
And a crashtest.
Attachment #720697 - Flags: review?(kinetik)
Attachment #720691 - Flags: review?(kinetik) → review+
Attachment #720697 - Flags: review?(kinetik) → review+
Comment on attachment 720696 [details] [diff] [review]
r=

use NS_SUCCEEDED rather than comparing with NS_OK directly.
Attachment #720696 - Flags: review?(kinetik) → review+
(Assignee)

Comment 8

6 years ago
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.
Attachment #720691 - Flags: sec-approval?
(Assignee)

Comment 9

6 years ago
Comment on attachment 720696 [details] [diff] [review]
r=

(See comment 8)
Attachment #720696 - Flags: sec-approval?
(Assignee)

Comment 10

6 years ago
Comment on attachment 720697 [details] [diff] [review]
r=

(See comment 8)
Attachment #720697 - Flags: sec-approval?
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.
Attachment #720696 - Flags: sec-approval? → sec-approval+
Attachment #720697 - Flags: sec-approval? → sec-approval+
Attachment #720691 - Flags: sec-approval? → sec-approval+
I've given sec-approval+ for this to go into trunk. Paul, please nominate for Aurora and Beta and prepare patches there as well.
(Assignee)

Comment 15

6 years ago
(Assignee)

Comment 16

6 years ago
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
Attachment #723423 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 17

6 years ago
(Assignee)

Updated

6 years ago
Attachment #723423 - Attachment is obsolete: true
Attachment #723423 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

6 years ago
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
Attachment #723423 - Attachment is obsolete: false
Attachment #723423 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

6 years ago
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
Attachment #723430 - Flags: approval-mozilla-beta?
Comment on attachment 723423 [details] [diff] [review]
Rebased for Aurora

Approving to land to Aurora once this has been merged successfully to mozilla-central.
Attachment #723423 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #723430 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/0fa7493d1dcf
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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
Whiteboard: [asan] → [asan][adv-main20+]
Whiteboard: [asan][adv-main20+] → [asan][adv-main20-]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security
You need to log in before you can comment on or make changes to this bug.