Closed Bug 441372 Opened 13 years ago Closed 10 years ago

Potential integer overflow in gfxImageSurface::gfxImageSurface


(Core :: Graphics, defect)

Not set





(Reporter: bsterne, Assigned: jrmuizel)


(Whiteboard: [sg:want][ccbr])

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))

    // 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 */
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;

    // 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 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.
Whiteboard: [sg:investigate]
Vlad: please investigate or assign to an appropriate member of your team.
Assignee: nobody → vladimir
Product: Core → Core Graveyard
Component: GFX → GFX: Thebes
Product: Core Graveyard → Core
QA Contact: general → thebes
Whiteboard: [sg:investigate] → [sg:critical?]
Assignee: vladimir → jmuizelaar
Whiteboard: [sg:critical?] → [sg:critical?][ccbr]
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.
Bug 555798 would also help here.
Whiteboard: [sg:critical?][ccbr] → [sg:critical?][ccbr][critsmash:will likely drop]
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.
Whiteboard: [sg:critical?][ccbr][critsmash:will likely drop] → [sg:want][ccbr]
This has already been fixed by using CheckedInt in CheckSurfaceSize.
Group: core-security
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.