Closed Bug 580445 (CVE-2010-2766) Opened 12 years ago Closed 12 years ago

normalizeDocument Remote Code Execution Vulnerability (ZDI-CAN-866)

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status2.0 --- ?
blocking1.9.2 --- .9+
status1.9.2 --- .9-fixed
blocking1.9.1 --- .12+
status1.9.1 --- .12-fixed

People

(Reporter: reed, Assigned: jst)

Details

(Keywords: verified1.9.1, verified1.9.2, Whiteboard: [sg:critical?])

Attachments

(1 file)

Attached file 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
bp-e842aa05-915a-4161-abd0-e7e7d2100720

Mozilla/5.0 (X11; Linux i686; rv:2.0b2pre) Gecko/20100720 Minefield/4.0b2pre
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
status2.0: --- → ?
Attached patch Fix.Splinter Review
Attachment #458839 - Flags: review?(jonas)
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 on attachment 458839 [details] [diff] [review]
Fix.

stupid mutation events
Attachment #458839 - Flags: review?(jonas) → review+
Please add a comment so we don't try to "optimize" this later and reintroduce the bug.
Fixed.

http://hg.mozilla.org/mozilla-central/rev/bb303ad2db3e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 458839 [details] [diff] [review]
Fix.

This applies cleanly to 1.9.1 and 1.9.2.
Attachment #458839 - Flags: approval1.9.2.8?
Attachment #458839 - Flags: approval1.9.1.12?
(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...
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?
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.
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.
Assignee: nobody → jst
Flags: in-testsuite?
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.
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .8+
Comment on attachment 458839 [details] [diff] [review]
Fix.

Approved for 1.9.2.8 and 1.9.1.12, a=dveditz for release-drivers
Attachment #458839 - Flags: approval1.9.2.8?
Attachment #458839 - Flags: approval1.9.2.8+
Attachment #458839 - Flags: approval1.9.1.12?
Attachment #458839 - Flags: approval1.9.1.12+
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.
Alias: CVE-2010-2766
blocking2.0: ? → final+
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.