360021 - (zdi-can-126) Exploitable crash when appending SVG comment to element in a non-SVG document (ZDI-CAN-126)

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Veditz [:dveditz]]

ZDI-CAN-126: Mozilla Firefox SVG Processing Remote Code Execution

-- ABSTRACT ------------------------------------------------------------

3Com has identified a vulnerability affecting the following products:

Mozilla Firefox 1.5.0.7
Mozilla Firefox 2.0

-- VULNERABILITY DETAILS -----------------------------------------------

This vulnerability allows remote attackers to execute arbitrary code on
systems with affected installations of Mozilla Firefox. Exploitation
requires that the target user visit a malicious web page.

The flaw exists in the browser's handling of SVG comment objects.
Firefox does not correctly handle requests to append SVG comments to
elements in other types of documents. Attempting such an operation
results in memory corruption that can be trivially exploited to execute
arbitrary code.

Memory corruption can be produced by attempting to append an SVG
comment node from a document contained in a child frame to an HTML
element in the parent document with that element's 'appendChild'
method.

-- CREDIT --------------------------------------------------------------

This vulnerability was discovered by:

    Anonymous

-- INFORMATION ABOUT THE ZDI -------------------------------------------

The Zero Day Initiative (ZDI) represents a best-of-breed model for
rewarding security researchers for responsibly disclosing discovered
vulnerabilities.

Comment 1

[tor]

Do they have a testcase which demonstrates the problem?  One that I put together seems to move elements, including a comment object, from the <object> embedded svg to the parent HTML doc without incident.

Comment 2

[Daniel Veditz [:dveditz]]

Should have mentioned that -- I've asked for the PoC, hopefully will get it tomorrow.

Comment 3

[Daniel Veditz [:dveditz]]

Attachment: testcase (modified from ZDI PoC)

ZDI sent me their proof-of-concept testcases, this is a variant on the simplest one (my changes were to add a button so it didn't crash on open, and to incorporate the SVG file into a data URL to make it easier to attach).

There's a more complex version that launches calc.exe on windows, but I don't think it adds anything to help us fix the vuln.

Comment 4

[Daniel Veditz [:dveditz]]

Although involved in the testcase, I think SVG may be off the hook. This crash started on the 1.8.0 branch between 2006-04-21-04 and 2006-04-22-07 builds

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_0_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-21&maxdate=2006-04-22-06&cvsroot=%2Fcvsroot

On the 1.8 branch (FF2) it regressed between 2006-04-20-05 and 2006-04-21-03 builds

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=AviarySuiteBranchTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-04-20&maxdate=2006-04-21-03&cvsroot=%2Fcvsroot

TB25858699 (nsExternalAppHandler::OnStopRequest)
TB25860155 (nsAttrAndChildArray::RemoveChildAt)
TB25870222 (nsAttrAndChildArray::RemoveChildAt)
TB25870497 (js_LookupPropertyWithFlags)
TB25871144 (nsAttrAndChildArray::RemoveChildAt)

The only code in common is dbaron's bug 302536 in nsImageFrame (unlikely)
and from sicking bug 324918 and bug 325426.

Before I looked at the FF2 branch (which ruled them out) I was thinking it was more likely the mutation-related bug 329982 and bugs 325730, 330084, and 330925 checkins.

The testcase never crashed on the trunk, even around the time bug 324918 and some of these others landed.

Comment 5

[Daniel Veditz [:dveditz]]

Jonas, any progress?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This was a bad one. When implicitly removing a child from a document (i.e. by inserting the child elsewhere) our code is totally horked. The child is removed from the wrong child list, meaning that all kinds of nasty things happen both in the document (which will have an unexpected child left) and the node whos childlist we actually used (which will have one child too few).

Though the patch is big-ish the fix is very simple. Most of the patch is just exposing new methods that already exist. The actual code change was to move a function call from nsContentOrDocument::RemoveChildAt to nsDocument::RemoveChildAt so that the right child-list can be grabbed.

The reason this doesn't affect trunk is that all this code is vastly different there and much cleaner and safer.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 247100 [details] [diff] [review]
Patch to fix

>Index: content/base/public/nsIDocument.h
> #define NS_IDOCUMENT_MOZILLA_1_8_0_BRANCH_IID      \

I think it might be worth it to just create nsIDocument_MOZILLA_1_8_0_BRANCH2 here...  Since we're QIing at the one callsite anyway, I would prefer that to changing this API, even though I suspect you're right that no one is using it.  It doesn't really cost much to add the other interface, and is guaranteed safe.

>+   * Remove a child from this content node.

s/content node/document/

>+   * @param aNotify whether to notify the document that the replace 

s/replace/remove/

>Index: content/base/src/nsDocument.cpp
>+nsDocument::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)

>+  if (kid) {

And if not, you need to return NS_OK.

r+sr=bzbarsky with those nits.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: With review comments

This addresses review comments. I talked bz out of having to add nsIDocument_MOZILLA_1_8_BRANCH2. First of all it's extremely unlikely that someone's using the interface. Second, I don't want to have to deal with the long-term consequences of doing that forever.

This patch should be safe. And the value is very high given how broken things are (not just from a security point of view, but also because some things actually just don't work currently)

Comment 9

[Jay Patel [:jay]]

Comment on attachment 247118 [details] [diff] [review]
With review comments

Approved for both branches, a=jay for drivers.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in on both branches

Comment 11

[Boris Zbarsky [:bzbarsky]]

We should probably add tests for this to mochitest or something.

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.9, 1.8.1.1, 1.9 windows/linux/macppc