572986 - (CVE-2010-1208) DOM Attribute Cloning Remote Code Execution Vulnerability (ZDI-CAN-832)

Status: RESOLVED FIXED

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

Description

[Reed Loden [:reed]]

Attachment: 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

Comment 1

[Reed Loden [:reed]]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

IIRC, either boris or jonas changed attribute handling a bit.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

But I'm investigating this on 1.9.2.
I think I know where the problem is.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: very safe patch

This should be very safe patch, but I don't like this.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Ah, on trunk SetText isn't called with notify=true.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

This was fixed on trunk in  Bug 429175.
Should we actually land that to branches too.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 452232 [details] [diff] [review]
patch

Why do you need the added RemoveChildAt call?

Comment 12

[Daniel Veditz [:dveditz]]

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?)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 16

[Robert Sayre]

can you update the status here? or request view?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I will need to investigate if the simple fix would cause problems (event security problems) with ranges. I'll try to do that tomorrow.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

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.

Comment 19

[LegNeato]

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.

Comment 20

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cad7ffc0b2b3
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aaa9c80a559c

Comment 21

[Al Billings [:abillings - ex-MoCo]]

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 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 453323 [details] [diff] [review]
patch

Approved for 1.9.0.20, a=dveditz

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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