Closed Bug 572986 (CVE-2010-1208) Opened 10 years ago Closed 9 years ago

DOM Attribute Cloning Remote Code Execution Vulnerability (ZDI-CAN-832)


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

Not set



Tracking Status
blocking2.0 --- final+
status2.0 --- ?
blocking1.9.2 --- .7+
status1.9.2 --- .7-fixed
blocking1.9.1 --- .11+
status1.9.1 --- .11-fixed


(Reporter: reed, Assigned: smaug)


(Keywords: fixed1.9.0.20, verified1.9.1, verified1.9.2, Whiteboard: [sg:critical?])


(3 files)

Attached file PoC
ZDI-CAN-832: Mozilla Firefox DOM Attribute Cloning Remote Code Execution Vulnerability

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

TippingPoint has identified a vulnerability affecting the following 

    Mozilla Firefox 3.6.x

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

This vulnerability allows remote attackers to execute arbitrary code on
vulnerable installations of Mozilla Firefox. User interaction is
required to exploit this vulnerability in that the target must visit a
malicious page or open a malicious file.

The specific flaw exists due to a workaround that was implemented in
order to support recursive cloning of attribute nodes. If an event is
added to the first attribute node, the application can be made to free
the node, and then later access a reference to it. This can lead to code
execution under the  context of the application.

The issue occurs due to a workaround in the application using the first
child when cloning an attribute node and inserting children. Due to the
side-effect of a attribute's children not actually being inserted due to
bug id 56758 [], an
event can be made to remove each attribute. This will then free a
reference to an attribute node, which can then be replaced with another
type of the same size. The application will later use this freed type.

nsNodeUtils::CloneAndAdopt(nsINode *aNode, PRBool aClone, PRBool aDeep,
                           nsNodeInfoManager *aNewNodeInfoManager,
                           JSContext *aCx, JSObject *aOldScope,
                           JSObject *aNewScope,
                           nsCOMArray<nsINode> &aNodesWithProperties,
                           nsINode *aParent, nsIDOMNode **aResult)

  // The DOM spec says to always adopt/clone/import the children of
  // nodes.
  // XXX The following block is here because our implementation of
  //     nodes is broken when it comes to inserting children. Instead of
  //     their children we force creation of the only child by calling
  //     GetChildAt(0). We can remove this when
  // is fixed.

  if (aClone && aNode->IsNodeOfType(nsINode::eATTRIBUTE)) {
    nsCOMPtr<nsINode> attrChildNode = aNode->GetChildAt(0);
    // We only need to do this if the child node has properties (because
    // might need to call a userdata handler).
    if (attrChildNode && attrChildNode->HasProperties()) {
      nsCOMPtr<nsINode> clonedAttrChildNode = clone->GetChildAt(0);
      if (clonedAttrChildNode) {
        PRBool ok = aNodesWithProperties.AppendObject(attrChildNode) &&

Version(s)  tested: Mozilla Firefox 3.6.3
Platform(s) tested: Windows XP SP3

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

This vulnerability was discovered by:
    * regenrecht
I can't get this to crash on trunk... Will try 3.6.x later unless somebody beats me to it.

Error: node is null
Source File: file:///.../poc.html
Line: 24

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a6pre) Gecko/20100617 Minefield/3.7a6pre
IIRC, either boris or jonas changed attribute handling a bit.
But I'm investigating this on 1.9.2.
I think I know where the problem is.
Assignee: nobody → Olli.Pettay
Jonas, nsDOMAttribute::EnsureChildState is your code.
I don't really understand why it updates the value in some cases, but not
always. And does it really have to notify.
If it wouldn't notify, we wouldn't fire the extra mutation event which
causes this bug.
Btw, I can't reproduce the crash on 1.9.2 debug build, but I do see lots of
evil assertions and I can see the extra mutation event.
Attached patch very safe patchSplinter Review
This should be very safe patch, but I don't like this.
Attached patch patchSplinter Review
Usually when attribute is in element, textnode's value gets updated via
mutation observer. 
Setting .value should create a Text node, so in practice we can
just remove the old. The new one will be created lazily.

I'll write some tests for this.
Attachment #452232 - Flags: review?(jonas)
Ah, on trunk SetText isn't called with notify=true.
This was fixed on trunk in  Bug 429175.
Should we actually land that to branches too.
Attachment #452232 - Flags: review?(jonas)
(In reply to comment #9)
> This was fixed on trunk in  Bug 429175.
> Should we actually land that to branches too.

Hum, I had convinced myself that the callsites that were fixed there weren't actually causing vulnerabilities, but obviously I was wrong.

I don't think we should port that bug still though as lots of callsites were for sure not vulnerable and just false positives.
Comment on attachment 452232 [details] [diff] [review]

Why do you need the added RemoveChildAt call?
blocking1.9.2: ? → needed
I assume this is also a problem on 1.9.1?

If this is the right patch can we get this in in time for (code freeze probably end of next week?)
(In reply to comment #11)
> (From update of attachment 452232 [details] [diff] [review])
> Why do you need the added RemoveChildAt call?

I was sanitizing value handling (actually make it work more like what the spec
and other browsers do), but then I realized that it is not needed for this
bug. That is why I cancelled the review.
I'll fix that in a separate non-sg* bug.
I would say we should just change the SetText call to have aNotify=false and mark this bug fixed. All this stuff is significantly different on trunk anyway.
> I would say we should just change the SetText call to have aNotify=false

Is there any danger of that causing ranges to get out of sync?
blocking1.9.1: ? → .11+
blocking1.9.2: needed → .6+
can you update the status here? or request view?
I will need to investigate if the simple fix would cause problems (event security problems) with ranges. I'll try to do that tomorrow.
Attached patch patchSplinter Review
Since nsIRange has strong refs to mRoot and mStartParent and mEndParent, I
can't really see how this could cause bad problems with ranges.
Few things can go wrong, but not because of this patch, but because
we don't handle attribute nodes the same way as other nodes.
Attachment #453323 - Flags: review?(jonas)
Attachment #453323 - Flags: approval1.9.2.6?
Attachment #453323 - Flags: approval1.9.1.11?
Comment on attachment 453323 [details] [diff] [review]

a=LegNeato for and Please land this on mozilla-1.9.2 default and mozilla-1.9.1 default.

Code freeze is this Friday @ 11:59 pm PST.
Attachment #453323 - Flags: approval1.9.2.6?
Attachment #453323 - Flags: approval1.9.2.6+
Attachment #453323 - Flags: approval1.9.1.11?
Attachment #453323 - Flags: approval1.9.1.11+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2
Alias: CVE-2010-1208
Verified fixed in with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100713 Firefox/3.6.7 (.NET CLR 3.5.30729) using PoC. PoC crashes

Verified fixed for with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100701 Firefox/3.5.11 (.NET CLR 3.5.30729) as well. Crashes
Comment on attachment 453323 [details] [diff] [review]

Requesting on this patch so that we can take it in upcoming Camino 2.0.x security and stability updates.  If approved, I'll handle the checkins, unless the patch author requests otherwise.
Attachment #453323 - Flags:
Comment on attachment 453323 [details] [diff] [review]

Approved for, a=dveditz
Attachment #453323 - Flags: →
Checking in content/base/src/nsDOMAttribute.cpp;
/cvsroot/mozilla/content/base/src/nsDOMAttribute.cpp,v  <--  nsDOMAttribute.cpp
new revision: 1.112; previous revision: 1.111
Keywords: fixed1.9.0.20
Group: core-security
blocking2.0: ? → final+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.