Closed
Bug 1312431
Opened 8 years ago
Closed 8 years ago
Support for higher sample rates in dtmfinband.cc can not be reached
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: dminor, Assigned: dminor)
References
Details
(Keywords: csectype-bounds, sec-low, Whiteboard: [post-critsmash-triage][adv-main52-])
Attachments
(2 files)
3.96 KB,
patch
|
jesup
:
review-
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Assignee: nobody → dminor
Group: core-security
Status: NEW → ASSIGNED
Rank: 15
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8803916 -
Flags: review?(rjesup)
Updated•8 years ago
|
Group: core-security → media-core-security
Comment 2•8 years ago
|
||
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+
Comment 3•8 years ago
|
||
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•8 years 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 5•8 years ago
|
||
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-
Comment 6•8 years ago
|
||
(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
Updated•8 years ago
|
Keywords: csectype-bounds
Assignee | ||
Comment 7•8 years ago
|
||
Sorry, I missed that you had changed from r+ to r- before I pushed this morning.
Attachment #8805532 -
Flags: review?(rjesup)
Assignee | ||
Comment 8•8 years 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•8 years ago
|
||
First push was to: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=355c7ec8b68e965787e18517eeef2a693434992a
Updated•8 years ago
|
Attachment #8805532 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 10•8 years ago
|
||
Second push to: https://hg.mozilla.org/integration/mozilla-inbound/rev/c136ac92dcdc37dfd09f4868887f1ccf86522989
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/355c7ec8b68e https://hg.mozilla.org/mozilla-central/rev/c136ac92dcdc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Group: media-core-security → core-security-release
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52-]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•