Last Comment Bug 360021 - (zdi-can-126) Exploitable crash when appending SVG comment to element in a non-SVG document (ZDI-CAN-126)
: Exploitable crash when appending SVG comment to element in a non-SVG document...
[sg:critical] [at risk] needs patch
: crash, regression, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 1.8 Branch
: All All
-- critical (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Depends on:
Blocks: 325730
  Show dependency treegraph
Reported: 2006-11-08 15:19 PST by Daniel Veditz [:dveditz]
Modified: 2006-12-22 10:49 PST (History)
7 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (modified from ZDI PoC) (607 bytes, text/html)
2006-11-11 22:14 PST, Daniel Veditz [:dveditz]
no flags Details
Patch to fix (8.53 KB, patch)
2006-11-30 14:08 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
With review comments (8.54 KB, patch)
2006-11-30 16:40 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
jonas: review+
jonas: superreview+
jaymoz: approval1.8.0.9+
jaymoz: approval1.8.1.1+
Details | Diff | Splinter Review

Description User image Daniel Veditz [:dveditz] 2006-11-08 15:19:29 PST
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
Comment 1 User image tor 2006-11-08 16:14:36 PST
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 User image Daniel Veditz [:dveditz] 2006-11-08 18:54:20 PST
Should have mentioned that -- I've asked for the PoC, hopefully will get it tomorrow.
Comment 3 User image Daniel Veditz [:dveditz] 2006-11-11 22:14:02 PST
Created attachment 245375 [details]
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 User image Daniel Veditz [:dveditz] 2006-11-11 23:01:53 PST
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.
Comment 5 User image Daniel Veditz [:dveditz] 2006-11-27 11:07:51 PST
Jonas, any progress?
Comment 6 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-30 14:08:33 PST
Created attachment 247100 [details] [diff] [review]
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 User image Boris Zbarsky [:bz] (still a bit busy) 2006-11-30 16:32:35 PST
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.
Comment 8 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-30 16:40:40 PST
Created attachment 247118 [details] [diff] [review]
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 User image Jay Patel [:jay] 2006-11-30 16:44:04 PST
Comment on attachment 247118 [details] [diff] [review]
With review comments

Approved for both branches, a=jay for drivers.
Comment 10 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-30 16:50:19 PST
Checked in on both branches
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2006-11-30 19:19:01 PST
We should probably add tests for this to mochitest or something.
Comment 12 User image Bob Clary [:bc:] 2006-12-06 15:02:59 PST
verified fixed,, 1.9 windows/linux/macppc

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