Last Comment Bug 369249 - Crash [@ nsSVGPathSegList::GetItem] with pathSegList.getItem(-1);
: Crash [@ nsSVGPathSegList::GetItem] with pathSegList.getItem(-1);
Status: RESOLVED FIXED
[sg:critical?]
: crash, testcase, verified1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 326633
  Show dependency treegraph
 
Reported: 2007-02-04 01:59 PST by Jesse Ruderman
Modified: 2007-12-13 00:12 PST (History)
4 users (show)
dveditz: blocking1.8.1.4+
dveditz: blocking1.8.0.12+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes firefox) (354 bytes, image/svg+xml)
2007-02-04 01:59 PST, Jesse Ruderman
no flags Details
proposed patch (2.62 KB, patch)
2007-02-22 06:29 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
proposed patch (8.56 KB, patch)
2007-02-22 15:06 PST, Olli Pettay [:smaug]
tor: review+
tor: superreview+
Details | Diff | Splinter Review
for branches (7.45 KB, patch)
2007-03-21 16:59 PDT, Olli Pettay [:smaug]
dveditz: approval1.8.1.4+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-02-04 01:59:13 PST
Created attachment 253933 [details]
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 [:bz] 2007-02-04 02:57:36 PST
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] 2007-02-04 11:15:16 PST
Who is minding the SVG store?

/be
Comment 3 Jesse Ruderman 2007-02-14 15:57:07 PST
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] 2007-02-22 06:29:14 PST
Created attachment 256029 [details] [diff] [review]
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] 2007-02-22 14:55:22 PST
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] 2007-02-22 15:06:41 PST
Created attachment 256093 [details] [diff] [review]
proposed patch
Comment 7 Olli Pettay [:smaug] 2007-02-22 15:29:09 PST
checked in
Comment 8 Daniel Veditz [:dveditz] 2007-03-21 11:16:44 PDT
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] 2007-03-21 16:23:50 PDT
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] 2007-03-21 16:59:49 PDT
Created attachment 259271 [details] [diff] [review]
for branches
Comment 11 Daniel Veditz [:dveditz] 2007-03-27 15:32:19 PDT
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] 2007-05-08 16:22:26 PDT
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 2007-12-13 00:12:22 PST
Crashtest checked in.

Note You need to log in before you can comment on or make changes to this bug.