Last Comment Bug 572986 - (CVE-2010-1208) DOM Attribute Cloning Remote Code Execution Vulnerability (ZDI-CAN-832)
(CVE-2010-1208)
: DOM Attribute Cloning Remote Code Execution Vulnerability (ZDI-CAN-832)
Status: RESOLVED FIXED
[sg:critical?]
: fixed1.9.0.20, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla1.9.2
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-18 00:33 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2010-09-15 16:09 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
?
.7+
.7-fixed
.11+
.11-fixed


Attachments
very safe patch (677 bytes, patch)
2010-06-18 03:05 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (1.06 KB, patch)
2010-06-18 03:39 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (556 bytes, patch)
2010-06-23 01:57 PDT, Olli Pettay [:smaug]
jonas: review+
christian: approval1.9.2.7+
christian: approval1.9.1.11+
dveditz: approval1.9.0.next+
Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2010-06-18 00:33:50 PDT
Created attachment 452203 [details]
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] (use needinfo?) 2010-06-18 00:53:45 PDT
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] 2010-06-18 00:57:51 PDT
IIRC, either boris or jonas changed attribute handling a bit.
Comment 3 Olli Pettay [:smaug] 2010-06-18 01:03:20 PDT
But I'm investigating this on 1.9.2.
I think I know where the problem is.
Comment 4 Olli Pettay [:smaug] 2010-06-18 02:26:19 PDT
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] 2010-06-18 02:27:19 PDT
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] 2010-06-18 03:05:01 PDT
Created attachment 452225 [details] [diff] [review]
very safe patch

This should be very safe patch, but I don't like this.
Comment 7 Olli Pettay [:smaug] 2010-06-18 03:39:58 PDT
Created attachment 452232 [details] [diff] [review]
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] 2010-06-18 05:43:16 PDT
Ah, on trunk SetText isn't called with notify=true.
Comment 9 Olli Pettay [:smaug] 2010-06-18 05:48:30 PDT
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 2010-06-18 08:12:59 PDT
(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 2010-06-18 08:14:13 PDT
Comment on attachment 452232 [details] [diff] [review]
patch

Why do you need the added RemoveChildAt call?
Comment 12 Daniel Veditz [:dveditz] 2010-06-18 10:36:21 PDT
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] 2010-06-18 11:25:17 PDT
(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 2010-06-18 14:30:55 PDT
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 [:bz] (still a bit busy) 2010-06-18 16:23:29 PDT
> 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 2010-06-22 13:57:34 PDT
can you update the status here? or request view?
Comment 17 Olli Pettay [:smaug] 2010-06-22 14:02:52 PDT
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] 2010-06-23 01:57:25 PDT
Created attachment 453323 [details] [diff] [review]
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 christian 2010-06-24 13:37:50 PDT
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 21 Al Billings [:abillings] 2010-07-16 14:35:25 PDT
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) 2010-07-18 21:57:37 PDT
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] 2010-07-22 19:17:44 PDT
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) 2010-07-24 14:50:37 PDT
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

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