1312431 - Support for higher sample rates in dtmfinband.cc can not be reached

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

As jseward pointed out in [1] we can never hit the code added in [2] to support higher sample rates in dtmfinband.cc because the sample rate is a 16 bit signed value. There is a similar problem with DtmfInband::SetSampleRate and potentially elsewhere in that file.

As far as I know, negative sample rates don't exist in the webrtc code, so a simple fix may be to just make these values unsigned.

I believe this code is removed in newer versions of webrtc.org, so the update to branch 49 may also "fix" this.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1291715#c104
[2] https://hg.mozilla.org/integration/autoland/rev/5f4460d092e5

Comment 1

[Dan Minor [:dminor]]

Attachment: Change fs to be unsigned and increase maximum buffer size.

Comment 2

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

Comment on attachment 8803916 [details] [diff] [review]
Change fs to be unsigned and increase maximum buffer size.

Review of attachment 8803916 [details] [diff] [review]:
-----------------------------------------------------------------

Add a #define for MAX_DTMF_SAMPLERATE (or better reuse an existing define), and use that to determine the buffer size instead of a hard constant.

Comment 3

[Daniel Veditz [:dveditz]]

Dan: what concern caused you to mark this as a security bug? The sec-triage team is unsure how to rate this.

Comment 4

[Dan Minor [:dminor]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Dan: what concern caused you to mark this as a security bug? The sec-triage
> team is unsure how to rate this.

I was worried about the potential buffer overflow I noticed while I was fixing this. In hindsight, I'm not sure if it needed to be marked as security bug. At any rate, DTMF is currently preffed off, so the impact of this is pretty low.

Comment 5

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

Comment on attachment 8803916 [details] [diff] [review]
Change fs to be unsigned and increase maximum buffer size.

Review of attachment 8803916 [details] [diff] [review]:
-----------------------------------------------------------------

You need to modify output_mixer.cc and channel.cc where they call Get10msTone()

Comment 6

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

(In reply to Dan Minor [:dminor] from comment #4)
> (In reply to Daniel Veditz [:dveditz] from comment #3)
> > Dan: what concern caused you to mark this as a security bug? The sec-triage
> > team is unsure how to rate this.
> 
> I was worried about the potential buffer overflow I noticed while I was
> fixing this. In hindsight, I'm not sure if it needed to be marked as
> security bug. At any rate, DTMF is currently preffed off, so the impact of
> this is pretty low.

It would potentially cause a security issue if preffed on if the mixing frequency was set to >32000 Hz.  44100 and 48000 hz are allowed by the generator.  I believe this is impossible since the transmit mixer has a max of 32000 hz currently (kAudioProcMaxNativeSampleRateHz = 32000).

Still, the dtmf generator allows 44100 & 48000, but asks for a 320-sample buffer, which is wrong.

Comment 7

[Dan Minor [:dminor]]

Attachment: Update call sites in channel.cc and output_mixer.cc match new buffer size

Sorry, I missed that you had changed from r+ to r- before I pushed this morning.

Comment 8

[Dan Minor [:dminor]]

(In reply to Randell Jesup [:jesup] from comment #6)

> It would potentially cause a security issue if preffed on if the mixing
> frequency was set to >32000 Hz.  44100 and 48000 hz are allowed by the
> generator.  I believe this is impossible since the transmit mixer has a max
> of 32000 hz currently (kAudioProcMaxNativeSampleRateHz = 32000).

I hit an assertion when writing the insertDTMF mochitest from an attempt to set sampling frequency to 48000, but I never saw this when testing using the webrtc.org sample code.

Comment 9

[Dan Minor [:dminor]]

First push was to: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=355c7ec8b68e965787e18517eeef2a693434992a

Comment 10

[Dan Minor [:dminor]]

Second push to: https://hg.mozilla.org/integration/mozilla-inbound/rev/c136ac92dcdc37dfd09f4868887f1ccf86522989

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/355c7ec8b68e
https://hg.mozilla.org/mozilla-central/rev/c136ac92dcdc