Last Comment Bug 576447 - (CVE-2010-2765) FRAMESET integer overflow via new operator in nsHTMLFrameSetElement::ParseRowCol()
(CVE-2010-2765)
: FRAMESET integer overflow via new operator in nsHTMLFrameSetElement::ParseRow...
Status: RESOLVED FIXED
[sg:critical?][critsmash:patch]
: crash, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
-- critical (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 466445
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-01 22:00 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2010-09-28 23:57 PDT (History)
20 users (show)
jruderman: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.9+
.9-fixed
.12+
.12-fixed


Attachments
PoC (3.39 KB, text/html)
2010-07-01 22:00 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details
Patch to fix (1020 bytes, patch)
2010-07-16 12:52 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
jst: review-
Details | Diff | Splinter Review
Patch to fix v2 (1020 bytes, patch)
2010-07-20 16:41 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
jst: review+
Details | Diff | Splinter Review
Branch patch (1.09 KB, patch)
2010-08-12 19:54 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
dveditz: approval1.9.2.9+
dveditz: approval1.9.1.12+
Details | Diff | Splinter Review

Description User image Reed Loden [:reed] (use needinfo?) 2010-07-01 22:00:17 PDT
Created attachment 455624 [details]
PoC

Chris@Matasano.com reported the following integer overflow vulnerability to security@:

==============================================================================

FRAMESET integer overflow in new operator on line 332 of nsHTMLFrameSetElement.cpp

A FRAMESET tag looks like this:

  < FRAMESET cols="%20,%80" >

When a call to setAttribute occurs on the FRAMESET element we reach the following function on line 332 of nsHTMLFrameSetElement.cpp:

nsresult
nsHTMLFrameSetElement::ParseRowCol(const nsAString & aValue,
                                   PRInt32& aNumSpecs,
                                   nsFramesetSpec** aSpecs)
{
...
  // Count the commas 
  PRInt32 commaX = spec.FindChar(sComma);
  PRInt32 count = 1;
  while (commaX != kNotFound) {
    count++;
    commaX = spec.FindChar(sComma, commaX + 1);
  }

  nsFramesetSpec* specs = new nsFramesetSpec[count];
...


nsHTMLFrameSetElement::ParseRowCol counts the number of commas in the tag and then allocates a heap buffer using that number with the new operator. The size of the nsFramesetSpec structure is 8 bytes, so we need 536870912 or more commas to trigger the integer overflow. When this happens, too small of an allocation occurs and the proceeding heap memory is overwritten in the for loop on line 347.
Here is the crash dump from WinDBG:

0:000> r
eax=ffffffff ebx=00000000 ecx=00000000 edx=0000002c esi=001f0004 edi=00000000
eip=5e560c97 esp=001ee984 ebp=001eea78 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010246
xul!nsHTMLFrameSetElement::ParseRowCol+0xaf:
5e560c97 8366fc00        and     dword ptr [esi-4],0  ds:0023:001f0000=00905a4d


There is one mitigating factor, as you will see when you push the button below. The script requires such an enormous allocation of commas that FF will ask the user several times if the script should be allowed to continue or not. Exploiting this bug in a 32bit Firefox process might be difficult as the commas take up most of the available heap memory. But a 64bit process is a different story.

Fixing this vulnerability is relatively straight forward. Define a max number of commas and check count within the while loop to see if it has reached that threshold. If it has, exit the while loop. Alternatively checking for integer overflow before the new operator is called is also suggested.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-01 22:08:11 PDT
Wait a second.  Shouldn't operator new[] handle this correctly instead of randomly allocating the wrong amount of memory?  Fixing this at |new[]| callsites seems like the wrong approach; every single new[] callsite could be thus vulnerable...

Is this a bug in our mozalloc new, or in the stdlib one?
Comment 2 User image Reed Loden [:reed] (use needinfo?) 2010-07-01 22:23:30 PDT
Oh, I forgot to mention that Chris reported this against Firefox 3.6.6. Does 3.6.x have the new |mozalloc new|, or is that on trunk only? It's possible trunk isn't vulnerable to this...
Comment 3 User image Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-07-01 22:38:09 PDT
By the time we get to ::operator new[](size_t size), it's too late to (reliably) check for overflow in the computation of |size|, AFAIK.  For starters the code would be architecture-specific, we'd have to read the manuals very carefully to ensure the overflow/carry bit is preserved across the asm the compiler generates for the call to ::operator new(), and additionally we might detect false positives depending on previous instructions.  Even if we could get something working on x86 for simple cases like |new[x*y]|, I think it's basically impossible to defend against false negatives like
  x = y*z //overflow
  y += 0  // clear overflow flag
  new[x]
So again AFAIK we're pretty screwed here at the ::operator new[]() level :(.

A static analysis for possible overflow at allocation sites could help, although I seriously doubt a useful, sound analysis is possible.
Comment 4 User image Zack Weinberg (:zwol) 2010-07-02 09:50:39 PDT
If I understand the original bug report correctly, this is actually a multiplication overflow *inside* the code generated for 'new T[n]'.  It's true that by the time we get to ::operator new[](size_t) it is too late to detect the problem.  GCC should, but does not, detect overflow at the call site and throw std::bad_alloc; I'm told that MSVC does detect the overflow. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19351 (ignore all comments made by Andrew Pinski).

From our side of things, I would suggest avoiding direct use of new[] in favor of TArray or the oft-murmured std::vector clone, which would allow us to do a manual check in one place.
Comment 5 User image Zack Weinberg (:zwol) 2010-07-02 09:55:03 PDT
Correction: per C++0x the compiler is required to throw a new exception type, std::bad_array_new_length - this strikes me as unnecessary multiplication of entities, but that's standards land for you.  At least it's a subclass of bad_alloc.  And at least the compiler *is* required to throw *something* now; I think it was undefined behavior in C++98.
Comment 6 User image Chris R 2010-07-02 09:57:44 PDT
FWIW, on Win32, new[] will throw an exception when overflow occurs. gnu/stdlib will not. In this particular case you will want to declare 'count' as unsigned and implement a check for overflow before calling the new[] operator:

...
+  PRUInt32 count = 1;

 while (commaX != kNotFound) {
   count++;
   commaX = spec.FindChar(sComma, commaX + 1);
 }

+  if(count >= UINT_MAX /  sizeof(nsFramesetSpec))
+  {
+    error()
+  }

 nsFramesetSpec* specs = new nsFramesetSpec[count];
...

All of this assumes you actually keep the new[] operator in favor of some other mozilla wrapped allocation routine. I hope that is helpful :)
Comment 7 User image Daniel Veditz [:dveditz] 2010-07-02 10:45:30 PDT
Is the crash caught in winDBG in comment 0 using mozilla-produced builds, or your own windows build using mingw or something else?
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-02 10:48:00 PDT
Ugh.  I hate compiler braindamage...  So we knew about this issue as of bug 466445 (not for this callsite but in general)? 

In the short run, I think we should forbid the use of new[] in favor of a macro or inline function or something that does the check and then calls new[], or indeed the use of nsTArray something like this:

  nsTArray<foo> x(length);
  if (x.Capacity() != length) {
    // failed
  }

Note that nsTArray will NS_ERROR on too-big length; is that desirable?

And I guess we should audit all new[] callsites.  I hope there aren't too many.  Anyone have time to do that?

Have I mentioned compiler braindamage?  :(
Comment 9 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-02 11:35:41 PDT
Pending an answer to comment 7, btw, how reasonable is it to compile our builds with a gcc with taras' patch and make it very public that this is necessary if people want a non-broken build?
Comment 10 User image Chris R 2010-07-02 11:45:19 PDT
I used the build found on Mozilla's website. Version 3.6.6 on Windows 7 32-bit. The WinDBG output has symbols because I use the Mozilla symbol server.

I took a brief look at some of the other new[] calls, nothing obvious jumps out, but I just don't have enough free time to investigate each one. Matasano would love to do this for Mozilla (again), but that's another thread :)
Comment 11 User image Chris R 2010-07-03 19:16:12 PDT
I would also like to add that the original POC, while correct contains an extra call to setAttribute(). The first one is enough to reach the vulnerability. Sorry for any confusion!
Comment 12 User image Chris R 2010-07-08 12:39:42 PDT
Is there an ETA on a patch for this bug?
Comment 13 User image Boris Zbarsky [:bz] (still a bit busy) 2010-07-08 15:58:12 PDT
Not yet... the question seems to be whether we can just fix the whole class of bugs or whether we need to hack around this compiler breakage in the meantime.
Comment 14 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-13 01:15:18 PDT
I'll take this, can have a patch tomorrow
Comment 15 User image Chris R 2010-07-15 20:29:46 PDT
Any progress on this patch? I will make time to review it this weekend if that is helpful.
Comment 16 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-16 12:52:35 PDT
Created attachment 457942 [details] [diff] [review]
Patch to fix

This seemed like a simple way to fix this. I guess we could put the 100000 into a #define, but it doesn't seem like something we'd ever need to change. That many frames makes no sense anyway.
Comment 17 User image Johnny Stenback (:jst, jst@mozilla.com) 2010-07-16 13:38:23 PDT
Comment on attachment 457942 [details] [diff] [review]
Patch to fix

+  // Count the commas. Don't count more than 100000 commas to avoid integer
+  // overflow in the allocation below.
   PRInt32 commaX = spec.FindChar(sComma);
   PRInt32 count = 1;
-  while (commaX != kNotFound) {
+  while (commaX != kNotFound && comma < 100000) {

Um, you want count < 100000 here, not "comma"?

I'm assuming there's no sane way to write tests for this, or given this limit, doesn't seem like we can really hold 100000 frames in memory on a 32-bit system...
Comment 18 User image Chris R 2010-07-16 17:44:13 PDT
So the patch looks sane but there could be a problem if the nsFramesetSpec structure ever gets bigger. Of course the structure would have to grow to a very large size for an integer overflow to occur at just 100,000 allocations (something like 42000+ bytes). I would also check for integer overflow before the allocation as I posted in my previous patch:

+   if(count >= UINT_MAX /  sizeof(nsFramesetSpec))
+   {
+     error()
+   }

    nsFramesetSpec* specs = new nsFramesetSpec[count];
Comment 19 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-20 16:41:25 PDT
Created attachment 458833 [details] [diff] [review]
Patch to fix v2

Doh!
Comment 20 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-20 17:09:09 PDT
regarding comment 18, I was thinking about that when I wrote the patch. However nsFramesetSpec would have to be in the order of 40000 bytes large for that to happen. It's currently 8 bytes. It seems very unlikely to happen.

If people really think it's worth it, I can add a static assertion.
Comment 21 User image Johnny Stenback (:jst, jst@mozilla.com) 2010-07-20 17:22:45 PDT
Comment on attachment 458833 [details] [diff] [review]
Patch to fix v2

A static assert would be easy, but I'm fine either way here. r=jst
Comment 22 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-22 15:49:37 PDT
Fixed on m-c:

http://hg.mozilla.org/mozilla-central/rev/5899956fba1c
Comment 23 User image Reed Loden [:reed] (use needinfo?) 2010-07-22 17:41:07 PDT
Comment on attachment 458833 [details] [diff] [review]
Patch to fix v2

Can we actually get the patch that landed attached to the bug and approval requested on it, please?
Comment 24 User image David Baron :dbaron: ⌚️UTC-8 2010-07-22 17:47:50 PDT
Maybe your PR_STATIC_ASSERT should reference (1<<30) instead of 2^30 (which equals 28)?
Comment 25 User image David Baron :dbaron: ⌚️UTC-8 2010-07-22 17:52:45 PDT
Er, actually, I think you're asserting 0 ^ 30, which is 30.
Comment 26 User image Reed Loden [:reed] (use needinfo?) 2010-07-22 18:47:10 PDT
sicking also landed http://hg.mozilla.org/mozilla-central/rev/6f9698905efb to fix the issue dbaron mentioned.
Comment 27 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-12 19:54:24 PDT
Created attachment 465530 [details] [diff] [review]
Branch patch
Comment 28 User image Daniel Veditz [:dveditz] 2010-08-12 20:00:56 PDT
Comment on attachment 465530 [details] [diff] [review]
Branch patch

Approved for 1.9.2.9 and 1.9.1.12, a=dveditz for release-drivers
Comment 29 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-08-12 20:03:09 PDT
Fixed on branches:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/184125a9b1f8
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/10046599e0ca
Comment 30 User image Al Billings [:abillings] 2010-08-20 17:29:08 PDT
Verified for 1.9.2.9 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9pre) Gecko/20100818 Namoroka/3.6.9pre ( .NET CLR 3.5.30729) and the PoC. 

Verified for 1.9.1.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.12pre) Gecko/20100818 Shiretoko/3.5.12pre ( .NET CLR 3.5.30729) and the PoC.

No more crashes.

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