814029 - (CVE-2013-0756) obj_toSource Use-After-Free Remote Code Execution Vulnerability (ZDI-CAN-1571)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: PoC

-- CVSS -----------------------------------------
7.5, AV:N/AC:L/Au:N/C:P/I:P/A:P

-- ABSTRACT -------------------------------------
TippingPoint has identified a vulnerability affecting the following products:

  Mozilla Firefox

-- VULNERABILITY DETAILS ------------------------
Tested on 16.0.2

From js/src/jsobj.cpp:

obj_toSource(JSContext *cx, uintN argc, Value *vp) {
...
JSObject *obj = ToObject(cx, &vp1);
if (!obj)
return false;

JSHashEntry *he = js_EnterSharpObject(cx, obj, &ida, &chars);
    if (!he)
        return false;
    ...
}
js_EnterSharpObject(JSContext *cx, JSObject *obj, JSIdArray **idap,
jschar **sp) {
...
*sp = NULL;
map = &cx->sharpObjectMap;
...

ida = NULL;
    if (map->depth == 0) {
        ...
        ++map->depth;
        he = MarkSharpObjects(cx, obj, &ida);
        --map->depth;
        ...
    } else {
        ...
    }
    ...
}
MarkSharpObjects(JSContext *cx, JSObject *obj, JSIdArray **idap) {
...
map = &cx->sharpObjectMap;
JS_ASSERT(map->depth >= 1);
table = map->table;
hash = js_hash_object(obj);
hep = JS_HashTableRawLookup(table, hash, obj);
he = *hep;
if (!he) {
...
ida = JS_Enumerate(cx, obj);
if (!ida)
return NULL;

ok = JS_TRUE;
        for (i = 0, length = ida->length; i < length; i++) {
            id = ida->vector[i];
            ok = obj->lookupGeneric(cx, id, &amp;obj2, &amp;prop);
            ...
        }
        ...
    } else {
        ...
    }
    ...
}
Note that |jsid|s stored at |ida| are unrooted during execution of |obj_toSource()| and its sub-functions. This means that after successful
call to |JS_Enumerate()| one can drop (previously returned) |jsid|s from the
parent object and force their removal due to garbage collection. It can be done
during call to |obj->lookupGeneric()|. Such action will lead to a dangling
pointer vulnerability at the following |for| iteration: |ida->vector[i]| will
contain pointer to already freed object.


-------
Exploit
-------

1. Create JavaScript Proxy object with the following characteristics:
a) Handler method "get" returns dummy data just to fulfill the contract
with the iterator.
b) Handler method "has" always returns "true" to prolong process of
iterating over properties. It also sprays the heap and forces garbage
collection.
c) Handler method "keys" returns bogus unrooted list of properties.
Instances of E4X QName class are used for the sake of reliability.
2. Call JavaScript object's |toSource()| method - this leads to a dangling
pointer condition.

-- CREDIT ---------------------------------------
This vulnerability was discovered by:

   regenrecht

-- FURTHER DETAILS ------------------------------
If supporting files were contained with this report they are provided within a password protected ZIP file. The password is the ZDI candidate number in the form: ZDI-CAN-XXXX where XXXX is the ID number.

Please confirm receipt of this report. We expect all vendors to remediate ZDI vulnerabilities within 180 days of the reported date. If you are ready to release a patch at any point leading up the the deadline please coordinate with us so that we may release our advisory detailing the issue. If the 180 day deadline is reached and no patch has been made available we will release a limited public advisory with our own mitigations so that the public can protect themselves in the absence of a patch. Please keep us updated regarding the status of this issue and feel free to contact us at any time:

Zero Day Initiative
[e-mail]

The PGP key used for all ZDI vendor communications is available from:

     http://www.zerodayinitiative.com/documents/zdi-pgp-key.asc

-- INFORMATION ABOUT THE ZDI ---------------------
Established by TippingPoint, The Zero Day Initiative (ZDI) represents a best-of-breed model for rewarding security researchers for responsibly disclosing discovered vulnerabilities.

The ZDI is unique in how the acquired vulnerability information is used. TippingPoint does not re-sell the vulnerability details or any exploit code. Instead, upon notifying the affected product vendor, TippingPoint provides its customers with zero day protection through its intrusion prevention technology. Explicit details regarding the specifics of the vulnerability are not exposed to any parties until an official vendor patch is publicly available. Furthermore, with the altruistic aim of helping to secure a broader user base, TippingPoint provides this vulnerability information confidentially to security vendors (including competitors) who have a vulnerability protection or mitigation product.

Please contact us for further information or refer to:

    http://www.zerodayinitiative.com

-- DISCLOSURE POLICY ----------------------------
Our vulnerability disclosure policy is available online at:

    http://www.zerodayinitiative.com/advisories/disclosure_policy/

Comment 1

[Andrew McCreight [:mccr8]]

I think toSource was rewritten in bug 761723 in 17, so it is possible this isn't a problem any more.

Comment 2

[Terrence Cole [:terrence]]

Ah, looks like it's time to do some bugzilla archeology.  Ah hem:

The sharpObjectMap was introduced by :igor in Bug 566141 to stop various sharp object related "crashes".  AFAICT from reading the bug, he just rooted a bunch of stuff and hoped for the best.  |ida| should have been protected by an |AutoIdArray| at this time, although I haven't gone back to check.

Later, :Waldo removed sharp's entirely from SpiderMonkey (Bug 566700) because they are not part of any standard and the list of security bugs they were causing us is absurdly massive. [1]  He didn't remove the recursive marking code, however, because it largely worked and rewriting it would have been an unnecessary hassle.  I believe that |ida| was protected by an |AutoIdArray| at this time.

Even later, I wanted to kill off the JSD hash tables so that I would only need to deal with moving objects in one type of table.  This is Bug 723346.  After this rewrite I /thought/ that |ida| was still protected by an |AutoIdArray|.  If I did mess up the rooting here, sorry.

However, the point is largely moot because after a few more months, :njn filed Bug 782467 to remove the sharpObjectMap entirely.  Since /be agreed with the notion, I went ahead and ripped it out at the roots.  Now, the only Sharp in SpiderMonkey is Werner Sharp, in jsxml.cpp.

If the sec drivers think it would be warranted, it would not terribly problematic to backport the "fix" from Bug 782467 to any branch desired.


[1] - Just search bugzilla for "sharp" && "Sharp" and pretty much everything that turns up is a security bug (red) or a "crash fix" (black).

Comment 3

[Terrence Cole [:terrence]]

The patch still applies cleanly against esr17 and release. I'd like to land on these branches in bug 782467 under the cover of wanting to remove sharps from esr17. Does this sound reasonable?

Comment 4

[Daniel Veditz [:dveditz]]

The shadowing thing is making this bug harder to understand, I think we can go for a traditional "depends on" in this case.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

talked with dan in IRC yesterday and we're not going to take this on the respin (not significant enough to be a driver), we'll get it in 18 and land to esr17 that ships with 18 so our advisories line up.

Comment 6

[Josh Aas]

Should this bug be marked fixed since it was fixed, and we're now just tracking an esr patch?

Comment 7

[Andrew McCreight [:mccr8]]

Marking fixed, as it is fixed on trunk. Assigning to Terrence, who landed the relevant patch on 17.

Comment 8

[Naveed Ihsanullah [:naveed]]

Terrence is out on PTO till 2013. What needs to be done here?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It sounds like one option is to land bug 782467 on esr17. It's a big patch, but it probably touches code that changes rarely, so it might be pretty safe.

I'm guessing that another option is to add a rooter somewhere.

Nick, with Terrence gone, you're probably the best person to assess the risks here.

Comment 10

[Terrence Cole [:terrence]]

The patch applied cleanly on esr17 with no rebasing when I tried it a few weeks ago.  The risk should be quite minimal, since the esr17 code is, I think, completely unchanged from when I landed 18.

Comment 11

[Nicholas Nethercote [inactive]]

> It sounds like one option is to land bug 782467 on esr17. It's a big patch,
> but it probably touches code that changes rarely, so it might be pretty safe.
> 
> I'm guessing that another option is to add a rooter somewhere.
> 
> Nick, with Terrence gone, you're probably the best person to assess the
> risks here.

Given those two choices, I definitely suggest landing the patch from bug 782467.  It is a biggish patch, but it removes much more code than it adds, and it's been baking on the channels for a while without apparent problem.  In contrast, adding a rooter is tricky and would require someone to dig and really understand this code and even then they might not fix it.

Comment 12

[Alex Keybl [:akeybl]]

OK - let's move forward with that. Please nominate for ESR17 approval.

Comment 13

[Terrence Cole [:terrence]]

Attachment: https://hg.mozilla.org/mozilla-central/rev/7d8832ad7f1f

I just got back from holiday and saw this wasn't done yet. Do we still have time to land this?

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:  This is sec-crit.
User impact if declined: Sec-crit vulnerability in ESR 17.
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 14

[Terrence Cole [:terrence]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/f497aa48101f