Closed Bug 599468 (CVE-2010-3767) Opened 9 years ago Closed 9 years ago

NewIdArray Integer Overflow Remote Code Execution Vulnerability (ZDI-CAN-884)

Categories

(Core :: JavaScript Engine, defect, critical)

1.9.2 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
status2.0 --- unaffected
blocking1.9.2 --- .13+
status1.9.2 --- .13-fixed
blocking1.9.1 --- .16+
status1.9.1 --- .16-fixed

People

(Reporter: reed, Assigned: njn)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical])

Attachments

(2 files)

Attached file 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
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
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
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
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
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.
Sure, I'll take a look.
Assignee: general → nnethercote
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.)
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.
Attachment #480562 - Flags: review?(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.
Attachment #480562 - Flags: review?(gal) → review+
Whiteboard: [sg:critical?] → [sg:critical]
Trunk is not affected.
blocking2.0: ? → ---
(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?
(In reply to comment #8)
> Trunk is not affected.

You can set "status2.0" to "unaffected" to reflect this.
This patch is ready to land on 1.9.1 and 1.9.2.
(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.
(In reply to comment #12)
> 
> You need to request approval on the patch for each branch.

How do I do that?
(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".
Status: NEW → ASSIGNED
Attachment #480562 - Flags: approval1.9.2.12?
Attachment #480562 - Flags: approval1.9.1.15?
blocking1.9.1: needed → .15+
blocking1.9.2: needed → .12+
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.
Attachment #480562 - Flags: approval1.9.2.12?
Attachment #480562 - Flags: approval1.9.2.12+
Attachment #480562 - Flags: approval1.9.1.15?
Attachment #480562 - Flags: approval1.9.1.15+
Sayre has volunteered to do the landings.
Alias: ZDI-CAN-884
Alias: ZDI-CAN-884
Keywords: checkin-needed
Whiteboard: [sg:critical] → [sg:critical][needs sayre to land]
Alias: CVE-2010-3767
sayrer, can you get this landed?
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:critical][needs sayre to land] → [sg:critical]
Attachment #496163 - Flags: review?(gal)
Attachment #496163 - Flags: review?(gal) → review+
Group: core-security
You need to log in before you can comment on or make changes to this bug.