580445 - (CVE-2010-2766) normalizeDocument Remote Code Execution Vulnerability (ZDI-CAN-866)

Status: RESOLVED FIXED

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

Description

[Reed Loden [:reed]]

Attachment: 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]]

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)]

Attachment: Fix.

Comment 3

[Johnny Stenback (:jst)]

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) No longer reading bugmail consistently]

Comment on attachment 458839 [details] [diff] [review]
Fix.

stupid mutation events

Comment 5

[Jesse Ruderman]

Please add a comment so we don't try to "optimize" this later and reintroduce the bug.

Comment 6

[Johnny Stenback (:jst)]

Fixed.

http://hg.mozilla.org/mozilla-central/rev/bb303ad2db3e

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 458839 [details] [diff] [review]
Fix.

This applies cleanly to 1.9.1 and 1.9.2.

Comment 8

[Reed Loden [:reed]]

(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)]

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]

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 [:bzbarsky]]

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)]

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]]

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 14

[Johnny Stenback (:jst)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5da73af750ce
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c1d0170d5045

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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.