351296 - int overflow in nsCanvasRenderingContext2D::GetImageData

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[georgi - hopefully not receiving bugspam]

int overflow in nsCanvasRenderingContext2D::GetImageData

there is int overflow in nsCanvasRenderingContext2D::GetImageData

---------
  if (w <= 0 || h <= 0 ||
        x + w > mWidth || y + h > mHeight)
        return NS_ERROR_DOM_SYNTAX_ERR;
---------

with luser controlled w,h,x,y the above checks are close to useless.

later:

   allocatedSurfaceData = new PRUint8[w * h * 4];
....
   nsAutoArrayPtr<jsval> jsvector(new jsval[w * h * 4]);

gdb session:
Breakpoint 1, nsCanvasRenderingContext2D::GetImageData (this=0x8dab428)
    at /opt/joro/firefox/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2846
2846            allocatedSurfaceData = new PRUint8[w * h * 4];
(gdb) p w
$1 = 536870915
(gdb) p h
$2 = 536870915
(gdb) p w*h*4
$3 = 36
(gdb) p x
$4 = -536870914
(gdb) p y
$5 = -536870914
(gdb) cont
Continuing.
[Thread -1303647312 (LWP 15247) exited]

Program received signal SIGSEGV, Segmentation fault.
0xb7edb833 in pixman_fill_rect_32bpp (dst=0x8da8c78, xDst=0, yDst=0, width=3, 
    height=1, pixel=0xbfa6a154)
    at /opt/joro/firefox/mozilla/gfx/cairo/libpixman/src/icrect.c:143
143                 *(uint32_t *)data = int_pixel;
Current language:  auto; currently c
(gdb) x/4x data
0x88da8c34:     Cannot access memory at address 0x88da8c34

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: testcase

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nominating for 1.8; I'll have a safe patch for this tonight.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix image-related overflows in canvas

Fix the overflows.  We start by validating mWidth/mHeight (dimensions of the canvas itself), which also fixes bug 348351, and then we make sure that any passed-in x/y/w/h are >= 0, and that (x+w) and (y+h) are <= mWidth/mHeight.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This fixes 351295 as well.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 237529 [details] [diff] [review]
fix image-related overflows in canvas

+    if (x + w > mWidth || y + h > mHeight)

what if x+w overflows the signed integer, becoming negative?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ugh, yes; I really should just keep those in unsigned ints instead of signed.  New patch inc soon.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: overflow fix #2

How about something like this?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: canvas overflow fix (right file this time)

Attached the right file this time.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 237645 [details] [diff] [review]
canvas overflow fix (right file this time)

+CheckSaneSubrectSize (PRInt32 x, PRInt32 y, PRInt32 w, PRInt32 h, PRInt32 realWidth, PRInt32 realHeight)

80 column lines please

also, I'd have made this a member function instead, to avoid having to pass mWidth/mHeight, but this works too

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(This code wasn't in 1.8.0.x)

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in to trunk

Comment 12

[Mike Schroepfer]

If this code is on 1.8.1 please nom

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 237645 [details] [diff] [review]
canvas overflow fix (right file this time)

It is, it's already blocking.  Wanted to get a nightly out tomorrow to make sure things are fixed, then land on 1.8.1.

Comment 14

[Mike Schroepfer]

Comment on attachment 237645 [details] [diff] [review]
canvas overflow fix (right file this time)

a=schrep for 18drivers for topcrash fix.  Thanks vlad!

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in to 1.8.1 (and trunk)

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No, wait.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No, it's fixed, there was some update confusion when I tested with a trunk nightly.  Can someone other than me check this bug and the two dependent bugs' testcases here and verify all 3?  Should be no crashes, and some errors to error console.

Comment 18

[georgi - hopefully not receiving bugspam]

(In reply to comment #17)
> Can someone other than me check this bug and the two dependent bugs'
> testcases here and verify all 3?  Should be no crashes, and some errors to
> error console.
> 

on linux the bugs about getimagedata and putimagedata don't crash and give the expected errors in the console.

setdimensions bug causes exit due to X problem, but it did the same way before.

CC bclary to check on more platforms.