1500012 - Unsafe usage of CheckedInt #3

Status: UNCONFIRMED

(Core :: Graphics, defect, P3)

Description

[r2]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:61.0) Gecko/20100101 Firefox/61.0

Steps to reproduce:

Same as bug#1495011 and bug#1500011:

static inline int32_t
GetStrideGap(int32_t aWidth, SurfaceFormat aFormat, int32_t aStride)
{
  CheckedInt32 used = CheckedInt32(aWidth) * BytesPerPixel(aFormat);
  if (!used.isValid() || used.value() < 0) {
    return -1;
  }
  return aStride - used.value();
}

https://searchfox.org/mozilla-central/source/gfx/2d/Swizzle.cpp#277

The checked variable is converted back to raw value and then used for arithmetic calculations one more time. aStride is int32_t, thus it can affect both x86 and x64.

Comment 1

[:Gijs (out for now; he/him)]

Looks like this code was introduced in sec bug 1463244... Lee, you wrote the patch there, can you take a look or pass this to someone who can? Thank you.

Comment 2

[Daniel Veditz [:dveditz]]

It looks like the results of GetStrideGap are checked at every use and a negative value is handled as an error.

Comment 3

[Lee Salzman [:lsalzman]]

aStride is a positive int32_t value, and used.value() is a positive int32_t value. So in the worst case aStride - used.value() will be negative. All callers of GetStrideGap check that the value is positive, and if it is negative they just return. Plus there are debug asserts. I don't see how this could be exploited at all?

Comment 4

[r2]

Indeed, the callers check for a negative returned value, so this case cannot be exploited in practice - at least not with the current list of callers.

However the main problem with potentially unsafe uses of CheckedInt (and why I decided to audit them in the first place) is that such uses introduce a dangerous programming pattern, that undermines the security of the product in the strategic perspective. Extra arithmetics on unwrapped CheckedInt is a terrible anti-pattern, because CheckedInt is normally used with the code that is known to have a high risk of arithmetic overflows. Thus an unsafe use of CheckdInt still has a high risk to overflow after the checked operation, while giving the developer and reviewers a false sense of security.

New developers learn by reading the existing code, and pick up the idioms from it. As such, the security risk from unsafe uses of CheckedInt will propagate in the codebase over time. 

I strongly recommend that all uses of CheckedInt are made clean, while the usage of this idiom is still sparse: that is, only extract .value() after we're done with *all* arithmetic operations on the variable.