Bug 579655 (CVE-2010-3166)

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

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dveditz, Assigned: smontagu)

Tracking

({crash, testcase, verified1.9.2})

Trunk
crash, testcase, verified1.9.2
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)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 458099 [details]
PoC
(Reporter)

Updated

7 years ago
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → wanted
status2.0: --- → wanted
Created attachment 458113 [details]
PoC

Updated

7 years ago
Keywords: crash, testcase
Summary: heap overflow in text runs → heap overflow in text runs - crash [@ nsTextFrameUtils::TransformText]
(Assignee)

Comment 3

7 years ago
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?
(Assignee)

Comment 4

7 years ago
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.
(Assignee)

Comment 7

7 years ago
(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
(Assignee)

Comment 8

7 years ago
Created attachment 458438 [details]
Reduced testcase
Assignee: nobody → smontagu
(Assignee)

Comment 9

7 years ago
Created attachment 458444 [details]
Testcase reduced still further (without the js and rtl)

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.
(Assignee)

Comment 10

7 years ago
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 :(

Updated

7 years ago
Whiteboard: [sg:critical]
(Assignee)

Comment 11

7 years ago
Created attachment 458747 [details]
Frame tree before load()

Note the duplicated content 0xb0a88e20
(Assignee)

Comment 12

7 years ago
Created attachment 458749 [details]
Frame tree after load()

The content is no longer duplicated. The start and end pointers on frame 0xb0ab2db0 and its continuation are borked.

Updated

7 years ago
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]
(Reporter)

Updated

7 years ago
blocking1.9.2: ? → .8+
(Reporter)

Comment 13

7 years ago
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.
(Assignee)

Comment 15

7 years ago
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".
(Assignee)

Comment 16

7 years ago
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].
(Assignee)

Comment 17

7 years ago
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"'
(Assignee)

Comment 18

7 years ago
Created attachment 460257 [details] [diff] [review]
Patch

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

Comment 19

7 years ago
Created attachment 460259 [details] [diff] [review]
tests
(Reporter)

Comment 20

7 years ago
Does this also affect the 1.9.1 branch?

Updated

7 years ago
blocking2.0: ? → final+
(Assignee)

Comment 21

7 years ago
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.
Attachment #460257 - Flags: review?(roc) → review+
Seems like this is ready to land.
(Reporter)

Updated

7 years ago
blocking1.9.1: --- → .12+
status1.9.1: ? → wanted
(Reporter)

Updated

7 years ago
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:patch]
(Assignee)

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7b8dfb676be3
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Attachment #460257 - Flags: approval1.9.2.9?
Attachment #460257 - Flags: approval1.9.1.13?

Comment 24

7 years ago
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+
(Assignee)

Comment 25

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ae8c9ea8a71f
status1.9.2: wanted → .9-fixed
(Assignee)

Comment 26

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/430012b4f59a

I'll check in the testcases after we release with the fix.
status1.9.1: wanted → .12-fixed
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]
(Reporter)

Updated

7 years ago
Alias: CVE-2010-3166
(Reporter)

Updated

7 years ago
Group: core-security
(Assignee)

Comment 29

7 years ago
Checked in tests:

http://hg.mozilla.org/mozilla-central/rev/589f19de26c4
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/687bd1ef126b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/febfa330c454
Flags: in-testsuite? → in-testsuite+
(Assignee)

Comment 30

7 years ago
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.