Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 576447 (CVE-2010-2765)

FRAMESET integer overflow via new operator in nsHTMLFrameSetElement::ParseRowCol()

RESOLVED FIXED

Status

()

Core
DOM
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: reed, Assigned: sicking)

Tracking

(Depends on: 1 bug, 4 keywords)

unspecified
crash, testcase, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .9+, status1.9.2 .9-fixed, blocking1.9.1 .12+, status1.9.1 .12-fixed)

Details

(Whiteboard: [sg:critical?][critsmash:patch])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
status2.0: --- → ?

Comment 1

7 years ago
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?
(Reporter)

Comment 2

7 years ago
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...
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.
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.
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

7 years ago
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 :)
Is the crash caught in winDBG in comment 0 using mozilla-produced builds, or your own windows build using mingw or something else?
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
status1.9.1: ? → wanted
status1.9.2: ? → wanted

Comment 8

7 years ago
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?  :(
blocking1.9.1: needed → ?
blocking1.9.2: needed → ?
status1.9.1: wanted → ?
status1.9.2: wanted → ?

Updated

7 years ago
Depends on: 466445

Comment 9

7 years ago
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

7 years ago
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 :)
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
status1.9.1: ? → wanted
status1.9.2: ? → wanted

Comment 11

7 years ago
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

7 years ago
Is there an ETA on a patch for this bug?

Comment 13

7 years ago
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.
Component: HTML: Parser → DOM
QA Contact: parser → general
I'll take this, can have a patch tomorrow
Assignee: nobody → jonas

Comment 15

7 years ago
Any progress on this patch? I will make time to review it this weekend if that is helpful.
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.
Attachment #457942 - Flags: review?(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...
Attachment #457942 - Flags: review?(jst) → review-

Comment 18

7 years ago
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];

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
Created attachment 458833 [details] [diff] [review]
Patch to fix v2

Doh!
Attachment #457942 - Attachment is obsolete: true
Attachment #458833 - Flags: review?(jst)
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 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
Attachment #458833 - Flags: review?(jst) → review+
blocking1.9.1: needed → .12+
blocking1.9.2: needed → .8+
Attachment #458833 - Flags: approval1.9.2.8?
Attachment #458833 - Flags: approval1.9.1.12?
blocking2.0: ? → final+
status2.0: ? → ---
Fixed on m-c:

http://hg.mozilla.org/mozilla-central/rev/5899956fba1c
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 23

7 years ago
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?
Attachment #458833 - Flags: approval1.9.2.8?
Attachment #458833 - Flags: approval1.9.1.12?
Maybe your PR_STATIC_ASSERT should reference (1<<30) instead of 2^30 (which equals 28)?
Er, actually, I think you're asserting 0 ^ 30, which is 30.
(Reporter)

Comment 26

7 years ago
sicking also landed http://hg.mozilla.org/mozilla-central/rev/6f9698905efb to fix the issue dbaron mentioned.
Created attachment 465530 [details] [diff] [review]
Branch patch
Attachment #465530 - Flags: approval1.9.2.9?
Attachment #465530 - Flags: approval1.9.1.13?
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
Attachment #465530 - Flags: approval1.9.2.9?
Attachment #465530 - Flags: approval1.9.2.9+
Attachment #465530 - Flags: approval1.9.1.13?
Attachment #465530 - Flags: approval1.9.1.13+
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
status1.9.1: wanted → .12-fixed
status1.9.2: wanted → .9-fixed
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.
Keywords: verified1.9.1, verified1.9.2
Alias: CVE-2010-2765
Group: core-security

Updated

7 years ago
Flags: in-testsuite-
Attachment #465530 - Flags: approval1.9.1.14+ → approval1.9.1.12+
You need to log in before you can comment on or make changes to this bug.