Bug 599468 (CVE-2010-3767)

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: reed, Assigned: njn)

Tracking

({crash, testcase})

1.9.2 Branch
crash, testcase
Points:
---

Firefox Tracking Flags

(status2.0 unaffected, blocking1.9.2 .13+, status1.9.2 .13-fixed, blocking1.9.1 .16+, status1.9.1 .16-fixed)

Details

(Whiteboard: [sg:critical])

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
Created attachment 478372 [details]
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
(Reporter)

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
(Reporter)

Comment 1

7 years ago
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
status1.9.1: --- → wanted
status1.9.2: --- → wanted
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
Sure, I'll take a look.
Assignee: general → nnethercote
(Assignee)

Comment 5

7 years ago
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.)
(Assignee)

Comment 6

7 years ago
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.
Attachment #480562 - Flags: review?(gal)

Comment 7

7 years ago
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+

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical]

Comment 8

7 years ago
Trunk is not affected.
blocking2.0: ? → ---
(Assignee)

Comment 9

7 years ago
(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?
(Reporter)

Comment 10

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

You can set "status2.0" to "unaffected" to reflect this.
status2.0: --- → unaffected
(Assignee)

Comment 11

7 years ago
This patch is ready to land on 1.9.1 and 1.9.2.
(Reporter)

Comment 12

7 years ago
(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.
(Assignee)

Comment 13

7 years ago
(In reply to comment #12)
> 
> You need to request approval on the patch for each branch.

How do I do that?
(Reporter)

Comment 14

7 years ago
(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
(Assignee)

Updated

7 years ago
Attachment #480562 - Flags: approval1.9.2.12?
Attachment #480562 - Flags: approval1.9.1.15?

Updated

7 years ago
blocking1.9.1: needed → .15+
blocking1.9.2: needed → .12+

Comment 15

7 years ago
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+
(Assignee)

Comment 16

7 years ago
Sayre has volunteered to do the landings.
Alias: ZDI-CAN-884
(Reporter)

Updated

7 years ago
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?
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.
status1.9.1: wanted → .16-fixed
status1.9.2: wanted → .13-fixed
(Reporter)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [sg:critical][needs sayre to land] → [sg:critical]
Created attachment 496163 [details] [diff] [review]
Backport for 1.9.0
Attachment #496163 - Flags: review?(gal)

Updated

7 years ago
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.