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.
Who is minding the SVG store?
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.
Created attachment 256029 [details] [diff] [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.
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.
Created attachment 256093 [details] [diff] [review]
Comment on attachment 256093 [details] [diff] [review]
Why is this nominated only for 1.8.1.x? I get the same assertion and crash in 188.8.131.52
I think at some point I thought that 184.108.40.206 or 220.127.116.11 would be the
last 1.5.0.x release.
Created attachment 259271 [details] [diff] [review]
Comment on attachment 259271 [details] [diff] [review]
approved for 18.104.22.168/22.214.171.124, a=dveditz for release-drivers
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:126.96.36.199pre) Gecko/20070506 BonEcho/188.8.131.52pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:184.108.40.206pre) Gecko/20070508 Firefox/220.127.116.11pre
Testcase no longer crashes Firefox.
Crashtest checked in.