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

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dminor, Assigned: dminor)

Tracking

({csectype-bounds, sec-low})

unspecified
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed)

Details

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

Attachments

(2 attachments)

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: nobody → dminor
Group: core-security
Status: NEW → ASSIGNED
Rank: 15
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+
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)
(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
Sorry, I missed that you had changed from r+ to r- before I pushed this morning.
Attachment #8805532 - Flags: review?(rjesup)
(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.
Attachment #8805532 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/355c7ec8b68e
https://hg.mozilla.org/mozilla-central/rev/c136ac92dcdc
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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.