579655 - (CVE-2010-3166) heap overflow in text runs - crash [@ nsTextFrameUtils::TransformText]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Veditz [:dveditz]]

[e-mail] 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.

Comment 1

[Daniel Veditz [:dveditz]]

Attachment: PoC

Comment 2

[Reed Loden [:reed]]

Attachment: PoC

Comment 3

[Simon Montagu :smontagu]

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?

Comment 4

[Simon Montagu :smontagu]

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?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Simon Montagu :smontagu]

(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

Comment 8

[Simon Montagu :smontagu]

Attachment: Reduced testcase

Comment 9

[Simon Montagu :smontagu]

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

Comment 10

[Simon Montagu :smontagu]

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

Comment 11

[Simon Montagu :smontagu]

Attachment: Frame tree before load()

Note the duplicated content 0xb0a88e20

Comment 12

[Simon Montagu :smontagu]

Attachment: Frame tree after load()

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

Comment 13

[Daniel Veditz [:dveditz]]

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?

Comment 14

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

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.

Comment 15

[Simon Montagu :smontagu]

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".

Comment 16

[Simon Montagu :smontagu]

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

Comment 17

[Simon Montagu :smontagu]

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"'

Comment 18

[Simon Montagu :smontagu]

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

Comment 19

[Simon Montagu :smontagu]

Attachment: tests

Comment 20

[Daniel Veditz [:dveditz]]

Does this also affect the 1.9.1 branch?

Comment 21

[Simon Montagu :smontagu]

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems like this is ready to land.

Comment 23

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7b8dfb676be3

Comment 24

[LegNeato]

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.

Comment 25

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ae8c9ea8a71f

Comment 26

[Simon Montagu :smontagu]

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

I'll check in the testcases after we release with the fix.

Comment 27

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

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.

Comment 29

[Simon Montagu :smontagu]

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

Comment 30

[Simon Montagu :smontagu]

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