368675 - integer overflows in nsSVGFilterFrame::FilterPaint

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[georgi - hopefully not receiving bugspam]

Attachment: dafilter.xml

integer overflows in nsSVGFilterFrame::FilterPaint 
nsSVGFilterFrame::FilterPaint does some computations with |width|
|height| |s1| |s2| and |stride| - some of them may overflow.

testcase to follow.

nsSVGFilterFrame.cpp
  
  PRInt32 filterResX = PRInt32(s1 * width + 0.5);
  PRInt32 filterResY = PRInt32(s2 * height + 0.5);

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1225795008 (LWP 4910)]
0xb5039767 in nsSVGFilterFrame::FilterPaint (this=0x8a76a60, 
    aContext=0xbfe58ccc, aTarget=0x8a76bf0)
(gdb) li
397         for (PRUint32 yy=0; yy<filterResY; yy++)
398           for (PRUint32 xx=0; xx<filterResX; xx++) {
399             alphaData[stride*yy + 4*xx]     = 0;
400             alphaData[stride*yy + 4*xx + 1] = 0;
401             alphaData[stride*yy + 4*xx + 2] = 0;
402             alphaData[stride*yy + 4*xx + 3] = data[stride*yy + 4*xx + 3];
403         }
(gdb) p filterResX
$1 = 33333
(gdb) p filterResY
$2 = 33333
(gdb) x/i $eip
0xb5039767 <_ZN16nsSVGFilterFrame11FilterPaintEP16nsSVGRenderStateP16nsISVGChildFrame+2889>:    movzbl (%eax),%eax
(gdb) p/x $eax
$3 = 0xb2323003
(gdb) x/4x $eax
0xb2323003:     Cannot access memory at address 0xb2323003
(gdb) p stride
$4 = 133332

Comment 1

[Daniel Veditz [:dveditz]]

Roc: if not you please find someone to own this bug.

Comment 2

[Daniel Veditz [:dveditz]]

Sorry, blame says tor.

Comment 3

[georgi - hopefully not receiving bugspam]

according to coverity CID: 1083 from run 176:
Description: Dereferencing freed pointer "surface" in call to function "_moz_cairo_image_surface_get_data"

in this method may be use after free. the basic codepath is:
378  	  cairo_surface_destroy(surface);
...
400  	    PRUint8 *data = cairo_image_surface_get_data(surface);

Comment 4

[tor]

Attachment: decrease filter resolution when needed

Comment 5

[chris hofmann]

dan/pav/roc, can you review?

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 256110 [details] [diff] [review]
decrease filter resolution when needed

I wonder if 2K is big enough -- my screen at work is 1920x1200, and the driver in my laptop supports higher. But solves the problem: r=dveditz

Comment 7

[tor]

Comment on attachment 256110 [details] [diff] [review]
decrease filter resolution when needed

Donations of a larger monitor (currently working with 1400x1050 and 1280x1024) cheerfully accepted.

Seriously, a number of the filter are fairly slow, so this is in part a denial of service prevention.  However it would still be pretty easy for content to request a filter that would take an unreasonable amount of time to compute.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #3)
> according to coverity CID: 1083 from run 176:
> Description: Dereferencing freed pointer "surface" in call to function
> "_moz_cairo_image_surface_get_data"

Run 176 was Jan 16, I guess they don't scan the codebase all that often. That
code was removed with revision 1.27 on Jan 22 (and isn't related to the integer
overflow in this bug in any case).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 256110 [details] [diff] [review]
decrease filter resolution when needed

I would make the limit bigger, say 16384 (which is near the maximum it could be). I can easily imagine people want to render things that are bigger than the screen size.

Also, use PRUint32() constructor-style casts instead of those NS_STATIC_CAST casts.

Comment 10

[georgi - hopefully not receiving bugspam]

cloned comment #3 to Bug 372353

Comment 11

[tor]

Attachment: suggested changes plus more memory allocation checking

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 258002 [details] [diff] [review]
suggested changes plus more memory allocation checking

r=dveditz

Comment 13

[tor]

Attachment: checkin version - update to tip

Comment 14

[tor]

Checked in.