422286 - array_slice reads arbitrary memory when array's length is assigned

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was about to write this as a comment in bug 419695, but I realized I should probably file it as a separate security bug:

This is a JS engine bug in array_slice.

In particular, we're calling array_slice on an array with:

ARRAY_DENSE_LENGTH(obj) == 48
array->fslots[JSSLOT_ARRAY_COUNT] == 44
array->fslots[JSSLOT_ARRAY_LENGTH] == 164 (the .length property)

and we happily copy 164 bytes.

This is easily reproducable in xpcshell with:
var a = [];
a.length = 10000000;
a.slice(1);

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, maybe this isn't so bad.  I was thinking it was a way to read arbitrary memory, but you really only have that arbitrary memory in jsvals, which are only going to be readable without crashing 5/8 of the time (low bit 1, or 3 lowest bits 0x6), so it's probably not too useful.

I *think* it's also trunk only.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

That DENSE_LENGTH stuff is all trunk-only.  Taking, patch tonight.

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: Avoid fast-slice behaviour if end overreaches dslots.

We could optimize this in a couple of ways, but I don't think it's worth it.

r? to crowder, but I'll take a + from any JS hacker who's around.

Test fodder:

Array(10000).slice(1)
a = Array(1); a.length = 10000; a.slice(1);
a = Array(1); a.length = 10000; a.slice(-1);

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Adding some review possibles.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Isn't end == ARRAY_DENSE_LENGTH(obj) ok?

Also, doesn't this whole thing initialize nobj->fslots[JSSLOT_ARRAY_COUNT] incorrectly, since it assumes that everything in obj->dslots is not JSVAL_HOLE ?

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #5)
> Isn't end == ARRAY_DENSE_LENGTH(obj) ok?

No: DENSE_LENGTH, like LENGTH, counts one more than the highest available slot.

> Also, doesn't this whole thing initialize nobj->fslots[JSSLOT_ARRAY_COUNT]
> incorrectly, since it assumes that everything in obj->dslots is not JSVAL_HOLE
> ?

Yeah, it does, as does the JSOP_NEWARRAY patch elsewhere, and a few other such optimizations.  I want to write more tests for that before attacking them, so I'd rather leave it to another patch; it's not as high a priority as this crash/sec fix.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (In reply to comment #5)
> > Isn't end == ARRAY_DENSE_LENGTH(obj) ok?
> 
> No: DENSE_LENGTH, like LENGTH, counts one more than the highest available slot.

Yeah, but end is a fencepost too, not a fence rail. Dbaron's point was, I bet, that the test should use <=, not <.

/be

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Isn't end == ARRAY_DENSE_LENGTH(obj) ok?
> > 
> > No: DENSE_LENGTH, like LENGTH, counts one more than the highest available slot.
> 
> Yeah, but end is a fencepost too, not a fence rail. Dbaron's point was, I bet,
> that the test should use <=, not <.

Curses, and apologies!  New patch inc.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: As above, but take fast path all the way up to DENSE_LENGTH

Brendan gets the r?, since he's around!

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 308791 [details] [diff] [review]
As above, but take fast path all the way up to DENSE_LENGTH

Shaver asked me to review to get this in pronto.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #6)
> > Also, doesn't this whole thing initialize nobj->fslots[JSSLOT_ARRAY_COUNT]
> > incorrectly, since it assumes that everything in obj->dslots is not JSVAL_HOLE
> > ?
> 
> Yeah, it does, as does the JSOP_NEWARRAY patch elsewhere,

(which one, mine?)

> and a few other such
> optimizations.  I want to write more tests for that before attacking them, so
> I'd rather leave it to another patch; it's not as high a priority as this
> crash/sec fix.

Followup bug on file?

Sorry I was offline a bit, patch looks good to me too.

/be

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Fixed, vlad's watching the tree in case we were depending on that lack of memory safety or something.

/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.169; previous revision: 3.168
done

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Follow-up hole-audit bug is 422333, fwiw.

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

dbaron: I don't think this is sensitive, since it's trunk-only.  OK with opening it?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yep.  I was going to ask you the same thing...

Comment 16

[Bob Clary [:bc] (inactive)]

verified linux|mac|windows

/cvsroot/mozilla/js/tests/js1_5/Array/regress-422286.js,v  <--  regress-422286.js
initial revision: 1.1