441372 - Potential integer overflow in gfxImageSurface::gfxImageSurface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Brandon Sterne (:bsterne)]

Drew Yao reported this issue to security@mo.  From his email:

Potential integer overflow in gfxImageSurface::gfxImageSurface
This was from looking at the code for FF3.  I have not looked at FF2.

gfxImageSurface::gfxImageSurface(const gfxIntSize& size, gfxImageFormat format) :
    mSize(size), mFormat(format)
{
    mStride = ComputeStride();

    if (!CheckSurfaceSize(size))
        return;

    // if we have a zero-sized surface, just set mData to nsnull
    if (mSize.height * mStride > 0) {
        mData = (unsigned char *) malloc(mSize.height * mStride);   //<-- potential overflow here.

CheckSurfaceSize does some validation of mSize.height and mSize.width, and mStride is based on the width.
I think that given these checks, an overflow is normally impossible.


/* static */
PRBool
gfxASurface::CheckSurfaceSize(const gfxIntSize& sz, PRInt32 limit)
{
    if (sz.width < 0 || sz.height < 0) {
        NS_WARNING("Surface width or height < 0!");
        return PR_FALSE;
    }

#if defined(XP_MACOSX)
    // CoreGraphics is limited to images < 32K in *height*, so clamp all surfaces on the Mac to that height
    if (sz.height > SHRT_MAX) {
        NS_WARNING("Surface size too large (would overflow)!");
        return PR_FALSE;
    }
#endif

    // check to make sure we don't overflow a 32-bit
    PRInt32 tmp = sz.width * sz.height;
    if (tmp && tmp / sz.height != sz.width) {
        NS_WARNING("Surface size too large (would overflow)!");
        return PR_FALSE;
    }

    // always assume 4-byte stride
    tmp = tmp * 4;
    if (tmp && tmp / 4 != sz.width * sz.height) {
        NS_WARNING("Surface size too large (would overflow)!");
        return PR_FALSE;
    }

    // reject images with sides bigger than limit
    if (limit &&
        (sz.width > limit || sz.height > limit))
        return PR_FALSE;

    return PR_TRUE;
}

However, these integer overflow checks all use signed arithmetic and allow the overflow to happen before checking it.   gfxIntSize is defined in gfxPoint.h with width and height as signed ints.   The behavior of signed integer overflow is undefined in the C standard, and in some cases compilers can optimize these checks away by assuming that it could never happen.
See http://www.kb.cert.org/vuls/id/162289 for more on that.
On Mac OS X with gcc 4.0.1, none of the checks get optimized away, but it's something to consider as a hardening measure(and possibly a vulnerability on some platforms).
The preferred method of checking would be to either use unsigned arithmetic, or to use signed but don't let the overflow happen, e.g.
if (sz.width > INT_MAX / sz.height) error

I was trying to reproduce it and couldn't, but just for fun, the attached jpg file has height = 2 bytes at offset 0x6d2 and width = 2 bytes at offset 0x6d4.  It doesn't trigger the overflow, but it does try to allocate a huge buffer.

Comment 1

[Drew Yao]

I just realized that the cert link I gave only mentions integer overflows on pointers being undefined.  

Better links, re: signed integer overflow:
https://www.securecoding.cert.org/confluence/display/seccode/MSC15-A.+Do+not+depend+on+undefined+behavior
https://www.securecoding.cert.org/confluence/display/seccode/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow

Comment 2

[Daniel Veditz [:dveditz]]

Vlad: please investigate or assign to an appropriate member of your team.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

This bug probably shouldn't be critical. It is more a theoretical problem that should be fixed, than a drop everything and fix it now thing.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Bug 555798 would also help here.

Comment 5

[Joe Drew (not getting mail)]

Our current compilers don't cause issues; this is purely something for us to be aware of in the future.

This can probably be opened up, too, but I'll leave that for someone else to decide.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

This has already been fixed by using CheckedInt in CheckSurfaceSize.