1423770 - Write out of bounds in ConvertAudioSamples

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Nils Ohlmeier [:drno]]

A Windows only crash which started in 55, e.g.: https://crash-stats.mozilla.com/report/index/9bc301ae-f7c6-44dc-b14d-8efd10171205

The problem appears to be somewhere in the left calculation of what needs to get written into the buffers here https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/dom/media/AudioPacketizer.h#132
And when it calls ConvertAudioSamples() it goes out of bounds, because it doesn't do any buffer length checks.

Comment 1

[Nils Ohlmeier [:drno]]

Alex, can I ask you to have look at this one? Feel free to re-assign if you don't feel comfortable handling this.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

At minimum sec-high, since it may be a controllable write-past-end-of-buffer.
NI padenot as well since he may know where the bug comes from.

First crash is 55b3, though it may have been in 55a1; rate is low.  54 crash on Fennec is something different.

NI dveditz to confirm sec-high and comment on if he thinks a fix would be a candidate for ride-along on any further 57 point release.  (Probably not in my opinion, but we haven't seen the source of the bug yet for real assessment.)

Comment 3

[Daniel Veditz [:dveditz]]

At least sec-high. Would like to uplift a fix to 58 but it's not a chemspill for 57.

Comment 4

[Paul Adenot (:padenot)]

Andreas, this is in the audio proxy thread code.

Comment 5

[Alex Chronopoulos [:achronop]]

I have the following theory about it. The output buffer that is provided in packetizer is set here:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#559

assuming at most 48kHz stereo data. During the execution though the rate is set from MSG rate here:

https://searchfox.org/mozilla-central/source/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#1805

The MSG rate can be more than 48kHz because it's retrieved from cubeb as the preferred rate of the output device. If that's the case the packetizer attempt to write outside the allocated buffer and crashes.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Do we limit the MSG rate to 48K?  If the user has a "pro" audio card set to 96 (or 192)K, do we run MSG at that rate or at 48?

I presume it's impossible for it to change in mid-stream, but could it change if the user switches devices in mid-call?  (I'd hope not, even if it means rate conversions for the rest of the lifetime of the MSG flow).

Comment 7

[Alex Chronopoulos [:achronop]]

No we do not limit the MSG rate. If card's default rate is 96K for example MSG will operate on that rate, I believe that causes the problem. Indeed it's impossible to change in mid-call. It is set on creation and every following stream gets that rate.

For this back I will change the buffer size to be adjusted dynamically according to the rate of the graph. This is just to avoid the crash. The scenario again will not work, I plan to open a new (non sec) bug to do something more clever when that's the case (downsampling maybe).

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Hmmm.  Note that a bunch of the webrtc.org modules we use have a specific set of frequencies they support (typically 8/16/32/48K), and any others force conversions (hopefully) or fail.  I would be really tempted to limit to 48K (and I've never personally seen a >48K rate that wasn't an integer multiple of 48K).  Running at higher rates does cause extra overhead, especially if there's real audio processing happening (not just copying buffers).

Perhaps WebAudio is a reason to run >48KHz, though one could make arguments about how actually useful that is.

Comment 9

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1423770 - Dynamic buffer allocation so support high sample rate.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?It's not easy but it's possible.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? 58, 57

If not all supported branches, which bug introduced the flaw? Bug 1369967

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? No. It's not easy to exploit that fault.

How likely is this patch to cause regressions; how much testing does it need? Not likely. Every WebRTC call exercises that change. You can test it by making calls using various device sampling rates.

Comment 10

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

Comment on attachment 8937200 [details] [diff] [review]
Bug 1423770 - Dynamic buffer allocation so support high sample rate.

sec-approval+ for trunk. Please nominate a patch for beta as well.

Comment 11

[Paul Adenot (:padenot)]

Landed a rebased version without the commit message on trunk.

https://hg.mozilla.org/integration/mozilla-inbound/rev/71637b33eae2145048b1dbec306b9ce4e5d8e863

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8937200 [details] [diff] [review]
Bug 1423770 - Dynamic buffer allocation so support high sample rate.

Approval Request Comment
[Feature/Bug causing the regression]: 1369967
[User impact if declined]: Crash if the user has a high sampling rate (often requires the use of a special sound-card)
[Is this code covered by automated tests?]: No (we don't have such a sound card in automation)
[Has the fix been verified in Nightly?]: Just landed
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really.
[Why is the change risky/not risky?]: This has been well analyzed (see comments in the bug).
[String changes made/needed]:

Comment 13

[Paul Adenot (:padenot)]

(the patch in this bug applies cleanly to beta).

Comment 14

[Jean-Yves Avenard [:jya]]

note that if the MSG rate can be greater than 48kHz we have an issue there too:
https://searchfox.org/mozilla-central/rev/b1e0ae2573e88391a7ed92407568be77994c9317/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#2068

in bug 1404997, I added a change that causes any buffer overflow to crash (there was no check whatsoever prior that).

But that only prevent a sec-high issue... bug is also there

Comment 15

[Jean-Yves Avenard [:jya]]

note that the issue above is a problem only following bug 1397793, before that the rate at which we pulled from NetEQ was 32kHz and we couldn't write outside the bound of the array

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71637b33eae2

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8937200 [details] [diff] [review]
Bug 1423770 - Dynamic buffer allocation so support high sample rate.

sec-high fix, beta58+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c4f1df0162ae

Comment 19

[Paul Adenot (:padenot)]

I fixed that properly a while back (adding resamplers and stuff). Clearing NI.