Last Comment Bug 580445 - (CVE-2010-2766) normalizeDocument Remote Code Execution Vulnerability (ZDI-CAN-866)
(CVE-2010-2766)
: normalizeDocument Remote Code Execution Vulnerability (ZDI-CAN-866)
Status: RESOLVED FIXED
[sg:critical?]
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- critical (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-20 16:50 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2010-09-27 18:24 PDT (History)
11 users (show)
reed: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
?
.9+
.9-fixed
.12+
.12-fixed


Attachments
Fix. (577 bytes, patch)
2010-07-20 16:59 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jonas: review+
dveditz: approval1.9.2.9+
dveditz: approval1.9.1.12+
Details | Diff | Review

Description Reed Loden [:reed] (use needinfo?) 2010-07-20 16:50:01 PDT
Created attachment 458836 [details]
PoC

ZDI-CAN-866: Mozilla Firefox normalizeDocument 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 flaw exists within the normalizeDocument function defined within
nsDocument.cpp. When handling children nodes the code does not account
for a varying number of children during normalization. An attacker can
abuse this problem along with the fact that the code does not validate
the child index is within bounds to access an invalid object and execute
arbitrary code under the context of the browser.

From content/base/src/nsDocument.cpp:

nsDocument::Normalize()
{
PRInt32 count = mChildren.ChildCount();
for (PRInt32 i = 0; i < count; ++i) {
nsCOMPtr<nsIDOMNode> node(do_QueryInterface(mChildren.ChildAt(i)));

if (node) {
node->Normalize();
}
}

return NS_OK;
}

There are two problems in the code above:

1. Number of root's children can change during normalization. Thus
fetching
mChildren.ChildCount() at the very beginning is incorrect.
2. mChildren.ChildAt() is not a fail-safe method, i.e. it does not check
if index argument is within children array bounds.

Attacker can create HTML document with some additional nodes after HTML
body and then remove them during initial steps of normalization process.
This directly leads to arbitrary code execution.

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

This vulnerability was discovered by:
    * regenrecht
Comment 1 Reed Loden [:reed] (use needinfo?) 2010-07-20 16:53:49 PDT
bp-e842aa05-915a-4161-abd0-e7e7d2100720

Mozilla/5.0 (X11; Linux i686; rv:2.0b2pre) Gecko/20100720 Minefield/4.0b2pre
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-20 16:59:37 PDT
Created attachment 458839 [details] [diff] [review]
Fix.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-20 17:01:28 PDT
And "node" in that patch doesn't necessarily need to be an nsCOMPtr, but I think it's safer to leave it be a strong reference in case code down the call chain can trigger destruction of the node or what not.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2010-07-20 17:25:02 PDT
Comment on attachment 458839 [details] [diff] [review]
Fix.

stupid mutation events
Comment 5 Jesse Ruderman 2010-07-20 17:33:28 PDT
Please add a comment so we don't try to "optimize" this later and reintroduce the bug.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-20 20:34:10 PDT
Fixed.

http://hg.mozilla.org/mozilla-central/rev/bb303ad2db3e
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-20 20:34:59 PDT
Comment on attachment 458839 [details] [diff] [review]
Fix.

This applies cleanly to 1.9.1 and 1.9.2.
Comment 8 Reed Loden [:reed] (use needinfo?) 2010-07-20 22:30:18 PDT
(In reply to comment #5)
> Please add a comment so we don't try to "optimize" this later and reintroduce
> the bug.

Poke. A comment did not get added on commit...
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-20 23:54:25 PDT
Duh, totally missed that here. Jesse, do you want me to add a comment now and make it more obvious what we're fixing, or wait until we've released 3.6 etc with this and add a comment to trunk then?
Comment 10 Jesse Ruderman 2010-07-21 00:40:03 PDT
Add a comment when you add the test, I guess.

Btw, the algorithm is probably still "incorrect" in that it can skip children if earlier children are removed in mutation event handlers.  Switching from array indexing to following next-sibling pointers would help, and might improve performance.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-07-21 01:21:32 PDT
How would that help?

The only sane algorithm that can work for a case like this is to build a list of nodes to normalize up front, then normalize them all.  Chasing pointers, for example, can trivially go into an infinite loop if scripts that fire on normalize rearrange the DOM right.
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2010-07-21 09:28:37 PDT
Yes, one could argue that the algorithm is "incorrect" but I'm arguing that it's not worth making it "correct" here. There's nothing unsafe about the fixed version of this code that I can see, and if script in a document mutates the document during normalization then, well, it won't be normalized, and I say that's ok.
Comment 13 Daniel Veditz [:dveditz] 2010-07-21 11:05:12 PDT
Comment on attachment 458839 [details] [diff] [review]
Fix.

Approved for 1.9.2.8 and 1.9.1.12, a=dveditz for release-drivers
Comment 15 [On PTO until 6/29] 2010-08-09 17:24:25 PDT
Verified for 1.9.2.9 with PoC and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9pre) Gecko/20100809 Namoroka/3.6.9pre ( .NET CLR 3.5.30729). PoC crashes 1.9.2.8.

Verified for 1.9.1.12 with PoC and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.12pre) Gecko/20100809 Shiretoko/3.5.12pre ( .NET CLR 3.5.30729). Crashes 1.9.1.11.

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