599468 - (CVE-2010-3767) NewIdArray Integer Overflow Remote Code Execution Vulnerability (ZDI-CAN-884)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Reed Loden [:reed]]

Attachment: PoC

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 
products:

    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,
&num_properties))
        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 *
sizeof(jsval));
    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]]

bp-60ba0c4f-3aa6-4bb2-9a1e-951912100924

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.11pre) Gecko/20100924 Namoroka/3.6.11pre

Comment 2

[Daniel Veditz [:dveditz]]

1.9.1.14pre 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]

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 [inactive]]

Sure, I'll take a look.

Comment 5

[Nicholas Nethercote [inactive]]

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) {
    print(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 [inactive]]

Attachment: 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]

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]

Trunk is not affected.

Comment 9

[Nicholas Nethercote [inactive]]

(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]]

(In reply to comment #8)
> Trunk is not affected.

You can set "status2.0" to "unaffected" to reflect this.

Comment 11

[Nicholas Nethercote [inactive]]

This patch is ready to land on 1.9.1 and 1.9.2.

Comment 12

[Reed Loden [:reed]]

(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 [inactive]]

(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]]

(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 (1.9.1.15 and 1.9.2.12), change their boxes to '?'. Click "Submit".

Comment 15

[LegNeato]

Comment on attachment 480562 [details] [diff] [review]
patch (against m-1.9.2 34659:6c6d174cf345)

a=LegNeato for 1.9.2.12 and 1.9.1.15. 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 [inactive]]

Sayre has volunteered to do the landings.

Comment 17

[Johnny Stenback (:jst)]

sayrer, can you get this landed?

Comment 18

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5450a19c59de
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/815454d74e8a

The 1.9.1 patch needed a slight tweak -- the old local variable name was oldsize rather than capacity.

Comment 19

[Mike Hommey [:glandium]]

Attachment: Backport for 1.9.0