Last Comment Bug 441372 - Potential integer overflow in gfxImageSurface::gfxImageSurface
: Potential integer overflow in gfxImageSurface::gfxImageSurface
Status: RESOLVED FIXED
[sg:want][ccbr]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jeff Muizelaar [:jrmuizel]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-23 12:38 PDT by Brandon Sterne (:bsterne)
Modified: 2011-08-05 15:17 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Brandon Sterne (:bsterne) 2008-06-23 12:38:35 PDT
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 2008-06-23 18:12:41 PDT
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] 2008-06-30 22:55:24 PDT
Vlad: please investigate or assign to an appropriate member of your team.
Comment 3 Jeff Muizelaar [:jrmuizel] 2010-04-20 14:16:46 PDT
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] 2010-04-20 14:28:38 PDT
Bug 555798 would also help here.
Comment 5 Joe Drew (not getting mail) 2010-05-11 14:25:36 PDT
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] 2011-08-05 15:17:17 PDT
This has already been fixed by using CheckedInt in CheckSurfaceSize.

Note You need to log in before you can comment on or make changes to this bug.