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

The following part 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)

image/src/imgFrame.cpp:58
runtime error: signed integer overflow: 65535 * 65535 cannot be represented in type 'int'
file:///builds/slave/test/build/tests/reftest/tests/image/test/crashtests/invalid-size.gif


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 this overflow is indeed checked afterwards:

>  // check to make sure we don't overflow a 32-bit
>  int32_t tmp = aWidth * aHeight;
>  if (MOZ_UNLIKELY(tmp / aHeight != aWidth)) {
>    NS_WARNING("width or height too large");
>    return false;
>  }

As the if branch is only hit in the overflow case, an optimizing compiler could completely remove it, assuming that overflow does not happen.

Putting needinfo on ehsan 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!

	(no longer active)
	Comment 1 • 11 years ago

I know nothing about that code, sorry.

	Benoit Jacob [:bjacob] (mostly away)
	Comment 3 • 11 years ago

Comment on attachment 813118 [details] [diff] [review]
Use CheckedInt to check for overflow

Review of attachment 813118 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments:

::: image/src/imgFrame.cpp
@@ +57,5 @@
>  
>    // check to make sure we don't overflow a 32-bit
> +  CheckedInt<int32_t> checkForOverflow(aWidth);
> +  checkForOverflow *= aHeight; 
> +  checkForOverflow *= 4;

I would prefer if that variable was named more descriptively after what the mathematical value represents. For example: requiredBytes, or checkedRequiredBytes.

Also, you can put this in one expression, and use convenience typedefs:
CheckedInt32 checkedRequiredBytes = CheckedInt32(aWidth) * aHeight * 4;

(this relies on left-to-right associativity of *. If you're not comfortable with that, you can also wrap aHeight into CheckedInt32(...).)

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 4 • 11 years ago

Review comments applied.  https://tbpl.mozilla.org/?tree=Try&rev=a528531ed7f7

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 5 • 11 years ago

Comment on attachment 813127 [details] [diff] [review]
Use CheckedInt to check for overflow.  Carry r=bjacob

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
See tracking bug 919486 - CERT Secure Coding Standard explicitly recommends to avoid this behavior
 
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No more than the original code did - we are just trying to check for the overflow in a more reliable and well defined manner.

Which older supported branches are affected by this flaw?
Since before the end of 2011 (the first version of the file.)

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.  CheckedInt is a well tested and supported class.

	Al Billings [:abillings - ex-MoCo]
	Comment 6 • 11 years ago

Comment on attachment 813127 [details] [diff] [review]
Use CheckedInt to check for overflow.  Carry r=bjacob

Since this is a sec-want (and not high or critical), it doesn't need sec-approval. Go ahead and check it into trunk. I expect that we won't take it on branches given the low severity.

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c00387255d25

	Phil Ringnalda (:philor)
	Comment 8 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/c00387255d25