Last Comment Bug 694953 - (CVE-2011-3654) Crash [@ nsAString_internal::EqualsASCII ] with <svg:mpath> linking to a non-SVG element
(CVE-2011-3654)
: Crash [@ nsAString_internal::EqualsASCII ] with <svg:mpath> linking to a non-...
Status: VERIFIED FIXED
[sg:critical][qa!]
: testcase, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Benjamin Smedberg [:bsmedberg]
:
Mentors:
Depends on:
Blocks: animateMotion
  Show dependency treegraph
 
Reported: 2011-10-17 05:15 PDT by Aki Helin
Modified: 2014-06-26 13:41 PDT (History)
8 users (show)
rforbes: sec‑bounty+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed
+
fixed
+
fixed
unaffected
unaffected


Attachments
Input to trigger the crash. (393 bytes, image/svg+xml)
2011-10-17 05:15 PDT, Aki Helin
no flags Details
Check node type, not just name, rev. 1 (2.02 KB, patch)
2011-10-17 09:06 PDT, Benjamin Smedberg [:bsmedberg]
dholbert: review+
Details | Diff | Splinter Review
Check namespace, not just name, rev. 2 (1.02 KB, patch)
2011-10-17 11:51 PDT, Benjamin Smedberg [:bsmedberg]
dholbert: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Automated test (1.05 KB, patch)
2011-10-17 11:55 PDT, Benjamin Smedberg [:bsmedberg]
dholbert: review+
Details | Diff | Splinter Review

Description Aki Helin 2011-10-17 05:15:26 PDT
Created attachment 567424 [details]
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 2011-10-17 05:36:35 PDT
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 [:bsmedberg] 2011-10-17 08:24:51 PDT
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 [:bsmedberg] 2011-10-17 09:06:36 PDT
Created attachment 567469 [details] [diff] [review]
Check node type, not just name, rev. 1
Comment 4 Daniel Holbert [:dholbert] 2011-10-17 10:03:42 PDT
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] 2011-10-17 10:26:31 PDT
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 2011-10-17 10:42:25 PDT
(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 [:bsmedberg] 2011-10-17 11:51:32 PDT
Created attachment 567512 [details] [diff] [review]
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 [:bsmedberg] 2011-10-17 11:55:20 PDT
Created attachment 567515 [details] [diff] [review]
Automated test
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2011-10-20 13:11:35 PDT
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] 2011-10-20 13:16:47 PDT
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 [:bsmedberg] 2011-10-20 13:22:44 PDT
FTR, https://hg.mozilla.org/integration/mozilla-inbound/rev/bae188e97e34 landed earlier today ;-)
Comment 13 Daniel Holbert [:dholbert] 2011-10-20 13:25:05 PDT
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, jst@mozilla.com) 2011-10-20 16:34:25 PDT
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] 2011-10-21 03:21:17 PDT
Landed on m-c: https://hg.mozilla.org/mozilla-central/rev/bae188e97e34
Comment 16 christian 2011-10-25 20:13:44 PDT
We'll need a backport for 3.6 as well if this affects that branch.
Comment 17 christian 2011-10-25 20:14:33 PDT
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] 2011-10-25 23:09:14 PDT
(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 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-11-24 13:36:13 PST
Verified fixed using the attached testcase in Firefox 8.0.1, 9.0b3, and 10.0a2

Note You need to log in before you can comment on or make changes to this bug.