Closed Bug 369249 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: SVG, defect)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: smaug)

Details

(4 keywords, Whiteboard: [sg:critical?])

Crash Data

Attachments

(3 files, 1 obsolete file)

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).
It's an nsCOMArray, not a raw nsVoidArray.  And this code should just be using SafeObjectAt() instead of rolling their own.
Flags: blocking1.9?
Who is minding the SVG store?

/be
Whiteboard: [sg:moderate?]
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?]
Attached patch proposed patch (obsolete) — Splinter Review
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)
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.
Attached patch proposed patchSplinter Review
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+
checked in
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: blocking1.9? → blocking1.8.1.3?
Resolution: --- → FIXED
Attachment #256093 - Flags: approval1.8.1.3?
Flags: blocking1.8.1.4? → blocking1.8.1.4+
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
Flags: blocking1.8.0.12?
Whiteboard: [sg:critical?] → [sg:critical?] what about 1.8.0 branch?
Flags: blocking1.8.0.12? → blocking1.8.0.12+
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.
Attachment #256093 - Flags: approval1.8.1.4?
Attached patch for branchesSplinter Review
Attachment #259271 - Flags: approval1.8.1.4?
Attachment #259271 - Flags: approval1.8.0.12?
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+
Whiteboard: [sg:critical?] what about 1.8.0 branch? → [sg:critical?]
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. 
Group: security
Crashtest checked in.
Flags: in-testsuite+
Crash Signature: [@ nsSVGPathSegList::GetItem]
You need to log in before you can comment on or make changes to this bug.