The default bug view has changed. See this FAQ.

Additional problems in Firefox 2.0 SVG "_cairo_pen_init"

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: dveditz, Assigned: vlad)

Tracking

({fixed1.8.1.4})

1.8 Branch
fixed1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate])

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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+
(Assignee)

Comment 1

10 years ago
Created attachment 262171 [details] [diff] [review]
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.
Attachment #262171 - Flags: review?(dveditz)
(Assignee)

Comment 2

10 years ago
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.



(Reporter)

Updated

10 years ago
Whiteboard: needs r=dveditz
(Reporter)

Comment 4

10 years ago
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+
(Assignee)

Comment 5

10 years ago
Created attachment 262927 [details] [diff] [review]
add additional checking, v2

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?
(Reporter)

Comment 6

10 years ago
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+
(Assignee)

Comment 7

10 years ago
/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
(Reporter)

Updated

10 years ago
Whiteboard: [sg:investigate]
(Reporter)

Updated

10 years ago
Group: security
Resolving as FIXED, correct me if I'm wrong!
Status: NEW → RESOLVED
Last Resolved: 8 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.