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

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
--
critical
7 years ago
7 years ago

People

(Reporter: Ian Beer, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

7 years ago
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
> 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.
Depends on: 615112
(Reporter)

Comment 2

7 years ago
My bad, you're completely right.
Resummarizing to cover the actual remaining issue.
Group: core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Possible integer truncation in nsCSSValue.cpp → String CSSValues of length greater than 0xffff are truncated to 0xffff chars
You need to log in before you can comment on or make changes to this bug.