656254 - String CSSValues of length greater than 0xffff are truncated to 0xffff chars

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ian Beer]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: FF 4.0.1

In layout/style/nsCSSValue.cpp

    601 already_AddRefed<nsStringBuffer>
    602 nsCSSValue::BufferFromString(const nsString& aValue)
    603 {
    604   nsStringBuffer* buffer = nsStringBuffer::FromString(aValue);
    605   if (buffer) {
    606     buffer->AddRef();
    607     return buffer;
    608   }
    609   
    610   PRUnichar length = aValue.Length();
    611 
    612   // NOTE: Alloc prouduces a new, already-addref'd (refcnt = 1) buffer.
    613   buffer = nsStringBuffer::Alloc((length + 1) * sizeof(PRUnichar));
    614   if (NS_LIKELY(buffer != 0)) {
    615     PRUnichar* data = static_cast<PRUnichar*>(buffer->Data());
    616     nsCharTraits<PRUnichar>::copy(data, aValue.get(), length);
    617     // Null-terminate.
    618     data[length] = 0;
    619   }
    620 
    621   return buffer;
    622 }

Line 610:

    PRUnichar length = aValue.Length();

If aValue.length == UINT16_MAX -> (length + 1) overflows, it's a stringBuffer, but gets its data element cast to a PRUinchar*.

This may well not be exploitable, as getting to this specific block requires nsStringBuffer::FromString(aValue) to return null. It certainly does return null sometimes, but I don't know enough about the nsString internals to understand when this happens.

It seems to rely on whether the F_SHARED flag is set, but I can't determine when this is, and if there is a code path where the flag is set correctly for long strings.

I've marked as security sensitive but as I say, I have no PoC and it might actually be safe.

The fix would be to change PRUnichar to a size_t type.

(This may or may not be related to BUG 615977?)

Reproducible: Couldn't Reproduce

Comment 1

[Boris Zbarsky [:bzbarsky]]

> If aValue.length == UINT16_MAX -> (length + 1) overflows

"1" is of type int.  When adding an unsigned short to an int, the unsigned short is promoted to int and the addition is done on ints.

So UINT16_MAX + 1 == 65536 of type int.

So as far as I can tell there is no issue here apart from strings that are longer than 0xffff chars getting truncated to 0xffff chars.

Comment 2

[Ian Beer]

My bad, you're completely right.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Resummarizing to cover the actual remaining issue.

Comment 4

[Mats Palmgren (inactive)]

https://searchfox.org/mozilla-central/source/layout/style/nsCSSValue.cpp#745
It appears we use nsString::size_type and size_t now, so I think
this bug was fixed already (by bug 615112).