Last Comment Bug 579655 - (CVE-2010-3166) heap overflow in text runs - crash [@ nsTextFrameUtils::TransformText]
(CVE-2010-3166)
: heap overflow in text runs - crash [@ nsTextFrameUtils::TransformText]
Status: RESOLVED FIXED
[sg:critical][critsmash:patch] [qa-ex...
: crash, testcase, verified1.9.2
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-17 09:01 PDT by Daniel Veditz [:dveditz]
Modified: 2011-06-13 10:01 PDT (History)
17 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
wanted
.9+
.9-fixed
.12+
.12-fixed


Attachments
Reduced testcase (580 bytes, text/html)
2010-07-19 14:55 PDT, Simon Montagu :smontagu
no flags Details
Testcase reduced still further (without the js and rtl) (222 bytes, text/html)
2010-07-19 15:06 PDT, Simon Montagu :smontagu
no flags Details
Frame tree before load() (10.11 KB, text/plain)
2010-07-20 12:27 PDT, Simon Montagu :smontagu
no flags Details
Frame tree after load() (9.79 KB, text/plain)
2010-07-20 12:29 PDT, Simon Montagu :smontagu
no flags Details
Patch (1.15 KB, patch)
2010-07-26 09:01 PDT, Simon Montagu :smontagu
roc: review+
christian: approval1.9.2.9+
christian: approval1.9.1.12+
Details | Diff | Review
tests (2.60 KB, patch)
2010-07-26 09:05 PDT, Simon Montagu :smontagu
no flags Details | Diff | Review

Description Daniel Veditz [:dveditz] 2010-07-17 09:01:56 PDT
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.
Comment 1 Daniel Veditz [:dveditz] 2010-07-17 09:05:34 PDT
Created attachment 458099 [details]
PoC
Comment 2 Reed Loden [:reed] (use needinfo?) 2010-07-17 13:59:16 PDT
Created attachment 458113 [details]
PoC
Comment 3 Simon Montagu :smontagu 2010-07-17 21:31:03 PDT
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 2010-07-17 21:46:09 PDT
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) (Exited; email my personal email if necessary) 2010-07-18 17:29:12 PDT
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 [:bz] 2010-07-18 18:52:57 PDT
> 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 2010-07-19 10:33:10 PDT
(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 2010-07-19 14:55:44 PDT
Created attachment 458438 [details]
Reduced testcase
Comment 9 Simon Montagu :smontagu 2010-07-19 15:06:10 PDT
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.
Comment 10 Simon Montagu :smontagu 2010-07-19 15:17:12 PDT
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 2010-07-20 12:27:08 PDT
Created attachment 458747 [details]
Frame tree before load()

Note the duplicated content 0xb0a88e20
Comment 12 Simon Montagu :smontagu 2010-07-20 12:29:41 PDT
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.
Comment 13 Daniel Veditz [:dveditz] 2010-07-21 10:45:44 PDT
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] 2010-07-21 10:48:56 PDT
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 2010-07-21 14:59:30 PDT
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 2010-07-26 02:13:55 PDT
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 2010-07-26 08:33:38 PDT
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 2010-07-26 09:01:48 PDT
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.
Comment 19 Simon Montagu :smontagu 2010-07-26 09:05:33 PDT
Created attachment 460259 [details] [diff] [review]
tests
Comment 20 Daniel Veditz [:dveditz] 2010-07-27 13:33:07 PDT
Does this also affect the 1.9.1 branch?
Comment 21 Simon Montagu :smontagu 2010-07-28 01:09:05 PDT
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) (Exited; email my personal email if necessary) 2010-08-03 12:49:18 PDT
Seems like this is ready to land.
Comment 24 christian 2010-08-10 12:06:57 PDT
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 2010-08-11 01:55:02 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ae8c9ea8a71f
Comment 26 Simon Montagu :smontagu 2010-08-11 02:22:19 PDT
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] 2010-08-17 14:54:21 PDT
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 30 Simon Montagu :smontagu 2010-10-20 11:12:29 PDT
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

Note You need to log in before you can comment on or make changes to this bug.