1602497 - Intl.ListFormat can return empty string if input strings are too large

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect, P1)

Description

[André Bargull [:anba]]

Test case:

if (this.addIntlExtras) this.addIntlExtras(Intl);

var s = "a".repeat(0x8F00000); // maybe system-dependent

print("len:", new Intl.ListFormat().format(Array(16).fill(s)).length);

Expected:

• Throws an oom-error

Actual:

• Prints "len: 0".

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:Waldo, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Jeff Walden [:Waldo]]

Compiling and running this with ubsan integer overflow detection enabled hits a signed overflow in ICU, so that's nice.

And on my system, compiling and running this without such detection, just crashes. So that's nice.

ICU, publicly and internally, considers string lengths as int32_t. What's happening here, is there's a string-append happening where the string being appended has length 0x8F00000, and the sum (current incremental length + 0x8F00000) overflows int32_t space. That overflow is what immediately triggers ubsan overflow detection.

But if overflow detection is off, we continue from that first line where overflow occurs, like so:

  int32_t newLength = oldLength + srcLength;

  // Check for append onto ourself
  const UChar* oldArray = getArrayStart();
  if (isBufferWritable() &&
      oldArray < srcChars + srcLength &&
      srcChars < oldArray + oldLength) {
    // Copy into a new UnicodeString and start over
    UnicodeString copy(srcChars, srcLength);
    if (copy.isBogus()) {
      setToBogus();
      return *this;
    }
    return doAppend(copy.getArrayStart(), 0, srcLength);
  }

  // optimize append() onto a large-enough, owned string
  if((newLength <= getCapacity() && isBufferWritable()) ||
      cloneArrayIfNeeded(newLength, getGrowCapacity(newLength))) {
    UChar *newArray = getArrayStart();
    // Do not copy characters when
    //   UChar *buffer=str.getAppendBuffer(...);
    // is followed by
    //   str.append(buffer, length);
    // or
    //   str.appendString(buffer, length)
    // or similar.
    if(srcChars != newArray + oldLength) {
      us_arrayCopy(srcChars, 0, newArray, oldLength, srcLength);

and we'll reach that final line (because newLength as computed will probably end up evaluating to a negative number) and will proceed to write past the end of the string's allocated characters as if sufficient space for the full string actually were allocated. No good.

We may be able to avoid some of the problem if we add our own restrictions on string lengths, to some degree -- if the sum of all input string lengths, plus (N - 1) * 100 or something, looks "close" to INT32_MAX, then report an allocation overflow.

But the very nature of Intl.ListFormat is that you're going to have periodic insertions in the final string of separator sequences of uncertain length, so really this is something we need ICU to do, and do correctly.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Bug 1602497. r=anba

Comment 4

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #2)

And on my system, compiling and running this without such detection, just crashes. So that's nice.

I've never managed to get this to crash, therefore I didn't hide the bug report. Turns out it's necessary to have a large enough free memory amount to trigger the crash. For example it doesn't crash for me with 10GB RAM, but when increasing the RAM to 16GB, it does crash for me, too. :-/

Comment 5

[André Bargull [:anba]]

Reported this to https://bugs.chromium.org/p/chromium/issues/detail?id=1044570, because Chrome does ship Intl.ListFormat by default.

Comment 6

[Jeff Walden [:Waldo]]

So we haven't shipped this yet -- only by opt-in in the shell -- so by our rules we could land this patch whenever.

But if v8 is also affected, a sufficiently lateral-thinking attacker could see our patch and know to try to exploit them.

Maybe you can post once they've publicly landed their patch, then I can land the one here? (sorry for an indeterminate-duration needinfo here)

Comment 7

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #6)

Maybe you can post once they've publicly landed their patch, then I can land the one here?

Sure.

Comment 8

[André Bargull [:anba]]

ICU patch: https://github.com/unicode-org/icu/pull/971
Landed in: https://chromium.googlesource.com/chromium/src.git/+/4d09af54617e24ed9969d6f7abb425786c69b993

Comment 9

[Jeff Walden [:Waldo]]

Bah, I missed comment 8 saying we could land this. Landed just now. (Do recall/note that this is an issue in a feature that is presently disabled by default on all branches, and the similar issue in v8 has been publicly fixed, so landing this sans sec-approval reveals nothing and is fine.)

Comment 10

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/mozilla-central/rev/69d466eda909a2f96ed4cf8a42ec19348cd87242

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 12

[Jeff Walden [:Waldo]]

Filled out the survey. I am...not kind to ICU for its choice of representing lengths using a signed integer type, in my comments. :-D