1201783 - Array literal element-initialization after a spread looks like it could hit int32_t overflow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

While reading bug 1199345, I saw JSOP_INITELEM_INC -- for adding a single element to an array, after a spread has occurred -- assumes that the index passed into it is int32_t.  It further assumes that adding 1 to that index, doesn't overflow positive int32_t space.

Is that really safe?  Seems like we could spread a bunch of elements and exceed that limit without too much trouble (other than it taking awhile).  I don't see anything enforcing an upper limit that's short of INT32_MAX.

I'm not sure what the consequences are of screwing this up, but DoSetElemFallback at least seems to make consequential decisions based on whether the index it receives is negative or not -- including concerning the array's old capacity and initialized length, whether to add out-of-bounds stubs, whether to remove existing stubs, &c.  I'm going to play it safe and assume overflow here is Bad until proven otherwise.

Comment 1

[Tooru Fujisawa [:arai]]

won't this catch that case? (I haven't reached it tho, I'll have to run js shell all night :P

https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter-inl.h#623
>     if (op == JSOP_INITELEM_INC && index == INT32_MAX) {
>         JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_SPREAD_TOO_LARGE);
>         return false;


(maybe off topic) I'm hitting strange behavior in following code while testing this:

  js> let a = []; a.length = 2**28 -1; let b = [...a]; b.length
  js>

There, InitArrayElemOperation seems to fail with some reason, with index around 0xf9ffffe, but no exception is thrown.
I'll investigate it, and will file separated bug later.

Comment 2

[Tooru Fujisawa [:arai]]

filed as bug 1201869.

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Naveed, is there anyone who might have some spare cycles to take a look at this?

Comment 4

[Tooru Fujisawa [:arai]]

So far, I cannot find any case that will hit int32_t overflow.

Array literal's length is limited to 2**28 in parser, so more than 2**30 elements should be spreaded, but no success.

js> a = []; a.length = 2**28; b = [...a]; b.length
268435456
js> a = []; a.length = 2**29; b = [...a]; b.length
uncaught exception: out of memory
js> a = []; a.length = 2**28; b = [...a, ...a]; b.length
uncaught exception: out of memory

js> a = []; for (i = 0; i < 2**28; i++) { a[i] = i; } b = [...a]; b.length
268435456
js> a = []; for (i = 0; i < 2**29; i++) { a[i] = i; } b = [...a]; b.length
uncaught exception: out of memory
js> a = []; for (i = 0; i < 2**28; i++) { a[i] = i; } b = [...a, ...a]; b.length
uncaught exception: out of memory

Comment 5

[Naveed Ihsanullah [:naveed]]

Jeff can you take a look at :arai's attempt to realize this bug? Is there still an issue here?

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Tooru Fujisawa [:arai] from comment #1)
> won't this catch that case? (I haven't reached it tho, I'll have to run js
> shell all night :P
> 
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Interpreter-inl.
> h#623
> >     if (op == JSOP_INITELEM_INC && index == INT32_MAX) {
> >         JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_SPREAD_TOO_LARGE);
> >         return false;

That handles non-holes, but in the hole case, we have

    if (val.isMagic(JS_ELEMENTS_HOLE)) {
        if (op == JSOP_INITELEM_INC) {
            if (!SetLengthProperty(cx, obj, index + 1))

that hits before then, don't we?  Which isn't worrisome security-sensically because SetLengthProperty takes double and so will do something sane-ish, but it does seem not quite right.  Or am I *still* missing something here?

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Throw range error for too long array spread before updating length.

If the condition could be hit, this will fix.  But I think this is not reproducible with current code.  maybe after extending maximum array length in future?

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8676802 [details] [diff] [review]
Throw range error for too long array spread before updating length.

Is this still sec-high?

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
I cannot think any way to exploit, or even hit it.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
All branches have same code.

> If not all supported branches, which bug introduced the flaw?
-

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same patch should be appricable.

> How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.  Almost no effect.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

This is too late for 42 as we are going to build the RC today.

Comment 10

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12a1cd0cb48a60598857fe0aa61cbe9e58fe06da
Bug 1201783 - Throw range error for too long array spread before updating length. r=Waldo, a=abillings

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12a1cd0cb48a

Comment 12

[Tooru Fujisawa [:arai]]

Does this need backport?

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Do you intend to uplift this to 43?  This is rated sec-high. But all the comments above indicate doubt as to whether it is sec-high or exploitable at all. 

We should either uplift this now, or wontfix it for 43.

Comment 14

[Tooru Fujisawa [:arai]]

IMO, this can be wontfix.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, marking wontfix, thanks arai.

Comment 16

[Daniel Veditz [:dveditz]]

Per comments above this is not exploitable