Last Comment Bug 615112 - Potential integer overflow leads to incorrect buffer size in nsCSSValue
: Potential integer overflow leads to incorrect buffer size in nsCSSValue
Status: RESOLVED FIXED
[sg:nse][security-sensitive until bug...
: sec-other
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86 All
: -- critical (vote)
: mozilla15
Assigned To: Mats Palmgren (:mats)
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks: 656254
  Show dependency treegraph
 
Reported: 2010-11-28 09:41 PST by Alex Miller
Modified: 2012-05-04 10:56 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (921 bytes, patch)
2012-05-01 18:14 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review

Description Alex Miller 2010-11-28 09:41:05 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.10 (maverick) Firefox/3.6.12
Build Identifier: 

A possible integer overflow in nsCSSValue::BufferFromString leads to incorrect buffer size calculation, resulting in a buffer overflow.

Reproducible: Always




The buffer size is calculated in PRUnichar length.

The integer overflow condition can really only occur on machines with lots of memory, as even if the string provided is in UTF-32 (4 byte characters), it would still require a string 1073741824 characters long to trigger this condition.

This should fixed by adding an integer overflow check.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-11-28 09:55:29 PST
There actually isn't a problem right now because |length| is a (16-bit) PRUnichar (unintentionally, I think), which means we'll never overflow a 32-bit value by doubling it.

Probably we should do something like this, though:

-  
-  PRUnichar length = aValue.Length();
-  buffer = nsStringBuffer::Alloc((length + 1) * sizeof(PRUnichar));
+
+  nsString::size_type length = aValue.Length();
+  size_t storage = (length + 1) * sizeof(PRUnichar);
+  if (NS_UNLIKELY(storage / sizeof(PRUnichar) < length)) {
+    buffer = nsnull;
+  } else {
+    buffer = nsStringBuffer::Alloc();
+  }
Comment 2 Alex Miller 2010-11-28 10:06:58 PST
(In reply to comment #1)
> There actually isn't a problem right now because |length| is a (16-bit)
> PRUnichar (unintentionally, I think), which means we'll never overflow a 32-bit
> value by doubling it.
> 
> Probably we should do something like this, though:
> 
> -  
> -  PRUnichar length = aValue.Length();
> -  buffer = nsStringBuffer::Alloc((length + 1) * sizeof(PRUnichar));
> +
> +  nsString::size_type length = aValue.Length();
> +  size_t storage = (length + 1) * sizeof(PRUnichar);
> +  if (NS_UNLIKELY(storage / sizeof(PRUnichar) < length)) {
> +    buffer = nsnull;
> +  } else {
> +    buffer = nsStringBuffer::Alloc();
> +  }

Hmm... But if it's 16-bit does that mean that UTF-16 could still trigger the issue? Or are UTF-16 characters still treated with 32-bit operations?

When nsStringBuffer::Alloc() is called without parameters it still allocates the correct amount of memory, right?
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-11-28 10:21:26 PST
(In reply to comment #2)
> Hmm... But if it's 16-bit does that mean that UTF-16 could still trigger the
> issue? Or are UTF-16 characters still treated with 32-bit operations?

I'm not sure what you mean.

> When nsStringBuffer::Alloc() is called without parameters it still allocates
> the correct amount of memory, right?

Sorry, I meant to write nsStringBuffer::Alloc(storage);
Comment 4 Alex Miller 2010-11-28 10:27:11 PST
Sorry for being incoherent, I just couldn't figure out how to explain exactly what I was saying about UTF-16.
Comment 5 neil@parkwaycc.co.uk 2010-11-28 11:34:46 PST
(In reply to comment #1)
> Probably we should do something like this, though:
> 
> -  
> -  PRUnichar length = aValue.Length();
> -  buffer = nsStringBuffer::Alloc((length + 1) * sizeof(PRUnichar));
> +
> +  nsString::size_type length = aValue.Length();
> +  size_t storage = (length + 1) * sizeof(PRUnichar);
> +  if (NS_UNLIKELY(storage / sizeof(PRUnichar) < length)) {
> +    buffer = nsnull;
> +  } else {
> +    buffer = nsStringBuffer::Alloc();
> +  }
If the above is a problem then this line from MutatePrep is too:
size_type storageSize = (capacity + 1) * sizeof(char_type);
Comment 6 Alex Miller 2010-11-28 11:43:00 PST
(In reply to comment #5)
> (In reply to comment #1)
> > Probably we should do something like this, though:
> > 
> > -  
> > -  PRUnichar length = aValue.Length();
> > -  buffer = nsStringBuffer::Alloc((length + 1) * sizeof(PRUnichar));
> > +
> > +  nsString::size_type length = aValue.Length();
> > +  size_t storage = (length + 1) * sizeof(PRUnichar);
> > +  if (NS_UNLIKELY(storage / sizeof(PRUnichar) < length)) {
> > +    buffer = nsnull;
> > +  } else {
> > +    buffer = nsStringBuffer::Alloc();
> > +  }
> If the above is a problem then this line from MutatePrep is too:
> size_type storageSize = (capacity + 1) * sizeof(char_type);

That was posted as a recommendation, not necessarily the area of the issue.
Comment 7 Mats Palmgren (:mats) 2010-11-28 22:32:01 PST
(filed bug 615147 for the problem Neil spotted in comment 5)
Comment 8 Alex Miller 2010-11-29 11:09:51 PST
(In reply to comment #7)
> (filed bug 615147 for the problem Neil spotted in comment 5)
Can I be CC'd to that?
Comment 9 Daniel Veditz [:dveditz] 2010-11-30 13:04:41 PST
(In reply to comment #1)
> There actually isn't a problem right now because |length| is a (16-bit)
> PRUnichar (unintentionally, I think), which means we'll never overflow a 32-bit
> value by doubling it.
> 
> -  PRUnichar length = aValue.Length();
> -  buffer = nsStringBuffer::Alloc((length + 1) * sizeof(PRUnichar));

Is there something in the parser that constrains CSS values to 64K? aValue.Length() returns PRUint32 so this truncates. Why are we not seeing a compiler warning about truncation/dataloss?

In this case the data is handled safely: the amount allocated and copied is based on the same possibly-truncated length. Could have been disastrous though.

I don't mind truncating insanely long values. Would be better to truncate (or reject) at the earliest stage though.
Comment 10 Boris Zbarsky [:bz] 2010-11-30 13:37:27 PST
> Is there something in the parser that constrains CSS values to 64K?

No.  Further, some of these values don't even come from the CSS parser (e.g. <font family="..."> and such).
Comment 11 Boris Zbarsky [:bz] 2010-11-30 13:37:40 PST
And yes, the PRUnichar there is accidental.
Comment 12 Boris Zbarsky [:bz] 2010-11-30 13:39:25 PST
On the other hand, note that I suspect aValue.Length() is the length of a string, so can't be bigger than the length of that string buffer...  And if it's an XPCOM string, then we earlier allocated (length+1)*sizeof(PRUnichar) for that buffer already, so it's overflow-safe.
Comment 15 Al Billings [:abillings] 2012-05-01 17:44:41 PDT
Is this bug still an issue in any way?
Comment 16 Mats Palmgren (:mats) 2012-05-01 18:14:31 PDT
Created attachment 620157 [details] [diff] [review]
fix
Comment 17 Boris Zbarsky [:bz] 2012-05-01 18:16:48 PDT
Comment on attachment 620157 [details] [diff] [review]
fix

r=me
Comment 19 Ed Morley [:emorley] 2012-05-04 10:56:44 PDT
https://hg.mozilla.org/mozilla-central/rev/0db59ddf1920

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