Closed Bug 360021 (zdi-can-126) Opened 18 years ago Closed 18 years ago

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


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

1.8 Branch
Not set





(Reporter: dveditz, Assigned: sicking)



(4 keywords, Whiteboard: [sg:critical] [at risk] needs patch)


(2 files, 1 obsolete file)

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

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

3Com has identified a vulnerability affecting the following products:

Mozilla Firefox
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'

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

This vulnerability was discovered by:


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

The Zero Day Initiative (ZDI) represents a best-of-breed model for
rewarding security researchers for responsibly disclosing discovered
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9+
Keywords: crash
Alias: zdi-can-126
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.
Should have mentioned that -- I've asked for the PoC, hopefully will get it tomorrow.
Whiteboard: [sg:needinfo]
Summary: ZDI-CAN-126: Firefox SVG Processing Remote Code Execution → Exploitable crash when appending SVG comment to element in a non-SVG document (ZDI-CAN-126)
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.
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

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

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.
Assignee: general → bugmail
Component: SVG → DOM
Whiteboard: [sg:needinfo] → [sg:critical]
Jonas, any progress?
Whiteboard: [sg:critical] → [sg:critical] [at risk] needs patch
Attached patch Patch to fix (obsolete) — Splinter Review
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.
Attachment #247100 - Flags: superreview?(bzbarsky)
Attachment #247100 - Flags: review?(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 


>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.
Attachment #247100 - Flags: superreview?(bzbarsky)
Attachment #247100 - Flags: superreview+
Attachment #247100 - Flags: review?(bzbarsky)
Attachment #247100 - Flags: review+
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)
Attachment #247100 - Attachment is obsolete: true
Attachment #247118 - Flags: superreview+
Attachment #247118 - Flags: review+
Attachment #247118 - Flags: approval1.8.1.1?
Attachment #247118 - Flags: approval1.8.0.9?
Comment on attachment 247118 [details] [diff] [review]
With review comments

Approved for both branches, a=jay for drivers.
Attachment #247118 - Flags: approval1.8.1.1?
Attachment #247118 - Flags: approval1.8.1.1+
Attachment #247118 - Flags: approval1.8.0.9?
Attachment #247118 - Flags: approval1.8.0.9+
Checked in on both branches
Closed: 18 years ago
Resolution: --- → FIXED
We should probably add tests for this to mochitest or something.
Flags: in-testsuite?
verified fixed,, 1.9 windows/linux/macppc
Keywords: regression
Group: security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.