372285 - Additional problems in Firefox 2.0 SVG "_cairo_pen_init"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Veditz [:dveditz]]

Spun-off from bug 360645:

--- Comment #16  G30rgi   2007-02-25 11:49:10 PST ---

+
+    /* Make sure we don't overflow the size_t for malloc */
+    if (pen->num_vertices > 0xffff) {
+        return CAIRO_STATUS_NO_MEMORY;
+    }
+

|pen->num_vertices| seems of type |signed int|

|_cairo_pen_vertices_needed| does some operations on |doubles| and returns
*signed int*.

so negative int will pass the check and cause trouble in malloc.

is it sure that |num_vertices| is nonnegativ in all cases?

--- Comment #17 Robert C. Seacord  2007-02-28 10:48:16 PST ---

Presumably pen->num_vertices could be declared as an unsigned int as you would
not normally expect a value like this to be negative.

A safer, more localized fix may be to simply check for a lower bound, i.e.:

    if ( (pen->num_vertices <= 0) || (pen->num_vertices > 0xffff) ) {
        return CAIRO_STATUS_NO_MEMORY;
    }

(It is usually best to eliminate the possibility of allocating zero bytes as
this behavior is implementation defined). 

--- Comment #18 G30rgi  2007-03-01 00:38:44 PST ---

another potential solution (at least for gcc) is:
(pen->num_vertices > 0xffffU)

Comment 1

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

Attachment: add additional checking to number of pen points

Add additional checking to number of pen points before allocating.  Patch is good for 1.8.0 and 1.8.

Comment 2

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

Er, sorry, not 1.8.0, just 1.8

Comment 3

[georgi - hopefully not receiving bugspam]

hi vlad,

int num_vertices;
...
[1]    num_vertices = pen->num_vertices + num_points;
    if (num_vertices > 0xffff)
	return CAIRO_STATUS_NO_MEMORY;

[1] may overflow in theory - you seem to make assumptions about |pen->num_vertices|.

if your goal is to save cpu cycles use unsigned types.

note that i don't claim [1] is a bug or is exploitable, but makes analysis harder.

Comment 4

[Daniel Veditz [:dveditz]]

Comment on attachment 262171 [details] [diff] [review]
add additional checking to number of pen points

>+    if (num_points <= 0 || num_points > 0xffff)
>+	return CAIRO_STATUS_NO_MEMORY;
>+
>     num_vertices = pen->num_vertices + num_points;

I'm satisfied that pen->num_vertices is within the bounds checked by _cairo_pen_init in our current code, but Georgi's right that having a local check would have saved me a lot of work code reading to do so. It wouldn't hurt to re-check that it's not negative here--or at least assert()--in case more ways to change num_vertices are added to cairo in the future.

We can't just switch to unsigned ints because that will hellify the task of merging in updates from the cairo team.

r=dveditz

Comment 5

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

Attachment: add additional checking, v2

Patch with extra check added, also pulling out magic constant into a #define.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 262927 [details] [diff] [review]
add additional checking, v2

thanks
r=dveditz

approved for 1.8.1.4, a=dveditz

Comment 7

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

/cvsroot/mozilla/gfx/cairo/cairo/src/cairo-pen.c,v  <--  cairo-pen.c
new revision: 1.1.4.4; previous revision: 1.1.4.3

in 1.8 branch.

Comment 8

[Nochum Sossonko [:Natch]]

Resolving as FIXED, correct me if I'm wrong!

Comment 9

[Jonathan Watt [:jwatt]]

I don't know if there was a reason Vlad left this open.

Note that normally you should not resolve bugs that are assigned to someone (unless you are that someone).