Bug 360021 (zdi-can-126)

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




12 years ago
12 years ago


(Reporter: dveditz, Assigned: sicking)


(4 keywords)

1.8 Branch
crash, regression, verified1.8.0.9, verified1.8.1.1
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)


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


(2 attachments, 1 obsolete attachment)



12 years ago
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


12 years ago
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9+
Keywords: crash


12 years ago
Alias: zdi-can-126

Comment 1

12 years ago
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

12 years ago
Should have mentioned that -- I've asked for the PoC, hopefully will get it tomorrow.
Whiteboard: [sg:needinfo]


12 years ago
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)

Comment 3

12 years ago
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

12 years ago
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


12 years ago
Whiteboard: [sg:needinfo] → [sg:critical]

Comment 5

12 years ago
Jonas, any progress?


12 years ago
Whiteboard: [sg:critical] → [sg:critical] [at risk] needs patch
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.
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+
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)
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 9

12 years ago
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
Last Resolved: 12 years ago
Keywords: fixed1.8.0.9, fixed1.8.1.1
Resolution: --- → FIXED
We should probably add tests for this to mochitest or something.
Flags: in-testsuite?

Comment 12

12 years ago
verified fixed,, 1.9 windows/linux/macppc
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1


12 years ago
Keywords: regression


12 years ago
Group: security
You need to log in before you can comment on or make changes to this bug.