576447 - (CVE-2010-2765) FRAMESET integer overflow via new operator in nsHTMLFrameSetElement::ParseRowCol()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Reed Loden [:reed]]

Attachment: PoC

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

[Boris Zbarsky [:bzbarsky]]

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

[Reed Loden [:reed]]

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

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

[Zack Weinberg (:zwol)]

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

[Zack Weinberg (:zwol)]

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

[Chris R]

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

[Daniel Veditz [:dveditz]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Chris R]

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

[Chris R]

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

[Chris R]

Is there an ETA on a patch for this bug?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll take this, can have a patch tomorrow

Comment 15

[Chris R]

Any progress on this patch? I will make time to review it this weekend if that is helpful.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Johnny Stenback (:jst)]

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

[Chris R]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix v2

Doh!

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

[Johnny Stenback (:jst)]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Fixed on m-c:

http://hg.mozilla.org/mozilla-central/rev/5899956fba1c

Comment 23

[Reed Loden [:reed]]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe your PR_STATIC_ASSERT should reference (1<<30) instead of 2^30 (which equals 28)?

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, actually, I think you're asserting 0 ^ 30, which is 30.

Comment 26

[Reed Loden [:reed]]

sicking also landed http://hg.mozilla.org/mozilla-central/rev/6f9698905efb to fix the issue dbaron mentioned.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Branch patch

Comment 28

[Daniel Veditz [:dveditz]]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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

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.