Closed
Bug 369249
Opened 17 years ago
Closed 17 years ago
Crash [@ nsSVGPathSegList::GetItem] with pathSegList.getItem(-1);
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: smaug)
Details
(4 keywords, Whiteboard: [sg:critical?])
Crash Data
Attachments
(3 files, 1 obsolete file)
354 bytes,
image/svg+xml
|
Details | |
8.56 KB,
patch
|
tor
:
review+
tor
:
superreview+
|
Details | Diff | Splinter Review |
7.45 KB,
patch
|
dveditz
:
approval1.8.1.4+
dveditz
:
approval1.8.0.12+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
It's an nsCOMArray, not a raw nsVoidArray. And this code should just be using SafeObjectAt() instead of rolling their own.
Flags: blocking1.9?
Comment 2•17 years ago
|
||
Who is minding the SVG store? /be
Reporter | ||
Updated•17 years ago
|
Whiteboard: [sg:moderate?]
Reporter | ||
Comment 3•17 years ago
|
||
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.
Whiteboard: [sg:moderate?] → [sg:critical?]
Assignee | ||
Comment 4•17 years ago
|
||
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.
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #256029 -
Flags: superreview?(tor)
Attachment #256029 -
Flags: review?(tor)
Assignee | ||
Comment 5•17 years ago
|
||
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.
Assignee | ||
Comment 6•17 years ago
|
||
Attachment #256029 -
Attachment is obsolete: true
Attachment #256093 -
Flags: superreview?(tor)
Attachment #256093 -
Flags: review?(tor)
Attachment #256029 -
Flags: superreview?(tor)
Attachment #256029 -
Flags: review?(tor)
Attachment #256093 -
Flags: superreview?(tor)
Attachment #256093 -
Flags: superreview+
Attachment #256093 -
Flags: review?(tor)
Attachment #256093 -
Flags: review+
Assignee | ||
Comment 7•17 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: blocking1.9? → blocking1.8.1.3?
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Attachment #256093 -
Flags: approval1.8.1.3?
Updated•17 years ago
|
Flags: blocking1.8.1.4? → blocking1.8.1.4+
Comment 8•16 years ago
|
||
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
Updated•16 years ago
|
Flags: blocking1.8.0.12?
Whiteboard: [sg:critical?] → [sg:critical?] what about 1.8.0 branch?
Updated•16 years ago
|
Flags: blocking1.8.0.12? → blocking1.8.0.12+
Assignee | ||
Comment 9•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #256093 -
Flags: approval1.8.1.4?
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #259271 -
Flags: approval1.8.1.4?
Attachment #259271 -
Flags: approval1.8.0.12?
Comment 11•16 years ago
|
||
Comment on attachment 259271 [details] [diff] [review] for branches approved for 1.8.0.12/1.8.1.4, a=dveditz for release-drivers
Attachment #259271 -
Flags: approval1.8.1.4?
Attachment #259271 -
Flags: approval1.8.1.4+
Attachment #259271 -
Flags: approval1.8.0.12?
Attachment #259271 -
Flags: approval1.8.0.12+
Assignee | ||
Updated•16 years ago
|
Keywords: fixed1.8.0.12,
fixed1.8.1.4
Whiteboard: [sg:critical?] what about 1.8.0 branch? → [sg:critical?]
Comment 12•16 years ago
|
||
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.
Updated•16 years ago
|
Group: security
Updated•12 years ago
|
Crash Signature: [@ nsSVGPathSegList::GetItem]
You need to log in
before you can comment on or make changes to this bug.
Description
•