Christian Holler (:decoder) Reporter
	Description • 11 years ago

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!

	Randell Jesup [:jesup] (needinfo me)
	Comment 1 • 11 years ago

I'll investigate; all appear to be upstream issues we'll need to file.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 2 • 11 years ago

Assigning to Jesup while he investigates.

	bhavana bajaj [:bajaj]
	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.

	Al Billings [:abillings - ex-MoCo]
	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).

	Randell Jesup [:jesup] (needinfo me)
	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.

	Jeff Walden [:Waldo]
	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.

	Maire Reavy [:mreavy]
	Comment 7 • 9 years ago

This is an upstream bug