157652 - Crash, possible heap corruption in JS Array.prototype.sort

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

Submitter name:                zen-parse
   Submitter email address:       [e-mail]
   Acknowledgement checkbox:      on
   Product:                       Netscape 6.x
   Operating system:              Unix: x86 Linux
   OS version:                    Redhat 7.0
   Issue summary:                 Javascript Array object heap corruption

Issue details:
Heap corruption with Javascript: the sort 
method of the Array object.


Should be a check that "(size_t) len * 
sizeof(jsval)" won't overflow the int
at 
http://lxr.mozilla.org/mozilla/source/js/src/jsarray.c#824

824     vec = (jsval *) JS_malloc(cx, 
(size_t) len * sizeof(jsval));


These lines of javascript will cause a crash.

...
 a=Array(1073741824); // 0x40000000 (or 
0x100000000/4)
 a.sort();
...

This example will just die trying to access 
unallocated memory, and is
intended merely to show there is a problem.

However, it is possible to control, to a 
degree, the contents of memory
being accessed by this code in such a way as 
to execute arbitrary
instructions.


Tested on:
 Netscape 4.77
 Netscape 6.2.3
(Also Internet Explorer (Version 
6.0.2600.0000.xpclnt_qfe.010827-1803
 with Update Versions: q319182,Q321232) dies 
with an access violation,
 so it seems to be a common construct in the 
handling.)



This form was submitted from
http://help.netscape.com/forms/bug-security.html?cp=bbpctr
with Mozilla/4.76 (Linux 2.2.19-7.0.16 i686; U) Opera 6.0  [en].

Comment 1

[Mitchell Stoltz (not reading bugmail)]

I'm not entirely sure this is exploitable, but I have reproduced the crash in a
recent trunk debug build. Please let me know ASAP if you think this can be
exploited to run arbitrary code, for example by overwriting a return address.

Comment 2

[Phil Schwartau]

Reassigning to Kenton -

Comment 3

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/js1_5/Array/regress-157652.js

Note: if I load this interactively in the debug JS shell, 
I get an assertion failure:

js> load('../../tests/js1_5/shell.js')
js> load('../../tests/js1_5/Array/regress-157652.js')
    BUGNUMBER: 157652
    STATUS: Testing that we don't crash on Array.sort()
    Assertion failure: nbytes != 0, at jsapi.c:1417

Comment 4

[Kenton Hanson (gone)]

Attachment: proposed patch

Checks for integer overflow.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 91586 [details] [diff] [review]
proposed patch

The (size_t) cast before len and the parens around the products len *
sizeof(jsval) and (double) len * sizeof(jsval) are unnecessary.

What's more, you do not want to return JS_FALSE without first reporting an
error when failing.  Note that if (!vec), then JS_malloc already reported an
OOM error.

/be

Comment 6

[Brendan Eich [:brendan]]

Rather than reporting a new error and extending js.msg, I suggest leaving the if
(!vec) return false clause alone, and adding the overflow test in an if
condition governing a then block that calls JS_ReportOutOfMemory(cx) and then
returns false.  Who needs "Array too long" as opposed to "Out of memory", for
this silly hard case?

/be

Comment 7

[Mitchell Stoltz (not reading bugmail)]

Brendan, the reporter claims this is exploitable and can be used to execute
arbitrary instructions. Do you think that's possible, or is this just a crash?

Comment 8

[Brendan Eich [:brendan]]

mstoltz: I don't know how to control the contents of memory beyond the piece
returned by malloc called by JS_malloc, the piece that has length
(size_t)(((double)len * sizeof(jsval)) % 4294967296) bytes.  If the reporter
knows, can you draw him or her out on the technique?

khanson, I suggest adding the overflow case in comment #6 *before* the JS_malloc
call, not after.

/be

Comment 9

[Kenton Hanson (gone)]

Attachment: revised patch

Fixed nits.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 91626 [details] [diff] [review]
revised patch

>Index: mozilla/js/src/jsarray.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsarray.c,v
>retrieving revision 3.36
>diff -w -u -2 -r3.36 jsarray.c
>--- mozilla/js/src/jsarray.c	27 May 2002 05:53:57 -0000	3.36
>+++ mozilla/js/src/jsarray.c	17 Jul 2002 11:05:43 -0000
>@@ -822,4 +822,9 @@
>     if (len == 0)
>         return JS_TRUE;
>+                                /* test for integer overflow */

This seems way overindented.  How about making it start in column 5, maybe with
a empty line before it, and capitalized with full stop at end?	Prevailing
style, etc.

>+    if ((size_t) len * sizeof(jsval) != (double) len * sizeof(jsval)) {
>+        JS_ReportOutOfMemory(cx);
>+	      return JS_FALSE;
>+	  }

The last two lines have tabs (one each per line, at the front), horrors!

>     vec = (jsval *) JS_malloc(cx, (size_t) len * sizeof(jsval));

Hmm, maybe a local variable for explicit common subexpression elim:

    size_t nbytes;
    ...
    nbytes = len * sizeof(jsval);
    if (nbytes != (double) len * sizeof(jsval)) {
	...
    }
    vec = (jsval *) JS_malloc(cx, nbytes);

Fix these true nits and sr=[e-mail].

/be
>     if (!vec)

Comment 11

[Kenton Hanson (gone)]

Attachment: revised revised patch

True nits fixed.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 93215 [details] [diff] [review]
revised revised patch

sr=[e-mail] -- thanks, sorry I lost track of this.  It would be good
to get into 1.1 and 1.0.1.

Roger, can you r=?

/be

Comment 13

[rogerl (gone)]

Comment on attachment 93215 [details] [diff] [review]
revised revised patch

r=rogerl

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 93215 [details] [diff] [review]
revised revised patch

Approved for 1.1 and 1.0.1.  Please add a fixed1.0.1 when fixed on branch.

Comment 15

[Brendan Eich [:brendan]]

I took the liberty of checking this in (with an extended comment, to justify the
major-comment-style and to make the problem completely clear) to the trunk and
the 1.0 branch.  Thanks to all,

/be

Comment 16

[Phil Schwartau]

Patch verified on the trunk and the MOZILLA_1_0_BRANCH in CVS.

The above testcase, mozilla/js/tests/js1_5/Array/regress-157652.js,
no longer crashes on Linux, WinNT, Mac9.1, or Mac OSX.

Marking Verified FIXED.

Comment 17

[Phil Schwartau]

Adding fixed1.0.1, verified1.0.1 keywords -