Bug 572986 (CVE-2010-1208)

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

RESOLVED FIXED in mozilla1.9.2

Status

()

Core
DOM
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: reed, Assigned: smaug)

Tracking

({fixed1.9.0.20, verified1.9.1, verified1.9.2})

unspecified
mozilla1.9.2
fixed1.9.0.20, verified1.9.1, verified1.9.2
Points:
---

Firefox Tracking Flags

(blocking2.0 final+, status2.0 ?, blocking1.9.2 .7+, status1.9.2 .7-fixed, blocking1.9.1 .11+, status1.9.1 .11-fixed)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
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
(Assignee)

Comment 2

7 years ago
IIRC, either boris or jonas changed attribute handling a bit.
(Assignee)

Comment 3

7 years ago
But I'm investigating this on 1.9.2.
I think I know where the problem is.
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 6

7 years ago
Created attachment 452225 [details] [diff] [review]
very safe patch

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

Comment 7

7 years ago
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.
Attachment #452232 - Flags: review?(jonas)
(Assignee)

Comment 8

7 years ago
Ah, on trunk SetText isn't called with notify=true.
(Assignee)

Comment 9

7 years ago
This was fixed on trunk in  Bug 429175.
Should we actually land that to branches too.
(Assignee)

Updated

7 years ago
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
status1.9.2: ? → wanted
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?)
(Assignee)

Comment 13

7 years ago
(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?

Updated

7 years ago
blocking1.9.1: ? → .11+
blocking1.9.2: needed → .6+
status1.9.1: ? → wanted

Comment 16

7 years ago
can you update the status here? or request view?
(Assignee)

Comment 17

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

Comment 18

7 years ago
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.
Attachment #453323 - Flags: review?(jonas)
Attachment #453323 - Flags: review?(jonas) → review+
(Assignee)

Updated

7 years ago
Attachment #453323 - Flags: approval1.9.2.6?
Attachment #453323 - Flags: approval1.9.1.11?

Comment 19

7 years ago
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+
(Reporter)

Comment 20

7 years ago
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
Last Resolved: 7 years ago
status1.9.1: wanted → .11-fixed
status1.9.2: wanted → .6-fixed
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.
Keywords: verified1.9.1, verified1.9.2
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+
You need to log in before you can comment on or make changes to this bug.