1124778 - Array concat integer overflow

Status: RESOLVED DUPLICATE of bug 688852

(Core :: JavaScript Engine, defect)

Description

[SkyLined]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

Array concat can overflow the 32-bit unsigned integer in which the length of the array is stored. I haven't found a way to exploit this, but I am marking this as a security issue just in case there is something I haven't thought of.

Repro:

a = new Array(0xFFFFFFFF);
console.log(b.length.toString(16));
try {a.push(1);} catch (e) { console.log("Can't push");}
try {b=a.concat(1,2,3);} catch (e) { console.log("Can't concat");}
console.log(b.length.toString(16));
console.log(b[0]);


Actual results:

"0"
"Can't push"
"2"
2


Expected results:

"FFFFFFFF"
"Can't push"
"Can't concat"
"FFFFFFFF"
undefined

Comment 1

[SkyLined]

Doh! Code should have read:

a = new Array(0xFFFFFFFF);
console.log(a.length.toString(16)); /// <<< NOT "a" not "b" as earlier.
try {a.push(1);} catch (e) { console.log("Can't push");}
try {b=a.concat(1,2,3);} catch (e) { console.log("Can't concat");}
console.log(b.length.toString(16));
console.log(b[0]);

Actual result should be:

"ffffffff"
"Can't push"
"2"
2

Sorry for the confusion.

Comment 2

[Andrew McCreight [:mccr8]]

Steve, could you look at this?  Thanks.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Check array lengths in array_concat

Yeah, we're just doing completely the wrong thing in array_concat.

Unfortunately, I'm very uncertain as to what the right way to handle this is at the present time. This patch does a minimal change that tries to follow the ES6 spec, only using 2^32-1 in place of ES6's 2^53-1. (Our arrays use 2^32-1 everywhere, so I'm not going to dive into that for this bug.)

array_push currently does something different, where it creates named properties on int32 overflow but then throws a TypeError too. Apparently, that's what ES5 specified.

If Waldo gives me f+ for this, I'll add a test.

I don't see any way this could be exploitable, so I think this bug can be opened up. There are no out of bounds accesses going on, just artificially low lengths.