Bug 579655 (CVE-2010-3166)

heap overflow in text runs - crash [@ nsTextFrameUtils::TransformText]

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dveditz, Assigned: smontagu)

Tracking

({crash, testcase, verified1.9.2})

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, status2.0 wanted, blocking1.9.2 .9+, status1.9.2 .9-fixed, blocking1.9.1 .12+, status1.9.1 .12-fixed)

Details

(Whiteboard: [sg:critical][critsmash:patch] [qa-examined-191], crash signature)

Attachments

(6 attachments)

wooshi@gmail.com sent the following to the security alias:

Firefox exploitable Vulnerability

Discovery Date: May 24, 2010
Discovery By : wushi of team509

Systems Affected

This vulnerability affects the following software :

  * mozilla firefox 3.6.6

Overview

firefox contains a vulnerability. This vulnerability may allow attackers
to remotely
execute arbitrary code on the affected system. Exploitation may occur as
the result of using the
affected webkit application to visit a website. The privileges gained by
a remote attacker depend on the software
component being attacked.


I. Description:
unpack the ff4.rar and got the 1.html , use mozilla firefox to open it(I
used
3.6.6 on windows xp sp3) firefox will crash.

the crash will like this:

(de8.9f4): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=0012b600 ebx=00000000 ecx=00000001 edx=0012daa5 esi=0012b620
edi=00130000
eip=101aca7a esp=0012b17c ebp=04c79a6a iopl=0 nv up ei pl nz ac pe cy
cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00010217
xul!nsTextFrameUtils::TransformText+0x5a:
101aca7a 8807 mov byte ptr [edi],al ds:0023:00130000=41
0:000> kv
ChildEBP RetAddr Args to Child
0012b194 101545a0 04c761cc fffffffc 00000002
xul!nsTextFrameUtils::TransformText+0x5a (FPO: [Uses EBP] [5,4,0])
(CONV: cdecl)
[e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframeutils.cpp
@ 220]
0012c74c 10137a0c 0012d7d8 0477b058 0012dab0
xul!BuildTextRunsScanner::BuildTextRunForFrames+0x3f0 (FPO: [Uses EBP]
[1,1380,4]) (CONV: thiscall)
[e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframethebes.cpp
@ 1657]
0012d770 6165696f 0000005d 00000000 00000000
xul!BuildTextRunsScanner::FlushFrames+0xac (FPO: [Uses EBP] [1,1031,3])
(CONV: thiscall)
[e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframethebes.cpp
@ 1215]
WARNING: Frame IP not in any known module. Following frames may be wrong.
0012d778 00000000 00000000 00000000 00000000 0x6165696f


check the source code, you can find this line cause the crash:

if (!nowInWhitespace) {
if (IsDiscardable(ch, &flags)) {
aSkipChars->SkipChar();
nowInWhitespace = inWhitespace;
} else {
*aOutput++ = ch; ; error is here
aSkipChars->KeepChar();
}

notice the stack you can find aLength set to 0xfffffffc, it is a mistake.

check the xul!BuildTextRunsScanner::BuildTextRunForFrames' source code,
you can find the error is here:

nsIContent* content = f->GetContent();
const nsTextFragment* frag = content->GetText();
PRInt32 contentStart = mappedFlow->mStartFrame->GetContentOffset();
PRInt32 contentEnd = mappedFlow->GetContentEnd();
PRInt32 contentLength = contentEnd - contentStart; // error is here

in the test case, the contentEnd < contentStart , so made the mistake
and firefox didn't check the number.
Posted file PoC (obsolete) —
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status2.0: --- → wanted
Keywords: crash, testcase
Summary: heap overflow in text runs → heap overflow in text runs - crash [@ nsTextFrameUtils::TransformText]
Assertions before the crash:

###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file /home/smontagu/mozwork/hg1.9.2/mozilla/layout/generic/nsTextFrame.h, line 322
###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file /home/smontagu/mozwork/hg1.9.2/mozilla/content/base/src/nsContentUtils.cpp, line 1702 (multiple times)
###!!! ASSERTION: SetMayHaveFrame failed?: 'mContent->MayHaveFrame()', file /home/smontagu/mozwork/hg1.9.2/mozilla/layout/generic/nsFrame.cpp, line 347
###!!! ASSERTION: Creating ContinuingTextFrame, but there is no more content: 'mContentOffset < PRInt32(GetFragment()->GetLength())', file /home/smontagu/mozwork/hg1.9.2/mozilla/layout/generic/nsTextFrameThebes.cpp, line 3491
###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file ../../dist/include/nsTextFragment.h, line 191
###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file /home/smontagu/mozwork/hg1.9.2/mozilla/layout/generic/nsTextFrame.h, line 322
###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file ../../dist/include/nsTextFragment.h, line 191
###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file /home/smontagu/mozwork/hg1.9.2/mozilla/layout/generic/nsTextFrame.h, line 322
###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file /home/smontagu/mozwork/hg1.9.2/mozilla/layout/generic/nsTextFrame.h, line 322
###!!! ASSERTION: integer overflow: 'mMaxTextLength <= mMaxTextLength + aFrame->GetContentLength()', file /home/smontagu/mozwork/hg1.9.2/mozilla/layout/generic/nsTextFrameThebes.cpp, line 1257

This was fixed on trunk in the range http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fa1e6f870ff1&tochange=f60b3bbfa8ce, which looks like bug 500882. Does that make sense, Boris?
The "wallpaper patch" from bug 538062 leaves the first two assertions but removes the others and the crash. Maybe we want to take that on branch, and a parallel assertion on trunk?
That sounds OK.

However, it might be worth digging into bug 500882 and finding what exactly fixed the bug there.

It's probably also worth reducing this testcase. It doesn't look like it's going to be that complicated, and maybe we can get an easy direct fix for the bug on branch.
> Does that make sense, Boris?

It could if in the old world we somehow failed to find the primary frame for some mutated text node and notify it of the mutation.  Worth checking at least which of the changesets made this bug disappear.
(In reply to comment #6)

> Worth checking at least
> which of the changesets made this bug disappear.

That would be http://hg.mozilla.org/mozilla-central/rev/e4438ed238da
Posted file Reduced testcase
Assignee: nobody → smontagu
It's interesting that this version of the testcase shows duplicated content on trunk but not on branch. I'm now searching for the fix for that.
The content duplication was fixed within http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c887dff0da&tochange=d6bb0f9e9519. It was probably fixed by the HTML5 parser, so so much for that as something which might be easier to port to branch :(
Whiteboard: [sg:critical]
Note the duplicated content 0xb0a88e20
The content is no longer duplicated. The start and end pointers on frame 0xb0ab2db0 and its continuation are borked.
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]
blocking1.9.2: ? → .8+
I'm not seeing a crash from the reduced testcase on 1.9.1.x, but I do see the duplicated text from attachment 458444 [details]. Does that mean 1.9.1.x needs a patch too, or is that an unrelated problem?
The reduced test cases don't crash 1.9.1 but you get the doubling of the text. In 1.9.2, only the PoC crashes as well. The others only do the doubling.
So, after experimenting for a while I don't think there's anything from bug 500882 which can help us here with a branch fix. If there could be a fix to the old HTML parser to prevent the doubling, that would be great, but someone who knows the parser code better than me had better do that. Otherwise let's just go for the "wallpaper patch".
Before I do that, here's another data point: if I add a DOCTYPE to attachment 458438 [details], 1.9.2 asserts 10 times "unexpected disconnected nodes: 'aDisconnected', file .../content/base/src/nsContentUtils.cpp, line 1702", but doesn't trigger any other assertions and doesn't crash.

A DOCTYPE doesn't prevent the duplicated content in attachment 458444 [details].
OK, that line of investigation was also less productive than I hoped: the crash returns with the DOCTYPE if I change 'style="white-space: pre"' to 'style="white‑space: pre-wrap"'
Posted patch PatchSplinter Review
In spite of what I said in comment 4, I think we want the assertion and the offset fix-up in both trunk and branch. The fix-up will potentially prevent other crashes, and if we have the assertion we are still likely to fix the underlying bugs.
Attachment #460257 - Flags: review?(roc)
Posted patch testsSplinter Review
Does this also affect the 1.9.1 branch?
blocking2.0: ? → final+
Even if the current testcases don't crash on 1.9.1, I think we want to take the patch there for safety, since the code being patched was checked in there from bug 332655.
Seems like this is ready to land.
blocking1.9.1: --- → .12+
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:patch]
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7b8dfb676be3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #460257 - Flags: approval1.9.2.9?
Attachment #460257 - Flags: approval1.9.1.13?
Comment on attachment 460257 [details] [diff] [review]
Patch

a=LegNeato for 1.9.2.9 and 1.9.1.12.

Please note that for these releases code-freeze is this Thursday, 2010-08-12 @ 11:59 pm PST. This needs to be landed as soon as possible.
Attachment #460257 - Flags: approval1.9.2.9?
Attachment #460257 - Flags: approval1.9.2.9+
Attachment #460257 - Flags: approval1.9.1.13?
Attachment #460257 - Flags: approval1.9.1.12+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/430012b4f59a

I'll check in the testcases after we release with the fix.
Flags: in-testsuite?
Verified for 1.9.2 using the PoC (which crashes 1.9.2.8) and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9pre) Gecko/20100817 Namoroka/3.6.9pre ( .NET CLR 3.5.30729) 

I don't have working repro steps for the crash for 1.9.1. None of the attached tests crash on 1.9.1.
Keywords: verified1.9.2
Whiteboard: [sg:critical][critsmash:patch] → [sg:critical][critsmash:patch] [qa-examined-191]
Alias: CVE-2010-3166
Group: core-security
The reftest failed on mozilla-1.9.1, but I don't think it's worth pursuing why that happens so I just removed it.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0b5e0f7e7c57
Crash Signature: [@ nsTextFrameUtils::TransformText]
You need to log in before you can comment on or make changes to this bug.