Closed
Bug 922595
Opened 11 years ago
Closed 3 years ago
Signed integer overflows in WebRTC audio_coding/audio_processing
Categories
(Core :: WebRTC: Audio/Video, defect)
Tracking
()
RESOLVED
INVALID
backlog | parking-lot |
People
(Reporter: decoder, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: sec-want, testcase)
The following parts of our codebase show signed integer overflows at runtime (when running our tests, based on mozilla-inbound 8bf84234319a): # Format is: Location, Error, Test that triggered it (each in a single line) media/webrtc/trunk/webrtc/modules/audio_coding/neteq/recin.c:510 runtime error: signed integer overflow: -1599598353 * 3 cannot be represented in type 'int' /tests/dom/media/tests/mochitest/test_dataChannel_basicAudio.html media/webrtc/trunk/webrtc/modules/audio_processing/agc/digital_agc.c:208 runtime error: signed integer overflow: -1744830464 - 605880320 cannot be represented in type 'int' file:///builds/slave/test/build/tests/reftest/tests/dom/media/tests/crashtests/863929.html media/webrtc/trunk/webrtc/modules/audio_processing/agc/digital_agc.c:211 runtime error: signed integer overflow: 1610612736 + 605880320 cannot be represented in type 'int' file:///builds/slave/test/build/tests/reftest/tests/dom/media/tests/crashtests/863929.html Signed integer overflows are undefined behavior according to the C/C++ standard and discouraged by the CERT Secure Coding Standard. Even if the overflow itself is intended/checked (which should also be verified), we should not rely on the result, nor on any checks performed with the result after the computation. In the optimization stage, the compiler may assume overflow is not happening and produce wrong results as well as eliminate further checks, recognized as dead code. For further information, see the tracking bug. Marked as security sensitive until investigated because it isn't clear what's being done with the computed results. At first glance it looks like it's used for further computations and checks. Putting needinfo on rjesup according to hg blame for that particular code. If you're not the right person to take a look, could you please suggest someone? :) Thanks!
Flags: needinfo?(rjesup)
Comment 1•11 years ago
|
||
I'll investigate; all appear to be upstream issues we'll need to file.
Flags: needinfo?(rjesup)
Updated•11 years ago
|
status-firefox27:
--- → affected
tracking-firefox27:
--- → +
Comment 3•11 years ago
|
||
NI on :jesup here on next steps as this is tracked for Firefox 27 and we are already a few weeks in our Beta cycle.
Flags: needinfo?(rjesup)
Comment 4•11 years ago
|
||
I'm assuming that this still affects 28 and 29 as well (I don't know if it affects 26).
Comment 5•11 years ago
|
||
Signed overflows in the media processing code may be unavoidable given arbitrary input (e.g. clipping). While this may result in undefined behavior in terms of the signal, this should not be a security issue as it doesn't care about the overflow from a codeflow perspective. Worst case would be imperfect AGC or noise suppression. The overflow in recin.c may be officially undefined in the language spec, but timestamps are defined to wrap-around (overflow), so if this were built on a machine/compiler that didn't do the "expected" officially-undefined behavior, then NetEq would fail dramatically and WebRTC would be unusable on that platform. We can file a bug on upstream to resolve the undefined aspect of it, but I seriously doubt they'll care. Even if it failed in this way, it should not be a security concern (and on such a configuration we wouldn't enable WebRTC anyways). I suggest not tracking this bug.
Comment 6•11 years ago
|
||
Unless we're telling all the compilers we use to use -fwrapv or similar to *force* signed integer wraparound behavior here, we're just sitting waiting for a compiler's optimizer to get smart enough to do something interesting here because it knows per spec that no overflow can happen here. I'd be pretty surprised if upstream didn't care about that, knowing what Google thinks about secure coding and all. The recin.c bit looks at a glance like changing a few internalTS to uint32_t (and removing some casts) would fix it, or very nearly so. I imagine if you double-checked and found that to be correct, an upstream patch for this issue (note two functions appear to be affected) would be well-received. I can't eyeball similar solutions to the other two cases, but I'd say you should raise them in webrtc fora upstream. Poking people in #blink and asking where WebRTC is discussed would probably also get traction.
Updated•11 years ago
|
Updated•3 years ago
|
Assignee: rjesup → nobody
Updated•3 years ago
|
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•