615112 - Potential integer overflow leads to incorrect buffer size in nsCSSValue

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Alex Miller]

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-4, no longer working on Mozilla)]

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]

(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-4, no longer working on Mozilla)]

(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]

Sorry for being incoherent, I just couldn't figure out how to explain exactly what I was saying about UTF-16.

Comment 5

[[e-mail]]

(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]

(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 (inactive)]

(filed bug 615147 for the problem Neil spotted in comment 5)

Comment 8

[Alex Miller]

(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]]

(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 [:bzbarsky]]

> 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 [:bzbarsky]]

And yes, the PRUnichar there is accidental.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 - ex-MoCo]]

Is this bug still an issue in any way?

Comment 16

[Mats Palmgren (inactive)]

Attachment: fix

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 620157 [details] [diff] [review]
fix

r=me

Comment 18

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0db59ddf1920

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0db59ddf1920