1206362 - Assertion failure: aParam >= 0, at c:/Users/mozilla/debug-builds/mozilla-central/dom/media/webaudio/AudioBufferSourceNode.cpp:122

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Carsten Book [:Tomcat]]

Found via bughunter and reproduced on windows 7 debug build.

Steps to reproduce:

Load: https://ru.wargaming.net/globalmap/#province/baymak

after a few seconds:

Assertion failure: aParam >= 0, at c:/Users/mozilla/debug-builds/mozilla-central/dom/media/webaudio/AudioBufferSourceNode.cpp:122
#01: `mozilla::AudioNodeStream::SetInt32Parameter'::`2'::Message::Run (c:\users\mozilla\debug-builds
\mozilla-central\dom\media\webaudio\audionodestream.cpp:195)
#02: mozilla::MediaStreamGraphImpl::OneIteration (c:\users\mozilla\debug-builds\mozilla-central\dom\
media\mediastreamgraph.cpp:1209)
#03: mozilla::AudioCallbackDriver::DataCallback (c:\users\mozilla\debug-builds\mozilla-central\dom\m
edia\graphdriver.cpp:846)
#04: mozilla::AudioCallbackDriver::DataCallback_s (c:\users\mozilla\debug-builds\mozilla-central\dom
\media\graphdriver.cpp:691)
#05: noop_resampler::fill (c:\users\mozilla\debug-builds\mozilla-central\media\libcubeb\src\cubeb_re
sampler.cpp:89)
#06: cubeb_resampler_fill (c:\users\mozilla\debug-builds\mozilla-central\media\libcubeb\src\cubeb_re
sampler.cpp:245)
#07: `anonymous namespace'::refill (c:\users\mozilla\debug-builds\mozilla-central\media\libcubeb\src
\cubeb_wasapi.cpp:457)
#08: `anonymous namespace'::wasapi_stream_render_loop (c:\users\mozilla\debug-builds\mozilla-central
\media\libcubeb\src\cubeb_wasapi.cpp:586)
#09: _get_flsindex[MSVCR120 +0x2c01d]
#10: _get_flsindex[MSVCR120 +0x2c001]
#11: BaseThreadInitThunk[kernel32 +0x4ee1c]
#12: RtlInitializeExceptionChain[ntdll +0x637eb]
#13: RtlInitializeExceptionChain[ntdll +0x637be]

Comment 1

[Carsten Book [:Tomcat]]

Attachment: bughunter crash stack

Comment 2

[Carsten Book [:Tomcat]]

karl, i guess this might be something for you

Comment 3

[Karl Tomlinson (:karlt)]

Thanks, Carsten.

There is an overflow when converting double to int32_t at https://hg.mozilla.org/mozilla-central/annotate/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/dom/media/webaudio/AudioBufferSourceNode.cpp#l707

Before https://hg.mozilla.org/mozilla-central/rev/358256d1f999 that could only pass through as a negative upper bound for mBufferPosition in mBuffer.
Now the negative int32_t is converted to an uint32_t, which is too large.

[Tracking Requested - why for this release]:
sec-high

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: crashtest (don't check in yet)

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: be careful about double -> int conversion

Bounds checking against buffer length before conversion.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8663493 [details] [diff] [review]
be careful about double -> int conversion

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?
Once the problem is determined, it would be reasonably easy to construct and exploit to read memory.

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The test will not be checked in.
The code change has nothing that says exactly what was wrong, but analysis of the few lines changes can determine the problem.

> Which older supported branches are affected by this flaw?
43 is the only branch where the flaw is a security risk

>If not all supported branches, which bug introduced the flaw?
A change for bug 1201855 turned the flaw into a security risk.  (comment 3)

>Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial

>How likely is this patch to cause regressions; how much testing does it need?
There are a few existing tests and the change is not complicated.

Comment 7

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

Comment on attachment 8663493 [details] [diff] [review]
be careful about double -> int conversion

sec-approval+ because I assume this will need to go into trunk (44) as well as Aurora (43). 

Please nominate it for Aurora as well.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/830a804fa959

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8663493 [details] [diff] [review]
be careful about double -> int conversion

Approval Request Comment
[Feature/regressing bug #]:
A change for bug 1201855 turned an existing flaw into a security risk.
[User impact if declined]: sec-high
[Describe test coverage new/current, TreeHerder]: 
not checking in demonstration of bug
[Risks and why]: low
There are a few existing tests and the change is not complicated.
[String/UUID change made/needed]: none.

Comment 10

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8663493 [details] [diff] [review]
be careful about double -> int conversion

Sec-high, has test coverage, taking it for aurora.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9d6e9e5ab70

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bec2bc1a7919 (thats the tests)