Last Comment Bug 607160 - (CVE-2011-0058) Buffer size calculation failure when running an absurdly long and complex text run
(CVE-2011-0058)
: Buffer size calculation failure when running an absurdly long and complex tex...
Status: RESOLVED FIXED
[sg:critical] [qa-ntd-191] [qa-ntd-192]
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: 1.9.2 Branch
: x86 Windows 7
: -- critical (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-25 16:58 PDT by Alex Miller
Modified: 2014-09-04 09:39 PDT (History)
11 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.14+
.14-fixed
.17+
.17-fixed


Attachments
A stack trace with important pieces highlighted (50.34 KB, image/png)
2010-10-25 17:00 PDT, Alex Miller
no flags Details
Testcase (713 bytes, text/html)
2010-10-25 17:04 PDT, Alex Miller
no flags Details
patch, check for overflow when calculating buffer size (3.17 KB, patch)
2010-11-24 05:52 PST, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review
patch for 1.9.1 and 1.9.2 branches (3.21 KB, patch)
2010-12-23 03:15 PST, Jonathan Kew (:jfkthame)
christian: approval1.9.2.14+
christian: approval1.9.1.17+
Details | Diff | Splinter Review

Description Alex Miller 2010-10-25 16:58:29 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11

When creating a 2684354575-character long text run, containing many complex unicode characters and its font-weight property is 65537, inside a <div> that is 3px wide, the buffer size for a text run is incorrectly calculated. In my tests, I found that the program exits safely, but an attacker could leverage an incorrectly calculated buffer size to run arbitrary code.

Reproducible: Always

Steps to Reproduce:
1. View the testcase.
Actual Results:  
A sustained period of hanging, followed by a "Microsoft C++ Runtime" exception.

Expected Results:  
It should have just been a DOS.

This was only tested against Windows 7 Home Edition. View the attached stack trace for proof of an incorrectly calculated buffer.
Comment 1 Alex Miller 2010-10-25 17:00:36 PDT
Created attachment 485924 [details]
A stack trace with important  pieces highlighted

Take a look at what is in green.
Comment 2 Alex Miller 2010-10-25 17:04:54 PDT
Created attachment 485928 [details]
Testcase

Testcase
Comment 4 Brandon Sterne (:bsterne) 2010-10-25 21:49:33 PDT
Alex, can you paste a link to a crash report please?
http://support.mozilla.com/en-US/kb/Mozilla+Crash+Reporter#Viewing_crash_reports
Comment 5 Alex Miller 2010-10-25 21:52:15 PDT
(In reply to comment #4)
> Alex, can you paste a link to a crash report please?
> http://support.mozilla.com/en-US/kb/Mozilla+Crash+Reporter#Viewing_crash_reports

There isn't a crash report. To an end-user, the program terminates fully, including the crash reporter. The stack trace was taken at the time of the exception.
Comment 6 Jonathan Kew (:jfkthame) 2010-10-27 04:57:39 PDT
The stack from comment #1 looks partly bogus; in particular, the sequence BeginLineReflow() : arena_malloc_small() : ForEachFontInternal() etc makes no sense to me, nor does the random JS frame in the middle of the stack.

I tried this several times on both trunk and 1.9.2, but have not been able to trigger anything other than a straightforward out-of-memory exception/abort.

However, I do think there's a risk of integer overflow and hence incorrect buffer allocation here:

  http://mxr.mozilla.org/mozilla1.9.2/source/layout/generic/nsTextFrameThebes.cpp#1212

mMaxTextLength here is a PRUint32 value. It accumulates the total text length at

  http://mxr.mozilla.org/mozilla1.9.2/source/layout/generic/nsTextFrameThebes.cpp#1257

but (a) the integer-overflow assertion here will do nothing to protect a release build, which will blindly proceed with the wrong value, and (b) even if the accumulator doesn't actually overflow, the subsequent multiplication by 2 to calculate the buffer size for double-byte text could overflow. In either case, the result will be that the buffer allocated and passed to BuildTextRunForFrames will be too small for the number of characters we're going to collect in it.
Comment 7 Jonathan Kew (:jfkthame) 2010-10-27 07:20:05 PDT
There's also a risk of overflow in gfxTextRun::operator new(), where the total size needed for the textRun object (i.e. the object itself, plus the CompressGlyph and text arrays) is being computed.

http://mxr.mozilla.org/mozilla1.9.2/source/gfx/thebes/src/gfxFont.cpp#1798
Comment 8 Jonathan Kew (:jfkthame) 2010-10-27 07:24:48 PDT
(In reply to comment #7)
> There's also a risk of overflow in gfxTextRun::operator new(), where the total
> size needed for the textRun object (i.e. the object itself, plus the
> CompressGlyph and text arrays) is being computed.
> 
> http://mxr.mozilla.org/mozilla1.9.2/source/gfx/thebes/src/gfxFont.cpp#1798

This aspect was fixed on trunk in bug 545989, but remains a potential issue on branch.
Comment 9 Alex Miller 2010-10-27 11:33:58 PDT
(In reply to comment #6)
> I tried this several times on both trunk and 1.9.2, but have not been able to
> trigger anything other than a straightforward out-of-memory exception/abort.

Are you running Win7 Home Edition on your test machine?
Comment 10 Jonathan Kew (:jfkthame) 2010-10-27 11:50:01 PDT
I have Windows 7 Home Premium (64-bit version, with 12GB installed RAM) here.

As noted in comments 6 and 7, I see several places where there seems to be some risk we might calculate a buffer size incorrectly due to arithmetic overflow, with the result that we'd allocate too little space and then overwrite arbitrary memory; I just haven't yet managed to trigger these overflows without hitting an out-of-memory condition that safely terminates the browser. That doesn't mean it can't happen, though.
Comment 11 Alex Miller 2010-10-27 12:18:32 PDT
(In reply to comment #10)
> I have Windows 7 Home Premium (64-bit version, with 12GB installed RAM) here.
> 
> As noted in comments 6 and 7, I see several places where there seems to be some
> risk we might calculate a buffer size incorrectly due to arithmetic overflow,
> with the result that we'd allocate too little space and then overwrite
> arbitrary memory; I just haven't yet managed to trigger these overflows without
> hitting an out-of-memory condition that safely terminates the browser. That
> doesn't mean it can't happen, though.

Sorry then.
Comment 12 Alex Miller 2010-11-06 15:52:46 PDT
I modified the EIP variable in the attached testcase and there was an actual crash.

http://crash-stats.mozilla.com/report/index/4ff083d9-f5dd-4a6b-96dc-fbabc2101106
Comment 13 Alex Miller 2010-11-06 17:31:37 PDT
Oh yeah, by the way, the eip variable in the testcase overwrites the parameters for BuildTextRuns, except nsIFrame* aLineContainer, which is another possible issue.
Comment 14 Brandon Sterne (:bsterne) 2010-11-15 11:00:56 PST
The reporter has evidence of some additional potential memory corruption.  In his stack the xul!BuildTextRuns frame looks badly corrupted:

0022a914 6db7c52b KERNELBASE!RaiseException+0x58
0022a94c 6db84f13 MOZCRT19!_CxxThrowException(void * pExceptionObject = 0x0022a95c, struct _s__ThrowInfo * pThrowInfo = 0x6dbeaa24)+0x46 [f:\sp\vctools\crt_bld\self_x86\crt\prebuild\eh\throw.cpp @ 161]
0022a964 67ef056f MOZCRT19!operator new(unsigned int size = 0x67f93440)+0x73
 [e:\builds\moz2_slave\win32_build\build\obj-firefox\memory\jemalloc\crtsrc\new.cpp @ 61]
0022afb8 67f93440 xul!TextRunWordCache::MakeTextRun(wchar_t * aText = 0x27e00008 "???", unsigned int aLength = 0xa00000f, class gfxFontGroup * aFontGroup = 0x00000003, struct gfxTextRunFactory::Parameters * aParams = 0x0022b090, unsigned int aFlags = 0x1440100)+0x7f [e:\builds\moz2_slave\win32_build\build\gfx\thebes\src\gfxtextrunwordcache.cpp @ 579]
0022afe0 67efc34d xul!MakeTextRun(wchar_t * aText = 0x27e00008 "???", unsigned int aLength = 0xa00000f, class gfxFontGroup * aFontGroup = 0x00000003, struct gfxTextRunFactory::Parameters * aParams = 0x0022b090, unsigned int aFlags = 0x1440100)+0x39
 [e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframethebes.cpp @ 436]
0022c4e4 67eccd0c xul!BuildTextRunsScanner::BuildTextRunForFrames(void * aTextBuffer = 0x00000003)+0xb8d [e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframethebes.cpp @ 1783]
0022d514 67f080ea xul!BuildTextRunsScanner::FlushFrames(int aFlushLineBreaks = <Memory access error>, int aSuppressTrailingBreak = 3)+0xac [e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframethebes.cpp @ 1215]
0022d60c 680c7361
 xul!BuildTextRuns(class gfxContext * aContext = 41414141, class nsTextFrame * aForFrame = 41414141, class nsIFrame * aLineContainer = 0x00000003, class nsLineList_iterator * aForFrameLine = 41414141)+0x32a [e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframethebes.cpp @ 1149]

The rest of his email to security@mozilla.org follows:

As far as I can tell, everybody who has analyzed the crash has seen that it is a result of an OOM being thrown by jemalloc, which is not exploitable. Perhaps I was the only one to notice however, that sections of the stack were overwritten with string data, when calling BuildTextRuns(). Proof aside from the raw stack trace is actually in an odd place: when executing the provided testcase on Windows 7 Home Edition with Firefox 3.6.12, the "unresponsive script" dialog appears, but contains no text and a "C++ runtime exception" occurs. I believe this is a result of the gfxRenderingContext being overwritten, meaning that text runs are in an invalid state, but not actually causing a crash by pointing to an invalid memory address. Further evidence for this is when I modified the "eip" variable to 80000001 (2^31 + 1), a crash actually occured because it's an invalid address.
Comment 15 Alex Miller 2010-11-23 15:42:22 PST
Just so that last fiasco doesn't repeat itself, I'm going to get attributions out of the way now.

This testcase is somewhat based on my last testcase, which was based on the testcase for BID 38994.
Comment 16 Jonathan Kew (:jfkthame) 2010-11-24 05:52:26 PST
Created attachment 492981 [details] [diff] [review]
patch, check for overflow when calculating buffer size

Two issues that could lead to buffer overflow during textrun construction are addressed here: although BuildTextRunsScanner::FlushFrames() checks for allocation failure when it creates the buffer to pass to BuildTextRunForFrames(), and simply returns if memory isn't available, there's the potential for integer overflow when multiplying mMaxTextLength by 2.

(We could use 64-bit arithmetic for this to avoid the overflow, but there's no point given that we can't allocate such a big array anyway.)

Upstream from the actual point of allocation in FlushFrames(), there's also a problem in BuildTextRunsScanner::AccumulateRunInfo() because a huge document could cause mMaxTextLength to overflow when a new frame's ContentLength() is added. To address this, we clamp mMaxTextLength to PR_UINT32_MAX as an indication that the calculation has overflowed, and then FlushFrames() won't even try to proceed.

If we hit this overflow situation (or the buffer allocation fails because memory is tight), we get a spew of repeated assertions from layout:

###!!! ASSERTION: Should have been cleared: 'mBreakSinks.IsEmpty()', file /Users/jonathan/mozdev/mozilla-central/layout/generic/nsTextFrameThebes.cpp, line 722
###!!! ASSERTION: Should have been cleared: 'mMappedFlows.IsEmpty()', file /Users/jonathan/mozdev/mozilla-central/layout/generic/nsTextFrameThebes.cpp, line 725
###!!! ASSERTION: Should have Reset() before destruction!: 'mCurrentWord.Length() == 0', file /Users/jonathan/mozdev/mozilla-central/content/base/src/nsLineBreaker.cpp, line 51

but it seems to proceed safely and ends up displaying the document with text missing. (Tested by artificially setting the "overflow" threshold much smaller, and then loading large documents such as HTML5 and tinderbox logs.)
Comment 17 Jonathan Kew (:jfkthame) 2010-11-25 07:49:28 PST
Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/eb8d110a117a

This should prevent at least one possible source of incorrect text buffer allocation.

I'm leaving the bug open for now, as it's not clear whether there may be additional issues... I've never been able to reproduce anything besides a normal out-of-memory crash using this testcase locally, so further testing once this is in trunk nightlies would be appreciated.
Comment 18 Alex Miller 2010-11-25 14:21:00 PST
(In reply to comment #17)
> Pushed to trunk:
> http://hg.mozilla.org/mozilla-central/rev/eb8d110a117a
> 
> This should prevent at least one possible source of incorrect text buffer
> allocation.
> 
> I'm leaving the bug open for now, as it's not clear whether there may be
> additional issues... I've never been able to reproduce anything besides a
> normal out-of-memory crash using this testcase locally, so further testing once
> this is in trunk nightlies would be appreciated.

With your patch I did get these warnings:

WARNING: failed to allocate glyph/text storage for text run!: file /home/alex/Downloads/mozilla-central/gfx/thebes/gfxFont.cpp, line 2824
WARNING: failed to allocate glyph/text storage for text run!: file /home/alex/Downloads/mozilla-central/gfx/thebes/gfxFont.cpp, line 2824
WARNING: NS_ENSURE_TRUE(mTextRun) failed: file /home/alex/Downloads/mozilla-central/layout/generic/nsTextFrameThebes.cpp, line 6835
WARNING: Subdocument container has no content: file /home/alex/Downloads/mozilla-central/layout/base/nsDocumentViewer.cpp, line 2379
WARNING: NS_ENSURE_SUCCESS(rv, 0) failed with result 0x8000FFFF: file /home/alex/Downloads/mozilla-central/content/base/src/nsContentUtils.cpp, line 2917
WARNING: NS_ENSURE_TRUE(pusher.Push(aBoundElement)) failed: file /home/alex/Downloads/mozilla-central/content/xbl/src/nsXBLProtoImplMethod.cpp, line 331
WARNING: NS_ENSURE_SUCCESS(rv, 0) failed with result 0x8000FFFF: file /home/alex/Downloads/mozilla-central/content/base/src/nsContentUtils.cpp, line 2917
WARNING: NS_ENSURE_TRUE(pusher.Push(aBoundElement)) failed: file /home/alex/Downloads/mozilla-central/content/xbl/src/nsXBLProtoImplMethod.cpp, line 331
Comment 19 Jonathan Kew (:jfkthame) 2010-11-27 03:09:30 PST
(In reply to comment #18)
> With your patch I did get these warnings:
> 
> WARNING: failed to allocate glyph/text storage for text run!: file
> /home/alex/Downloads/mozilla-central/gfx/thebes/gfxFont.cpp, line 2824
> WARNING: failed to allocate glyph/text storage for text run!: file
> /home/alex/Downloads/mozilla-central/gfx/thebes/gfxFont.cpp, line 2824
> WARNING: NS_ENSURE_TRUE(mTextRun) failed: file
> /home/alex/Downloads/mozilla-central/layout/generic/nsTextFrameThebes.cpp, line
> 6835

These look normal for a testcase that's creating huge strings and injecting them into the document - eventually you reach a point where we fail to allocate the memory we need to lay out and render them. Apparently the failure was handled safely in this case, though.

I don't know about the remaining warnings, but note that there are quite a number of "warnings" that are normal when running debug builds; they don't necessarily have serious implications.
Comment 20 Alex Miller 2010-11-27 14:59:50 PST
(In reply to comment #19)
> (In reply to comment #18)
> > With your patch I did get these warnings:
> > 
> > WARNING: failed to allocate glyph/text storage for text run!: file
> > /home/alex/Downloads/mozilla-central/gfx/thebes/gfxFont.cpp, line 2824
> > WARNING: failed to allocate glyph/text storage for text run!: file
> > /home/alex/Downloads/mozilla-central/gfx/thebes/gfxFont.cpp, line 2824
> > WARNING: NS_ENSURE_TRUE(mTextRun) failed: file
> > /home/alex/Downloads/mozilla-central/layout/generic/nsTextFrameThebes.cpp, line
> > 6835
> 
> These look normal for a testcase that's creating huge strings and injecting
> them into the document - eventually you reach a point where we fail to allocate
> the memory we need to lay out and render them. Apparently the failure was
> handled safely in this case, though.
> 
> I don't know about the remaining warnings, but note that there are quite a
> number of "warnings" that are normal when running debug builds; they don't
> necessarily have serious implications.

Okay. I just felt compelled to post the results of testing on a trunk nightly.
Comment 21 Alex Miller 2010-11-27 16:52:16 PST
Also, just me thinking out loud here but being able to overwrite pointers does potentially allow for reuse of freed memory, right? (If an attacker is able to say, allocate 1GB of memory, and then free it... Like a freed heap spray thing)

I'm only bringing this up because of this stack frame I normally reproduce:

 xul!BuildTextRuns(class gfxContext * aContext = 41414141, class nsTextFrame *
aForFrame = 41414141, class nsIFrame * aLineContainer = 0x00000003, class
nsLineList_iterator * aForFrameLine = 41414141)+0x32a

I'm not sure if aForFrame or aForFrameLine are pointers, but gfxContext *aContext is.

Also: 

The invalidity of the aContext pointer is almost certainly the reason why on 32-bit Windows 7 there is an immediate crash as a result of displaying a textrun built where aContext is invalid.
Comment 22 Alex Miller 2010-11-27 20:42:11 PST
!exploitable -v seems to return Exploitability Classification: UNKNOWN
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-30 14:10:15 PST
Marking this bug fixed as the one reported issue is believed to be fixed here.
Alex, please open new bugs if you believe you've found other related problems
here!
Comment 24 Jonathan Kew (:jfkthame) 2010-12-23 03:15:59 PST
Created attachment 499492 [details] [diff] [review]
patch for 1.9.1 and 1.9.2 branches

Essentially the same patch as landed on trunk, with context fixed up slightly to make it apply to the older branches; the actual code changes are the same.
Comment 25 christian 2010-12-27 10:24:54 PST
Comment on attachment 499492 [details] [diff] [review]
patch for 1.9.1 and 1.9.2 branches

a=LegNeato for 1.9.2.14 and 1.9.1.17.
Comment 27 Al Billings [:abillings] 2011-01-04 14:39:32 PST
Running the testcase with the shipped 1.9.2.13 on Windows XP, I get the following crash if I wait long enough: http://crash-stats.mozilla.com/report/index/bb2d1793-7c1f-4ca3-8a2a-9ade22110104.

Running it with last night's 1.9.2.14pre build, I get the following crash: http://crash-stats.mozilla.com/report/index/7b0f33ec-b917-4d08-8163-b6d0e2110104

These look exactly the same. 

So, either this isn't fixed or I'm hitting another crash on the way.

Same thing with 1.9.1.

Pre-fix on 1.9.1.16: http://crash-stats.mozilla.com/report/index/595d0626-a36d-4120-8fbd-741f92110104

Post-fix on 1.9.1.17pre: http://crash-stats.mozilla.com/report/index/bp-b2938690-26a1-4459-acc5-0e6ba2110104
Comment 28 Jonathan Kew (:jfkthame) 2011-01-04 14:46:22 PST
(In reply to comment #27)
> Running the testcase with the shipped 1.9.2.13 on Windows XP, I get the
> following crash if I wait long enough:
> http://crash-stats.mozilla.com/report/index/bb2d1793-7c1f-4ca3-8a2a-9ade22110104.
> 
> Running it with last night's 1.9.2.14pre build, I get the following crash:
> http://crash-stats.mozilla.com/report/index/7b0f33ec-b917-4d08-8163-b6d0e2110104
> 
> These look exactly the same. 
> 
> So, either this isn't fixed or I'm hitting another crash on the way.
> 
> Same thing with 1.9.1.
> 
> Pre-fix on 1.9.1.16:
> http://crash-stats.mozilla.com/report/index/595d0626-a36d-4120-8fbd-741f92110104
> 
> Post-fix on 1.9.1.17pre:
> http://crash-stats.mozilla.com/report/index/bp-b2938690-26a1-4459-acc5-0e6ba2110104

These are all straightforward out-of-memory exceptions thrown by the Windows operator new[]. That's a typical (and safe) failure mode for testcases that inject ridiculously huge amounts of text into the document, and does not indicate anything sinister, AFAICT.
Comment 29 Al Billings [:abillings] 2011-01-04 14:47:53 PST
(In reply to comment #28)

> These are all straightforward out-of-memory exceptions thrown by the Windows
> operator new[]. That's a typical (and safe) failure mode for testcases that
> inject ridiculously huge amounts of text into the document, and does not
> indicate anything sinister, AFAICT.

They do make it hard to verify the fix since I'm crashing elsewhere, in the same consistent places, pre and post-fix.
Comment 30 Daniel Veditz [:dveditz] 2011-01-05 10:41:56 PST
abillings: any chance to test on a machine with more memory? Do the assertions change in a debug build?
Comment 31 Al Billings [:abillings] 2011-01-05 11:00:37 PST
I have a Win7 laptop here with 8GB (I believe) of RAM and can build debug on it. I'll try there.
Comment 32 Al Billings [:abillings] 2011-01-05 17:47:17 PST
On my win7 laptop with 8GB of RAM, I get the following.

Pre-fix in 1.9.2.13: http://crash-stats.mozilla.com/report/index/7c7a5b3c-60ec-454b-b6b6-62eda2110105  [@ KERNELBASE.dll@0xb727 ] 

Post-fix in 1.9.2.14pre: http://crash-stats.mozilla.com/report/index/731507d9-86ac-4189-8c74-7567d2110105 [@ KERNELBASE.dll@0xb727 ] 

These are crashes in a different place than the ones I reported above but I don't know if the information is any more helpful.
Comment 33 Al Billings [:abillings] 2011-01-05 18:03:18 PST
For the curious, on the Win7 laptop, 1.9.1.16 pre-fix doesn't crash but will eventually just hang and be unresponsive.
Comment 34 Alex Miller 2011-02-18 15:19:17 PST
(In reply to comment #14)
>  xul!BuildTextRuns(class gfxContext * aContext = 41414141, class nsTextFrame *
> aForFrame = 41414141, class nsIFrame * aLineContainer = 0x00000003, class
> nsLineList_iterator * aForFrameLine = 41414141)+0x32a
> [e:\builds\moz2_slave\win32_build\build\layout\generic\nstextframethebes.cpp @
> 1149]
And just for future reference, the lack of a 0x is a result of using Yahoo mail instead of Thunderbird, which hates copypasting, so that last corrupted frame was misaligned and all so I had to hand-type it, and it was subject to human error.
Comment 35 Alex Miller 2011-03-04 21:35:52 PST
Anything stopping this from being opened to the public?

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