Last Comment Bug 599468 - (CVE-2010-3767) NewIdArray Integer Overflow Remote Code Execution Vulnerability (ZDI-CAN-884)
: NewIdArray Integer Overflow Remote Code Execution Vulnerability (ZDI-CAN-884)
: crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.9.2 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Nicholas Nethercote [:njn]
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2010-09-24 13:02 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2011-01-27 17:22 PST (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (against m-1.9.2 34659:6c6d174cf345) (1.24 KB, patch)
2010-10-03 21:47 PDT, Nicholas Nethercote [:njn]
gal: review+
christian: approval1.9.2.13+
christian: approval1.9.1.16+
Details | Diff | Splinter Review
Backport for 1.9.0 (1.26 KB, patch)
2010-12-08 09:06 PST, Mike Hommey [:glandium]
gal: review+
Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2010-09-24 13:02:35 PDT
Created attachment 478372 [details]

ZDI-CAN-884: Mozilla Firefox NewIdArray Integer Overflow Remote Code Execution Vulnerability

-- CVSS ----------------------------------------------------------------
9, (AV:N/AC:L/Au:N/C:P/I:P/A:C)

-- ABSTRACT ------------------------------------------------------------

TippingPoint has identified a vulnerability affecting the following 

    Mozilla Firefox 3.6.x

-- VULNERABILITY DETAILS -----------------------------------------------

This vulnerability allows remote attackers to execute arbitrary code on
vulnerable installations of Mozilla Firefox. User interaction is
required to exploit this vulnerability in that the target must visit a
malicious page or open a malicious file.

The specific flaw exists within Firefox's management of the
JSSLOT_ARRAY_COUNT annotation. This value represents the number of items
filled within a given Array object. If an attacker creates an array to a
high enough value, an initialization routine can be made to mis-allocate
a buffer. This can be abused by an attacker to corrupt memory and
subsequently execute arbitrary code under the context of the user
running the browser.

Version(s)  tested: Mozilla Firefox 3.6.6
Platform(s) tested: Windows XP SP3 x86

The value associated with JSSLOT_ARRAY_COUNT (below as
"num_properties") is being used during the initialization phase of array
content enumeration. From js/src/jsapi.cpp:

JS_Enumerate(JSContext *cx, JSObject *obj) {
    jsint i, n;
    jsval iter_state, num_properties;
    jsid id;
    JSIdArray *ida;
    jsval *vector;

    /* Get the number of properties to enumerate. */
    if (!obj->enumerate(cx, JSENUMERATE_INIT, &iter_state,
        goto error;

    n = JSVAL_TO_INT(num_properties);

    /* Create an array of jsids large enough to hold all the properties
    ida = NewIdArray(cx, n);
    if (!ida)
        goto error;

    i = 0;
    vector = &ida->vector[0];
    for (;;) {
        if (!obj->enumerate(cx, JSENUMERATE_NEXT, &iter_state, &id))
            goto error;
        vector[i++] = id;

So let's assume that num_properties = 0x3fffffff.

NewIdArray(JSContext *cx, jsint length)
    JSIdArray *ida;

    ida = (JSIdArray *)
        cx->malloc(offsetof(JSIdArray, vector) + length *
    if (ida)
        ida->length = length;
    return ida;

For offsetof(JSIdArray, vector) = 4, length = 0x3fffffff and
sizeof(jsval) = 4 (32-bit x86) integer overflows and cx->malloc(0) call
is made. Next,
ida->length is set to huge positive value.

Coming back to JS_Enumerate we can see that 0-byte long vector array
will be overflown by indices values resulting in memory corruption.

-- CREDIT --------------------------------------------------------------

This vulnerability was discovered by:
    * regenrecht
Comment 1 Reed Loden [:reed] (use needinfo?) 2010-09-24 13:13:11 PDT

Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/20100924 Namoroka/3.6.11pre
Comment 2 Daniel Veditz [:dveditz] 2010-09-24 13:40:02 PDT crashes: bp-228f9185-3e28-4696-b5a5-ab9112100924

Trunk doesn't crash.

The signature on 1.9.1/1.9.2 is the same as bug 509075 which is the fix for the other recent ZDI submission, bug 598669
Comment 3 Andreas Gal :gal 2010-10-03 12:11:27 PDT
Nick, want to take a look? Trunk is probably fine (I removed ARRAY_COUNT a couple months ago, Nick reviewed the patch) but we should double check.
Comment 4 Nicholas Nethercote [:njn] 2010-10-03 15:44:30 PDT
Sure, I'll take a look.
Comment 5 Nicholas Nethercote [:njn] 2010-10-03 21:46:15 PDT
AIUI, internally we record the LENGTH as the number of elements in a dense
array, and the COUNT as the number of non-hole elements we are actually storing.  LENGTH and COUNT need to always be updated in tandem, but in array_length_setter they aren't.

Here's the key part of the JS code:

  for (i = 0; i < loops; ++i) {
    arr.unshift.apply(arr, filler);
    arr.length = overflow;

It repeatedly prepends 65536 elements to the front of the array with
unshift().  This causes both LENGTH and COUNT to be incremented by 65536,
correctly.  It then then sets 'length' to 'overflow' (== 100), which
truncates the final 65436 elements from the end of the array.  However,
COUNT is not updated at this time, as it should be.  Repeating this many
times allows COUNT to reach arbitrarily large numbers.

The attached patch changes array_length_setter so that it updates COUNT correctly.  Note that this makes the proof-of-concept run much more slowly -- not because of the hole counting, but because previously the array stayed dense, but now it gets slowified when 65536 elements are unshifted onto a 100-element array.  (It wasn't getting slowified because the count was wrong!)
If you change chunk_len from 1<<16 to 1<<8 the slowification is avoided.

I looked through all the other places where LENGTH is set and this was the only place where COUNT wasn't updated appropriately, AFAICT.

(Three cheers for COUNT having been removed on the trunk!  Life is much less error-prone without it.)
Comment 6 Nicholas Nethercote [:njn] 2010-10-03 21:47:48 PDT
Created attachment 480562 [details] [diff] [review]
patch (against m-1.9.2 34659:6c6d174cf345)

BTW, I considered porting the remove-COUNT patch from trunk, but there were several pieces to it and heaps of array-related changes preceded that patch, so a branch-only fix seemed easier.
Comment 7 Andreas Gal :gal 2010-10-03 22:23:54 PDT
Comment on attachment 480562 [details] [diff] [review]
patch (against m-1.9.2 34659:6c6d174cf345)

What about the other places that modify ARRAY_COUNT? And I think brendan wants {} around the body of the for since its more than one line.
Comment 8 Andreas Gal :gal 2010-10-03 22:24:14 PDT
Trunk is not affected.
Comment 9 Nicholas Nethercote [:njn] 2010-10-03 22:28:19 PDT
(In reply to comment #7)
> Comment on attachment 480562 [details] [diff] [review]
> patch (against m-1.9.2 34659:6c6d174cf345)
> What about the other places that modify ARRAY_COUNT?

Which other places?  What about them?
Comment 10 Reed Loden [:reed] (use needinfo?) 2010-10-03 22:40:53 PDT
(In reply to comment #8)
> Trunk is not affected.

You can set "status2.0" to "unaffected" to reflect this.
Comment 11 Nicholas Nethercote [:njn] 2010-10-04 16:15:23 PDT
This patch is ready to land on 1.9.1 and 1.9.2.
Comment 12 Reed Loden [:reed] (use needinfo?) 2010-10-04 23:28:58 PDT
(In reply to comment #11)
> This patch is ready to land on 1.9.1 and 1.9.2.

You need to request approval on the patch for each branch.
Comment 13 Nicholas Nethercote [:njn] 2010-10-04 23:52:07 PDT
(In reply to comment #12)
> You need to request approval on the patch for each branch.

How do I do that?
Comment 14 Reed Loden [:reed] (use needinfo?) 2010-10-05 00:19:47 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > You need to request approval on the patch for each branch.
> How do I do that?

Click "Details" next to the patch. Scroll down to the flags. For the two appropriate approval choices ( and, change their boxes to '?'. Click "Submit".
Comment 15 christian 2010-10-06 10:24:49 PDT
Comment on attachment 480562 [details] [diff] [review]
patch (against m-1.9.2 34659:6c6d174cf345)

a=LegNeato for and Please land only on the mozilla-1.9.2 default branch and mozilla-1.9.1 default branch, *not* the relbranches.
Comment 16 Nicholas Nethercote [:njn] 2010-10-06 18:08:42 PDT
Sayre has volunteered to do the landings.
Comment 17 Johnny Stenback (:jst, 2010-11-09 14:16:29 PST
sayrer, can you get this landed?
Comment 18 Daniel Veditz [:dveditz] 2010-11-18 14:41:10 PST

The 1.9.1 patch needed a slight tweak -- the old local variable name was oldsize rather than capacity.
Comment 19 Mike Hommey [:glandium] 2010-12-08 09:06:35 PST
Created attachment 496163 [details] [diff] [review]
Backport for 1.9.0

Note You need to log in before you can comment on or make changes to this bug.