649429 - Undefined behavior in gfxASurface::CheckSurfaceSize -- causes crashes with giant allocations

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Zack Weinberg (:zwol)]

gfxASurface::CheckSurfaceSize tries to check for signed 32-bit integer overflow with code of the form

    PRInt32 tmp = sz.width * sz.height;
    if (tmp && tmp / sz.height != sz.width)
        fail;

Unfortunately, signed integer overflow DOES NOT wrap; it's undefined behavior (I can quote chapter and verse if anyone really wants me to :) and some compilers (*cough* GCC >=4.4) will cheerfully go "well, that condition could only be true if undefined behavior had just happened, so in a correct program it can never be true, so I will delete the test entirely".  Which causes the function to not do its job.

The right way to check for signed integer overflow is

    PRUint32 tmp = PRUint32(sz.width) * sz.height;
    if (PRInt32(tmp) <= 0 || tmp / sz.height != PRUint32(sz.width)) 
        fail;

(the first cast to PRUint32 is necessary because C++ will *not* do unsigned multiplication just because the result is assigned to an unsigned variable, and the second cast avoids a warning).  Patch shortly.

Comment 1

[Zack Weinberg (:zwol)]

I should have mentioned that I caught this because of svg/crashtests/478511-1.svg.  Bug 385288 is related - SVG probably should put a limit on its pattern surfaces that's smaller than two gigabytes - but not the same.

Comment 2

[Zack Weinberg (:zwol)]

Bug number dyslexia strikes again: I meant bug 385228 (and see also bug 522502).

Comment 3

[Zack Weinberg (:zwol)]

Attachment: patch

The change to nsSVGUtils is not necessary to fix this bug, but made debugging easier, and according to the comments, was meant to happen after we got rid of non-libxul builds anyway.  Hence the double review request - joedrew for gfx, dholbert for nsSVGUtils.

Comment 4

[Shawn Wilsher :sdwilsh]

(zwol doesn't have the bits to set this, but asked me to)

Comment 5

[Zack Weinberg (:zwol)]

To make the security implications of this bug a little clearer: SVG pattern surfaces try to allocate (4 * x * y) bytes, where both x and y are directly controllable by web content.  CheckSurfaceSize is the only thing that prevents that arithmetic from overflowing and wrapping around to a much smaller size.  Allocation overflows of that form have been remote-code-execution exploits in the past.  I don't know whether this one is that bad, but it's certainly remote-instant-crash for a 32-bit build and remote-throw-your-computer-into-swap-thrashing for a 64-bit build.  (It's probably *not* possible to get a 64-bit build to overflow an allocation, though.)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 525458 [details] [diff] [review]
patch

>diff --git a/gfx/thebes/gfxASurface.cpp b/gfx/thebes/gfxASurface.cpp
>+    PRUint32 allocation = area * 4;
>+    if (PRInt32(area) <= 0 || allocation / 4 != area) {
>         NS_WARNING("Surface size too large (would overflow)!");
>         return PR_FALSE;

You want "PRInt32(allocation) <= 0"  (s/area/allocation/) in the if condition there, I think.

Comment 7

[Zack Weinberg (:zwol)]

Attachment: v1.1

Doh! Yes, I do want "allocation" there.  Thanks for the catch.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 525469 [details] [diff] [review]
v1.1

Looks good to me!  (Thanks for addressing the related XXXdholbert post-libxul-world cleanup - I'd forgotten about that :))

Comment 9

[Zack Weinberg (:zwol)]

Attachment: v2

It occurred to me this morning that we'd probably get better code generation (no divides, for one thing) if we just did a widening multiply to PRUint64 and then checked to see if it was bigger than PR_INT32_MAX.  And indeed it is so with gcc.  Can't say for MSVC.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

I think we should be using CheckedInt instead of checking for overflow manually.

Comment 11

[Zack Weinberg (:zwol)]

Attachment: v3

(In reply to comment #10)
> I think we should be using CheckedInt instead of checking for overflow
> manually.

I didn't know we had that.  Here's another revision.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 527611 [details] [diff] [review]
v3

It would be good if the XXXdholbert part was a separate patch, but other than that this looks good.

Comment 13

[Zack Weinberg (:zwol)]

I can split it for checkin if you would prefer.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

That would be great.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: split for checkin, 1/2

as will be checked in assuming try is green, 1/2 (gfx part)

Comment 16

[Zack Weinberg (:zwol)]

Attachment: split for checkin, 2/2

2/2 (layout part)

Comment 17

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/90934a261fac
http://hg.mozilla.org/mozilla-central/rev/33b198be1aa3