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

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

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.2
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

People

(Reporter: reed, Assigned: smaug)

Details

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

Attachments

(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 
products:

    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 [https://bugzilla.mozilla.org/show_bug.cgi?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.

content\base\src\nsNodeUtils.cpp:517
nsresult
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
attribute
  // nodes.
  // XXX The following block is here because our implementation of
attribute
  //     nodes is broken when it comes to inserting children. Instead of
cloning
  //     their children we force creation of the only child by calling
  //     GetChildAt(0). We can remove this when
  //     https://bugzilla.mozilla.org/show_bug.cgi?id=56758 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
we
    // might need to call a userdata handler).
    if (attrChildNode && attrChildNode->HasProperties()) {
      nsCOMPtr<nsINode> clonedAttrChildNode = clone->GetChildAt(0);
      if (clonedAttrChildNode) {
        PRBool ok = aNodesWithProperties.AppendObject(attrChildNode) &&
                   
aNodesWithProperties.AppendObject(clonedAttrChildNode);
        NS_ENSURE_TRUE(ok, NS_ERROR_OUT_OF_MEMORY);
      }
    }
  }

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]
patch

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 1.9.2.6 (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]
patch

a=LegNeato for 1.9.2.6 and 1.9.1.11. 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+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cad7ffc0b2b3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aaa9c80a559c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2
Alias: CVE-2010-1208
Verified fixed in 1.9.2.7 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7) Gecko/20100713 Firefox/3.6.7 (.NET CLR 3.5.30729) using PoC. PoC crashes 1.9.2.6.

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

Requesting approval1.9.0.next 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: approval1.9.0.next?
Comment on attachment 453323 [details] [diff] [review]
patch

Approved for 1.9.0.20, a=dveditz
Attachment #453323 - Flags: approval1.9.0.next? → approval1.9.0.next+
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
done
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.