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

RESOLVED FIXED

Status

()

Core
SVG
--
critical
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
Mac OS X
crash, testcase, verified1.8.0.12, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.4 +
blocking1.8.0.12 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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).
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
(Reporter)

Updated

11 years ago
Whiteboard: [sg:moderate?]
(Reporter)

Comment 3

10 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

10 years ago
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.
Assignee: general → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #256029 - Flags: superreview?(tor)
Attachment #256029 - Flags: review?(tor)
(Assignee)

Comment 5

10 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

10 years ago
Created attachment 256093 [details] [diff] [review]
proposed patch
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)

Updated

10 years ago
Attachment #256093 - Flags: superreview?(tor)
Attachment #256093 - Flags: superreview+
Attachment #256093 - Flags: review?(tor)
Attachment #256093 - Flags: review+
(Assignee)

Comment 7

10 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.9? → blocking1.8.1.3?
Resolution: --- → FIXED
(Assignee)

Updated

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

Comment 9

10 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

10 years ago
Attachment #256093 - Flags: approval1.8.1.4?
(Assignee)

Comment 10

10 years ago
Created attachment 259271 [details] [diff] [review]
for branches
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+
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.0.12, fixed1.8.1.4
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. 
Keywords: fixed1.8.0.12, fixed1.8.1.4 → verified1.8.0.12, verified1.8.1.4
Group: security
(Reporter)

Comment 13

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