Last Comment Bug 590771 - (CVE-2010-3766) nsDOMAttribute MutationObserver Remote Code Execution Vulnerability (ZDI-CAN-898)
(CVE-2010-3766)
: nsDOMAttribute MutationObserver Remote Code Execution Vulnerability (ZDI-CAN-...
Status: RESOLVED FIXED
[sg:critical?]
: crash, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 598877 600466 600468 600471 CVE-2011-3659
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-25 16:22 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2013-04-04 13:53 PDT (History)
9 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
.13+
.13-fixed
.16+
.16-fixed


Attachments
PoC (420 bytes, text/html)
2010-08-25 16:22 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details
Tests for 1.9.1 and 1.9.2 (1.92 KB, patch)
2010-09-27 16:30 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Review
Patch for 1.9.1 and 1.9.2 [checked-in] (13.80 KB, patch)
2010-09-27 16:31 PDT, Mounir Lamouri (:mounir)
jonas: review+
dveditz: approval1.9.2.13+
dveditz: approval1.9.1.16+
Details | Diff | Review
Tests for trunk (2.84 KB, patch)
2010-09-29 00:43 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Review
Patch for trunk [checked-in] (15.74 KB, patch)
2010-09-29 00:44 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Review

Description Reed Loden [:reed] (use needinfo?) 2010-08-25 16:22:33 PDT
Created attachment 469260 [details]
PoC

ZDI-CAN-898: Mozilla Firefox nsDOMAttribute MutationObserver Remote Code Execution Vulnerability

-- CVSS ----------------------------------------------------------------
10, (AV:N/AC:L/Au:N/C:C/I:C/A:C)

-- 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 within the application's support of an API used
for element traversal. Due to a particular element not implementing
functionality required by the API, a use-after free vulnerability can be
made to occur. This can be used to achieve code execution under the
context of the application.

The issue occurs within the application's support of the NodeIterator
API that is used for DOM Traversal as outlined in the DOM specification
on w3.org. 

In order to support the node iteration, the application needs to keep
track of modifications to the dom tree in order to update the state of
the iterator. This is done explicitly by a pair of methods called
AddMutationObserver/RemoveMutationObserver. Anytime an element is
created, the observer list will be updated so that a node iterator can
adjust for modified elements.

When adding an element, it will need to update the MutationObserver list
as in the following code.

content/base/src/nsNodeIterator.cpp:185
nsNodeIterator::nsNodeIterator(nsINode *aRoot,
                               PRUint32 aWhatToShow,
                               nsIDOMNodeFilter *aFilter,
                               PRBool aExpandEntityReferences) :
    nsTraversal(aRoot, aWhatToShow, aFilter, aExpandEntityReferences),
    mDetached(PR_FALSE),
    mPointer(mRoot, PR_TRUE)
{
    aRoot->AddMutationObserver(this);         // XXX
}

nsNodeIterator::~nsNodeIterator()
{
    /* destructor code */
    if (!mDetached && mRoot)
        mRoot->RemoveMutationObserver(this);    // XXX
}

AddMutationObserver/RemoveMutationObserver will add the provided element
to a page-global list that can be queried by the NodeIterator.

content/base/src/nsGenericElement.cpp:295
void
nsINode::AddMutationObserver(nsIMutationObserver* aMutationObserver)
{
  nsSlots* slots = GetSlots();
  if (slots) {
   
slots->mMutationObservers.AppendElementUnlessExists(aMutationObserver);
  }
}

void
nsINode::RemoveMutationObserver(nsIMutationObserver* aMutationObserver)
{
  nsSlots* slots = GetExistingSlots();
  if (slots) {
    slots->mMutationObservers.RemoveElement(aMutationObserver);
  }
}

One element however does not update the mutation list. This is the
nsDOMAttribute and is defined in content/base/src/nsDOMAttribute.cpp.
This will allow someone to manipulate the nsDOMAttribute node without
the nsNodeIterator api being informed. This will bypass the type-safety
of the compiler and can lead to code execution.

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

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

This vulnerability was discovered by:
    * regenrecht
Comment 1 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-08-26 05:52:45 PDT
I get a js error on trunk:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMAttr.removeChild]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: https://bug590771.bugzilla.mozilla.org/attachment.cgi?id=469260&t=BcPe7a6cgh :: run :: line 11"  data: no]

On Firefox3.6.8, I get an alert saying "[object Text]".
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2010-08-31 14:45:13 PDT
Mounir will investigate.
Comment 3 Mounir Lamouri (:mounir) 2010-09-16 23:13:42 PDT
I believe this has been fixed on trunk with bug 540848 with the following changesets:
http://hg.mozilla.org/mozilla-central/rev/068150b06d11
http://hg.mozilla.org/mozilla-central/rev/13571a9165ea
Comment 4 Mounir Lamouri (:mounir) 2010-09-23 01:53:56 PDT
Forget comment 3, this is not related actually.

Very likely, this is fixed on trunk because of a regression of bug 566416. I filed bug 598877 about the regression. Quickly, attributes.firstChild returns null in the test case with the current trunk which is wrong and make the script fail too early.
This bug expected, this is not fixed on trunk apparently.
Comment 5 Mounir Lamouri (:mounir) 2010-09-27 16:30:42 PDT
Created attachment 478909 [details] [diff] [review]
Tests for 1.9.1 and 1.9.2
Comment 6 Mounir Lamouri (:mounir) 2010-09-27 16:31:34 PDT
Created attachment 478910 [details] [diff] [review]
Patch for 1.9.1 and 1.9.2 [checked-in]
Comment 7 Daniel Veditz [:dveditz] 2010-09-27 16:42:46 PDT
Comment on attachment 478910 [details] [diff] [review]
Patch for 1.9.1 and 1.9.2 [checked-in]

Please get reviews before asking for branch approval
Comment 8 Daniel Veditz [:dveditz] 2010-09-27 16:43:44 PDT
Does this affect 1.9.1? If not, do we know what bug introduced the flaw?
Comment 9 Mounir Lamouri (:mounir) 2010-09-27 16:49:06 PDT
(In reply to comment #8)
> Does this affect 1.9.1? If not, do we know what bug introduced the flaw?

I'm building 1.9.1 to check that.
Asking for 2.0 blocking considering this is affecting the trunk too.
Comment 10 Mounir Lamouri (:mounir) 2010-09-27 16:57:02 PDT
(In reply to comment #8)
> Does this affect 1.9.1? If not, do we know what bug introduced the flaw?

1.9.1 is also affected. The same patch applies and work.
Comment 11 Daniel Veditz [:dveditz] 2010-09-28 13:39:26 PDT
Do we need a different trunk patch after the regression is fixed?
Comment 12 Mounir Lamouri (:mounir) 2010-09-28 14:14:34 PDT
(In reply to comment #11)
> Do we need a different trunk patch after the regression is fixed?

Yes. The code has changed so the current patch will not fix the issue on trunk.
Comment 13 Jonas Sicking (:sicking) 2010-09-28 15:20:54 PDT
Comment on attachment 478910 [details] [diff] [review]
Patch for 1.9.1 and 1.9.2 [checked-in]

Add a comment to the declaration of AttributeChildRemoved that says that attributes never have more than 1 children and so this is always that one single child that is removed.

>diff --git a/content/base/src/nsNodeIterator.cpp b/content/base/src/nsNodeIterator.cpp
>--- a/content/base/src/nsNodeIterator.cpp
>+++ b/content/base/src/nsNodeIterator.cpp
>@@ -212,17 +212,17 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsNodeIterator)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mRoot)
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFilter)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> // QueryInterface implementation for nsNodeIterator
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsNodeIterator)
>     NS_INTERFACE_MAP_ENTRY(nsIDOMNodeIterator)
>-    NS_INTERFACE_MAP_ENTRY(nsIMutationObserver)
>+    NS_INTERFACE_MAP_ENTRY(nsIMutationObserver2)
>     NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMNodeIterator)
>     NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(NodeIterator)
> NS_INTERFACE_MAP_END

You need to keep both interfaces in the interface map. We still want QIing to nsIMutationObserver to work.


>+nsNodeUtils::AttributeChildRemoved(nsINode* aAttribute,
>+                                   nsIContent* aChild)
>+{
>+  NS_PRECONDITION(aAttribute->IsNodeOfType(nsINode::eATTRIBUTE),
>+                  "container must be a nsIAttribute");
>+
>+  // This is a variant of IMPL_MUTATION_NOTIFICATION.

Add a comment to IMPL_MUTATION_NOTIFICATION so that if someone modifies that, this code is modified too.

r=me with those changes.
Comment 14 Mounir Lamouri (:mounir) 2010-09-28 15:28:55 PDT
Shouldn't this be blocking 1.9.1 too?
Comment 15 Mounir Lamouri (:mounir) 2010-09-28 16:55:00 PDT
Removing approval requests given that I've been told on IRC that this shouldn't land for this release.
Comment 16 Mounir Lamouri (:mounir) 2010-09-28 16:55:31 PDT
(In reply to comment #15)
> Removing approval requests given that I've been told on IRC that this shouldn't
> land for this release.

1.9.1 and 1.9.2 branches.
Comment 17 Mounir Lamouri (:mounir) 2010-09-29 00:43:57 PDT
Created attachment 479331 [details] [diff] [review]
Tests for trunk
Comment 18 Mounir Lamouri (:mounir) 2010-09-29 00:44:28 PDT
Created attachment 479332 [details] [diff] [review]
Patch for trunk [checked-in]
Comment 19 Mounir Lamouri (:mounir) 2010-09-29 00:46:29 PDT
I actually found a few regressions from bug 566416 and my patch for trunk is built on top of them, so I'm marking them as dependencies.
Comment 20 Mounir Lamouri (:mounir) 2010-10-12 07:51:21 PDT
Comment on attachment 479332 [details] [diff] [review]
Patch for trunk [checked-in]

Patch pushed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/56403a9ae8b6
Comment 21 Jonas Sicking (:sicking) 2010-10-12 13:50:07 PDT
Should this be marked FIXED?

If not, what remains to do?
Comment 22 Mounir Lamouri (:mounir) 2010-10-12 15:26:21 PDT
Patches should be pushed on branches (according to branch rules, I have to wait a few days before asking for an approval and it's not really urgent). In addition, I think that tests will have to be pushed after branch releases.
Comment 23 Mounir Lamouri (:mounir) 2010-10-18 10:07:41 PDT
Pushed in 1.9.1 and 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bcfb355e5c2b
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dc30f8b144a9

I will push all tests after 1.9.1 and 1.9.2 branching.
Comment 24 Reed Loden [:reed] (use needinfo?) 2010-10-31 13:25:21 PDT
Fixed on trunk == RESOLVED FIXED. Branches are tracked separately.
Comment 25 Reed Loden [:reed] (use needinfo?) 2010-10-31 13:27:34 PDT
(In reply to comment #22)
> Patches should be pushed on branches (according to branch rules, I have to wait
> a few days before asking for an approval and it's not really urgent). In
> addition, I think that tests will have to be pushed after branch releases.

(In reply to comment #23)
> Pushed in 1.9.1 and 1.9.2:
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bcfb355e5c2b
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dc30f8b144a9

What happened to the "asking for an approval" thing? *all* branch patches require approval before landing.

> I will push all tests after 1.9.1 and 1.9.2 branching.

No, wait until after the release has been made before landing tests.
Comment 26 Mounir Lamouri (:mounir) 2010-10-31 18:13:15 PDT
> (In reply to comment #23)
> > Pushed in 1.9.1 and 1.9.2:
> > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bcfb355e5c2b
> > http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dc30f8b144a9
> 
> What happened to the "asking for an approval" thing? *all* branch patches
> require approval before landing.

It has been marked as 'blocking'. Isn't that enough?

> > I will push all tests after 1.9.1 and 1.9.2 branching.
> 
> No, wait until after the release has been made before landing tests.

That's what I meant.
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-10-31 18:33:33 PDT
No, the approval policy for released branches is different than the one for under-development branches. For released branches all patches require approval, regardless of blocking status.
Comment 28 Daniel Veditz [:dveditz] 2010-11-01 10:44:08 PDT
Note that eventually Firefox 4 will end up in that state -- think of the branches as perpetual "Release Candidate" stage. Rules for each tree are always on their tinderboxen, and they differ. Please check.
Comment 29 Daniel Veditz [:dveditz] 2010-11-01 10:47:48 PDT
Comment on attachment 478910 [details] [diff] [review]
Patch for 1.9.1 and 1.9.2 [checked-in]

retroactive approval for 1.9.2.13 and 1.9.1.16, a=dveditz for release-drivers
Comment 30 Al Billings [:abillings] 2010-11-17 17:01:12 PST
Verified fixed for 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.13pre) Gecko/20101117 Namoroka/3.6.13pre and for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.16pre) Gecko/20101117 Shiretoko/3.5.16pre using PoC.
Comment 31 Mounir Lamouri (:mounir) 2010-12-17 20:10:56 PST
Tests pushed on trunk:
http://hg.mozilla.org/mozilla-central/rev/be885fbb66c2
Comment 32 Mounir Lamouri (:mounir) 2010-12-29 04:25:39 PST
Tests pushed in 1.9.1 and 1.9.2 branches:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d1f3e4b9a9d9
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9e561d402701

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