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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Abhishek Arya]

==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

Comment 1

[Abhishek Arya]

Attachment: Zip Archive(Part 1)

Comment 2

[Abhishek Arya]

Attachment: Zip Archive(Part 2)

Comment 3

[Abhishek Arya]

Attachment: Zip Archive(Part 3)

Comment 4

[Paul Adenot (:padenot)]

Attachment: r=

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.

Comment 5

[Paul Adenot (:padenot)]

Attachment: r=

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.

Comment 6

[Paul Adenot (:padenot)]

Attachment: r=

And a crashtest.

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 720696 [details] [diff] [review]
r=

use NS_SUCCEEDED rather than comparing with NS_OK directly.

Comment 8

[Paul Adenot (:padenot)]

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 9

[Paul Adenot (:padenot)]

Comment on attachment 720696 [details] [diff] [review]
r=

(See comment 8)

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 720697 [details] [diff] [review]
r=

(See comment 8)

Comment 11

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 12

[Alex Keybl [:akeybl]]

(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.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

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

Comment 14

[Paul Adenot (:padenot)]

Pushed a folded patch to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa7493d1dcf

Comment 15

[Paul Adenot (:padenot)]

Attachment: Rebased for Aurora

Comment 16

[Paul Adenot (:padenot)]

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 17

[Paul Adenot (:padenot)]

Attachment: Rebased for Beta

Comment 18

[Paul Adenot (:padenot)]

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 19

[Paul Adenot (:padenot)]

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 20

[Lukas Blakk [:lsblakk] use ?needinfo]

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 21

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fa7493d1dcf

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Paul Adenot (:padenot)]

Sorry about that.

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