Closed Bug 372285 Opened 17 years ago Closed 15 years ago

Additional problems in Firefox 2.0 SVG "_cairo_pen_init"

Categories

(Core :: SVG, defect)

1.8 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dveditz, Assigned: vlad)

Details

(Keywords: fixed1.8.1.4, Whiteboard: [sg:investigate])

Attachments

(2 files)

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)
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.3+
Add additional checking to number of pen points before allocating.  Patch is good for 1.8.0 and 1.8.
Attachment #262171 - Flags: review?(dveditz)
Er, sorry, not 1.8.0, just 1.8
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.



Whiteboard: needs r=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
Attachment #262171 - Flags: review?(dveditz) → review+
Patch with extra check added, also pulling out magic constant into a #define.
Attachment #262927 - Flags: review?(dveditz)
Attachment #262927 - Flags: approval1.8.1.4?
Comment on attachment 262927 [details] [diff] [review]
add additional checking, v2

thanks
r=dveditz

approved for 1.8.1.4, a=dveditz
Attachment #262927 - Flags: review?(dveditz)
Attachment #262927 - Flags: review+
Attachment #262927 - Flags: approval1.8.1.4?
Attachment #262927 - Flags: approval1.8.1.4+
/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.
Keywords: fixed1.8.1.4
Whiteboard: needs r=dveditz
Whiteboard: [sg:investigate]
Group: security
Resolving as FIXED, correct me if I'm wrong!
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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).
You need to log in before you can comment on or make changes to this bug.