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

RESOLVED FIXED in Firefox 44, Firefox OS master

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
10 months ago

People

(Reporter: Waldo, Assigned: arai)

Tracking

({sec-audit})

Trunk
mozilla44
sec-audit
Points:
---

Firefox Tracking Flags

(firefox41 wontfix, firefox42- wontfix, firefox43+ wontfix, firefox44+ fixed, firefox-esr38- wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 affected, b2g-v2.2r affected, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.5+][adv-main44-])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
Summary: Array literal element-initialization after a spreadcall looks like it could hit int32_t overflow → Array literal element-initialization after a spread looks like it could hit int32_t overflow
(Assignee)

Comment 2

3 years ago
filed as bug 1201869.
Keywords: sec-high
Group: core-security
Hi Naveed, is there anyone who might have some spare cycles to take a look at this?
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 4

2 years ago
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
Jeff can you take a look at :arai's attempt to realize this bug? Is there still an issue here?
Flags: needinfo?(nihsanullah) → needinfo?(jwalden+bmo)
(Reporter)

Comment 6

2 years ago
(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?
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 7

2 years ago
Created attachment 8676802 [details] [diff] [review]
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?
Assignee: nobody → arai.unmht
Attachment #8676802 - Flags: review?(jwalden+bmo)
(Reporter)

Updated

2 years ago
Attachment #8676802 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 8

2 years ago
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.
Attachment #8676802 - Flags: sec-approval?
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → wontfix
status-b2g-v2.1S: --- → wontfix
status-b2g-v2.2: --- → affected
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → affected
status-firefox41: --- → wontfix
status-firefox42: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → affected
tracking-firefox42: --- → ?
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
tracking-firefox-esr38: --- → ?
This is too late for 42 as we are going to build the RC today.
status-firefox42: affected → wontfix
tracking-firefox42: ? → -
tracking-firefox43: ? → +
tracking-firefox44: ? → +
Attachment #8676802 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 10

2 years ago
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
https://hg.mozilla.org/mozilla-central/rev/12a1cd0cb48a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-b2g-master: affected → fixed
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Comment 12

2 years ago
Does this need backport?
Group: javascript-core-security → core-security-release
Whiteboard: [b2g-adv-main2.5+]
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.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 14

2 years ago
IMO, this can be wontfix.
Flags: needinfo?(arai.unmht)
OK, marking wontfix, thanks arai.
status-firefox43: affected → wontfix
Per comments above this is not exploitable
Group: core-security-release
status-firefox-esr38: affected → wontfix
tracking-firefox-esr38: ? → -
Keywords: sec-high → sec-audit
Whiteboard: [b2g-adv-main2.5+] → [b2g-adv-main2.5+][adv-main44-]
(Reporter)

Updated

10 months ago
Flags: needinfo?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.