Bug 694953 (CVE-2011-3654)

Crash [@ nsAString_internal::EqualsASCII ] with <svg:mpath> linking to a non-SVG element

VERIFIED FIXED

Status

()

Core
SVG
--
critical
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Aki Helin, Assigned: bsmedberg)

Tracking

({testcase, verified-aurora, verified-beta})

Trunk
testcase, verified-aurora, verified-beta
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox7+ wontfix, firefox8+ fixed, firefox9+ fixed, firefox10+ fixed, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical][qa!])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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
Component: General → SVG
Product: Firefox → Core
QA Contact: general → general
Keywords: testcase
(Reporter)

Comment 1

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

Comment 2

6 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

6 years ago
Assignee: nobody → benjamin
Summary: Crash [@ nsAString_internal::EqualsASCII ] → Crash [@ nsAString_internal::EqualsASCII ] with <svg:mpath> linking to a non-SVG element
(Assignee)

Comment 3

6 years ago
Created attachment 567469 [details] [diff] [review]
Check node type, not just name, rev. 1
Attachment #567469 - Flags: review?(dholbert)
Attachment #567469 - Flags: review?(dholbert) → review+
Whiteboard: [sg:critical]
Version: 8 Branch → Trunk
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)
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
Version: Trunk → 8 Branch
Version: 8 Branch → Trunk
Blocks: 436418
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

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 567515 [details] [diff] [review]
Automated test
Attachment #567469 - Attachment is obsolete: true
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.
status-firefox10: --- → affected
status-firefox7: --- → wontfix
status-firefox8: --- → affected
status-firefox9: --- → affected
tracking-firefox10: --- → +
tracking-firefox7: --- → +
tracking-firefox8: ? → +
tracking-firefox9: ? → +
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.
Attachment #567512 - Flags: review+
(Assignee)

Comment 12

6 years ago
FTR, https://hg.mozilla.org/integration/mozilla-inbound/rev/bae188e97e34 landed earlier today ;-)
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.
Attachment #567515 - Attachment description: Automated tests → Automated test
Attachment #567515 - Flags: review+
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.
Landed on m-c: https://hg.mozilla.org/mozilla-central/rev/bae188e97e34
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Attachment #567512 - Flags: approval-mozilla-beta?
Attachment #567512 - Flags: approval-mozilla-aurora?

Comment 16

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

Comment 17

6 years ago
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.
Attachment #567512 - Flags: approval-mozilla-beta?
Attachment #567512 - Flags: approval-mozilla-beta+
Attachment #567512 - Flags: approval-mozilla-aurora?
Attachment #567512 - Flags: approval-mozilla-aurora+
(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.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
OS: Linux → All
Hardware: x86 → All
https://hg.mozilla.org/releases/mozilla-aurora/rev/532d27c289de
https://hg.mozilla.org/releases/mozilla-beta/rev/2c5518595251
status-firefox10: affected → fixed
status-firefox8: affected → fixed
status-firefox9: affected → fixed
Alias: CVE-2011-3654
Flags: in-testsuite?
Whiteboard: [sg:critical] → [sg:critical][qa+]
Verified fixed using the attached testcase in Firefox 8.0.1, 9.0b3, and 10.0a2
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.