694953 - (CVE-2011-3654) Crash [@ nsAString_internal::EqualsASCII ] with <svg:mpath> linking to a non-SVG element

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Aki Helin]

Attachment: Input to trigger the crash.

The attached SVG file causes a nasty looking crash at least in Firefox 8.0. I haven't tested with other versions yet. The original file crashed at 0x4, but the address moved while looking for a simpler triggering file and ended up at 0xff000004. High crash addresses often imply security impact, so reporting this as a security bug to be on the safe side.

Crash report: https://crash-stats.mozilla.com/report/index/bp-72d7c90a-2029-4ec7-9a66-1deca2111017

Comment 1

[Aki Helin]

7.0.1 crashed at NS_NewAtom, address 0x1800004 - https://crash-stats.mozilla.com/report/index/bp-15e5d8c1-af04-4f56-bc49-05ac42111017

Comment 2

[Benjamin Smedberg]

Local debug build I'm getting a crash at:

>	xul.dll!nsGenericElement::GetAttributes(aAttributes=0x0fd29d78)  Line 2510	C++

`this` == 0xffffffc8

 	xul.dll!mozilla::SVGMotionSMILAnimationFunction::RebuildPathAndVerticesFromMpathElem(aMpathElem=0x11dea100)  Line 251	C++
 	xul.dll!mozilla::SVGMotionSMILAnimationFunction::RebuildPathAndVertices(aTargetElement=0x0988de30)  Line 309	C++

And the bug is very likely to be in this code:

http://hg.mozilla.org/mozilla-central/annotate/d2241309d83c/content/svg/content/src/nsSVGMpathElement.cpp#l232

which also needs a namespace check

Comment 3

[Benjamin Smedberg]

Attachment: Check node type, not just name, rev. 1

Comment 4

[Daniel Holbert [:dholbert]]

Setting tracking flags, to be sure this makes it to stable branches.  (There's an aurora/beta channel/triage meeting this Thursday -- hopefully we can get approval and land this on branches on that day or the next)

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 567469 [details] [diff] [review]
Check node type, not just name, rev. 1

>-  if (genericTarget &&
>+  if (genericTarget && genericTarget->IsSVG() &&
>       genericTarget->Tag() == nsGkAtoms::path) {

Actually, after thinking a bit more, I'd prefer we explicitly check the namespace here (instead of using IsSVG()).

i.e. just add something like this
  genericTarget->GetNameSpaceID() == kNameSpaceID_SVG &&
either before or after the Tag() check.

IsSVG() includes some stuff that's not in the SVG namespace (I'm actually not entirely sure what, offhand), and it's probably better to be more specific.  It's also more consistent with the related function "GetFirstMpathChild"
  http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGMotionSMILAnimationFunction.cpp#158
which checks the namespace explicitly.

Comment 7

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 567469 [details] [diff] [review] [diff] [details] [review]
> Check node type, not just name, rev. 1
> 
> >-  if (genericTarget &&
> >+  if (genericTarget && genericTarget->IsSVG() &&
> >       genericTarget->Tag() == nsGkAtoms::path) {
> 
> Actually, after thinking a bit more, I'd prefer we explicitly check the
> namespace here (instead of using IsSVG()).

Agreed. Other code elsewhere has namespace checks so we should be consistent here.

Comment 8

[Benjamin Smedberg]

Attachment: Check namespace, not just name, rev. 2

This is the patch without the tests for immediate committal: the test will be embargoed until this is released/made public.

Comment 9

[Benjamin Smedberg]

Attachment: Automated test

Comment 10

[Johnny Stenback (:jst)]

bsmedberg, is this fix ready for review? If so, please request... Also, tracking for 8 and 9, we have a patch and it's a critical bug with a trivial fix, might as well take it.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 567512 [details] [diff] [review]
Check namespace, not just name, rev. 2

It's got implicit r+ (I r+'d the obsolete earlier version, and then requested this tweak).

explicitly marking r+ for clarity.

Comment 12

[Benjamin Smedberg]

FTR, https://hg.mozilla.org/integration/mozilla-inbound/rev/bae188e97e34 landed earlier today ;-)

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 567515 [details] [diff] [review]
Automated test

Just for good measure, r+ on the crashtest, too, though as bsmedberg said in comment 8, it should wait until the fix has made it to releases.

I tested it locally (with directory-targeted "make crashtests") and successfully crashed, in an unpatched build.

Comment 14

[Johnny Stenback (:jst)]

Excellent! Sorry I missed the earlier review on the obsoleted patch. Please request approval for the relevant branches as soon as this is looking good on m-c.

Comment 15

[Daniel Holbert [:dholbert]]

Landed on m-c: https://hg.mozilla.org/mozilla-central/rev/bae188e97e34

Comment 16

[LegNeato]

We'll need a backport for 3.6 as well if this affects that branch.

Comment 17

[LegNeato]

Comment on attachment 567512 [details] [diff] [review]
Check namespace, not just name, rev. 2

---------------------------------[ Triage Comment ]---------------------------------

Approving for 8beta and 9aurora as this is a sg:crit bug filed by an external reporter and the patch looks safe.

Please land ASAP.

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Christian Legnitto [:LegNeato] from comment #16)
> We'll need a backport for 3.6 as well if this affects that branch.

Happily, this doesn't affect that branch.  The feature being exploited here (SVG SMIL, animateMotion support in particular) was new in Firefox 4.

Comment 19

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/532d27c289de
https://hg.mozilla.org/releases/mozilla-beta/rev/2c5518595251

Comment 20

[u279076]

Verified fixed using the attached testcase in Firefox 8.0.1, 9.0b3, and 10.0a2