The default bug view has changed. See this FAQ.

Potential integer overflow in gfxImageSurface::gfxImageSurface

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: bsterne, Assigned: jrmuizel)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

(Reporter)

Description

9 years ago
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.
(Reporter)

Updated

9 years ago
Whiteboard: [sg:investigate]

Comment 1

9 years ago
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
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

Updated

7 years ago
Whiteboard: [sg:investigate] → [sg:critical?]
Assignee: vladimir → jmuizelaar

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][ccbr]
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
Bug 555798 would also help here.

Updated

7 years ago
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]
(Assignee)

Comment 6

6 years ago
This has already been fixed by using CheckedInt in CheckSurfaceSize.
Group: core-security
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.