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

RESOLVED FIXED in Firefox 52

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
15
RESOLVED FIXED
a year ago
29 days ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

({csectype-bounds, sec-low})

unspecified
mozilla52
csectype-bounds, sec-low
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52-])

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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
(Assignee)

Updated

a year ago
Assignee: nobody → dminor
Group: core-security
Status: NEW → ASSIGNED
Rank: 15
(Assignee)

Comment 1

a year ago
Created attachment 8803916 [details] [diff] [review]
Change fs to be unsigned and increase maximum buffer size.
Attachment #8803916 - Flags: review?(rjesup)
Group: core-security → media-core-security
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.
Attachment #8803916 - Flags: review?(rjesup) → review+
(Assignee)

Updated

a year ago
Blocks: 1313406
Dan: what concern caused you to mark this as a security bug? The sec-triage team is unsure how to rate this.
Flags: needinfo?(dminor)
(Assignee)

Comment 4

a year ago
(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.
Flags: needinfo?(dminor)
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()
Attachment #8803916 - Flags: review+ → review-
(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.
Keywords: sec-low
Keywords: csectype-bounds
(Assignee)

Comment 7

a year ago
Created attachment 8805532 [details] [diff] [review]
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.
Attachment #8805532 - Flags: review?(rjesup)
(Assignee)

Comment 8

a year ago
(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.
(Assignee)

Comment 9

a year ago
First push was to: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=355c7ec8b68e965787e18517eeef2a693434992a
Attachment #8805532 - Flags: review?(rjesup) → review+
(Assignee)

Comment 10

a year ago
Second push to: https://hg.mozilla.org/integration/mozilla-inbound/rev/c136ac92dcdc37dfd09f4868887f1ccf86522989
https://hg.mozilla.org/mozilla-central/rev/355c7ec8b68e
https://hg.mozilla.org/mozilla-central/rev/c136ac92dcdc
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.