369249 - Crash [@ nsSVGPathSegList::GetItem] with pathSegList.getItem(-1);

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes firefox)

The testcase triggers

###!!! ASSERTION: nsVoidArray::FastElementAt: index out of range: '0 <= aIndex && aIndex < Count()', file ../../dist/include/xpcom/nsVoidArray.h, line 72

followed by a crash.  Note the use of FastElementAt rather than SafeElementAt -- that means there's no runtime check in opt builds.

A simple fix would be to check that the PRInt32-casted index is nonnegative in nsSVGPathSegList::GetItem and the other functions.  But I think this code would be less confused if mSegments were an nsTArray (which uses PRUInt32 indices) instead of an nsVoidArray (which uses PRInt32 indices).

Comment 1

[Boris Zbarsky [:bzbarsky]]

It's an nsCOMArray, not a raw nsVoidArray.  And this code should just be using SafeObjectAt() instead of rolling their own.

Comment 2

[Brendan Eich [:brendan]]

Who is minding the SVG store?

/be

Comment 3

[Jesse Ruderman]

As dveditz points out, an out-of-bounds array read can be worse than maybe reading random memory (sg:moderate) depending on the data types involved.  For example, a virtual function call on a bogus pointer can result in arbitrary code execution (sg:critical).

In this bug, a bogus nsIDOMSVGPathSeg pointer is returned to script.  I think a simple QueryInterface call would end up being a virtual function call on a bogus pointer under an attacker's control.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

Better to use the right cast ;)
IMO, we want to use the explicit 
index >= NS_STATIC_CAST(PRUint32, mSegments.Count()) test so that
NS_ERROR_DOM_INDEX_SIZE_ERR can be thrown when needed.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Just noticed there are other similar problems in SVG code.
If the patch is ok, I'll post a new patch to fix rest of the
wrong kinds of PRInt32/PRUint32 casts.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

checked in

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 256093 [details] [diff] [review]
proposed patch

Why is this nominated only for 1.8.1.x? I get the same assertion and crash in 1.5.0.11

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I think  at some point I thought that 1.5.0.10 or 1.5.0.11 would be the
last 1.5.0.x release.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for branches

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 259271 [details] [diff] [review]
for branches

approved for 1.8.0.12/1.8.1.4, a=dveditz for release-drivers

Comment 12

[juan becerra [:juanb]]

Verified on:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.4pre) Gecko/20070506 BonEcho/2.0.0.4pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.12pre) Gecko/20070508 Firefox/1.5.0.12pre

Testcase no longer crashes Firefox. 

Comment 13

[Jesse Ruderman]

Crashtest checked in.