566141 - GC hazard in MarkSharpObjects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the following code in MarkSharpObject, http://hg.mozilla.org/tracemonkey/file/818656d6f1e4/js/src/jsobj.cpp#l276 :

            ok = obj->lookupProperty(cx, id, &obj2, &prop);
...
            ok = obj2->getAttributes(cx, id, prop, &attrs);
            if (ok) {
                if (obj2->isNative() &&
                    (attrs & (JSPROP_GETTER | JSPROP_SETTER))) {
                    JSScopeProperty *sprop = (JSScopeProperty *) prop;
                    val = JSVAL_VOID;
                    if (attrs & JSPROP_GETTER)
                        val = sprop->getterValue();
                    if (attrs & JSPROP_SETTER) {
                        if (val != JSVAL_VOID) {
                            /* Mark the getter, then set val to setter. */
                            ok = (MarkSharpObjects(cx, JSVAL_TO_OBJECT(val),
                                                   NULL)
                                  != NULL);
                        }
...
            }
            obj2->dropProperty(cx, prop);

This code has a bug as it can start a long-running operation between lookupProperty and dropProperty calls due to a recursion in MarkSharpObjects. Without the proposed changes from the bug 546590 this can only hurt embeddings that share objects between threads as it may nests locks itc. For the browser this is harmless as lookupProperty/getAttributes at worst can run a resolve hook and their implementation in the browser do not run arbitrary code.

But proxies chnage that. Now lookupProperty can run an arbitrary script. Thus if MarkSharpObjects for the setter discovers a proxy, it could run a script that can delete the property under lookup and its scope. That would lead to a hazard.

Comment 1

[Andreas Gal :gal]

This is not currently a bug. The code in question hasn't been landed yet. Also, we are eliminating shared objects. So critical is absolutely overstating this.

Comment 2

[Brendan Eich [:brendan]]

"we are eliminating shared objects"? You mean eliminating JSProperty * from the JSObjectOps hooks?

/be

Comment 3

[Andreas Gal :gal]

We are eliminating sharing native objects across threads.

Comment 4

[Reed Loden [:reed]]

gal, we don't use [sg:investigate] anymore. Better to just leave it blank (and the bug security-sensitive) if you're not sure.

Comment 5

[Igor Bukanov]

The proxy code would expose another problem in MarkSharpObjects. There the function does not root the id array it receives from JS_Enumerate. With a proxy it would be trivial to wright a script that in lookupProperty unroots and GC-collects an id stored in the array before they are used. 

Note that this is not a problem with a proxy as a similar issue may exists if a resolve hook could be triggered to run a script.

Comment 6

[Igor Bukanov]

One do not need a proxy to exploit this. When working over the object tree MarkSharpObjects may call getProperty on non-native object. But that can run an arbitrary script via, for example, a getter on a native prototype of a non-native fast array.

Comment 7

[Igor Bukanov]

(In reply to comment #5)
> The proxy code would expose another problem in MarkSharpObjects. There the
> function does not root the id array it receives from JS_Enumerate. With a proxy
> it would be trivial to wright a script that in lookupProperty unroots and
> GC-collects an id stored in the array before they are used. 

Sorry, the id itself is safe thanks to the JS_KEEP_ATOMS call. What is the problem is sprop itself. A script invoked when MarkSharpObjects is called to mark the getter can remove the property and force the GC so sprop would be deleted before it is used to access the setter. Another possibility is to GC obj2 after obj___proto__ = NULL before it is used in the dropProperty call.

But I will create a test script before doing any father work.

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> But I will create a test script before doing any father work.

My attempt to create a test case has revealed that one needs a power of proxy to exploit this. Without proxy JS_Enumerate for non-native objects returns ids that would never trigger any script hooks. Thus it is not possible to exploit the bug with getter defined on Array.prototype.

Comment 10

[Damon Sicore (:damons)]

Igor, so would you say this is not sg:critical based on Comment 8 ?

Comment 11

[Igor Bukanov]

(In reply to comment #8)
> My attempt to create a test case has revealed that one needs a power of proxy
> to exploit this. Without proxy JS_Enumerate for non-native objects returns ids
> that would never trigger any script hooks. Thus it is not possible to exploit
> the bug with getter defined on Array.prototype.

On 1.9.2 and before one can use liveconnect and create a Java applet with a some Java getter that will be invoked during id iteration loop in MarkSharpObject. That in turn can recurse into JavaScript world and do the damage AFAICS. So the bug is not critical only TM/mozilla-central tips where LiveConnect is switched off but the proxy code is not yet landed.

Comment 12

[Igor Bukanov]

(In reply to comment #10)
> Igor, so would you say this is not sg:critical based on Comment 8 ?

I would not say for sure yet (I will need to refresh my memories about LiveConnect and create an example for a proof), but it does look like sg:critical.

Comment 13

[Damon Sicore (:damons)]

Who should own this?  Need an owner here.

Thanks a bunch, Igor.

Comment 14

[Andreas Gal :gal]

Proxies are about to land, so this needs a fix. Igor, can you take this? You know the code a lot better than me. Otherwise I take it.

Comment 15

[Damon Sicore (:damons)]

Blocking 1.9.3 final as it's an sg:crit.

Comment 16

[Igor Bukanov]

Attachment: v1

This is what I am testing

Comment 17

[Igor Bukanov]

Here is test case that is based on the new Proxy code:

var proxy = Proxy.create({
      enumerateOwn: function() {
            delete x.property_that_can_be_deleted;
            gc();
            return [];
        }
    });

function f() { print("function f");}
f.proxy = proxy;

var x = {};
x.__defineGetter__("property_that_can_be_deleted", f);
x.__defineSetter__("property_that_can_be_deleted", f);
uneval(x);

The test case crashes/asserts when MarkSharpObjects tries to access a setter value for the deleted property.

Comment 18

[Igor Bukanov]

brendan: review ping

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 446189 [details] [diff] [review]
v1

Maybe invert !isNative test to put best foot forward in prefetched-is-predicted sense. r=me otherwise.

/be

Comment 20

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/54acf298872b - with the inverted isNative test.

Comment 21

[Robert Sayre]

fixed in tracemonkey?

Comment 22

[Igor Bukanov]

(In reply to comment #21)
> fixed in tracemonkey?

yes

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/54acf298872b

Comment 24

[Igor Bukanov]

This bug should be a blocker on branches. AFAICS with liveconnect one can build an exploit here.

Comment 25

[Igor Bukanov]

Attachment: fix for 192

192 backport follows the logic of TM patch adjusted for older internal API.

Comment 26

[Igor Bukanov]

Attachment: fix for 191

191 needs a separated fix to account for OBJ_LOOKUP_PROPERTY change on later
branches.

Comment 27

[Igor Bukanov]

Attachment: plain diff between 192 and 191 patches

Comment 28

[Daniel Veditz [:dveditz]]

Blocking for the next set of branch releases, the current ones are in (slushy) code-freeze as we try to wrap up.

Comment 29

[Igor Bukanov]

(In reply to comment #28)
> Blocking for the next set of branch releases, the current ones are in (slushy)
> code-freeze as we try to wrap up.

It is important not to open the bug 568465 until this and a related bug 568073 is fixed. Otherwise one can get a strong hit how to look for the problem. 

How do we note such open-together dependency?

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 466243 [details] [diff] [review]
fix for 191

Approved for 1.9.1.13, a=dveditz for release-drivers

Is the other patch ready for 1.9.2 approval, or was there a reason you didn't request it?

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 465981 [details] [diff] [review]
fix for 192

Approved for 1.9.2.10, a=dveditz for release-drivers

Comment 32

[Igor Bukanov]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f070c6116d9d

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a3974b15591b

Comment 33

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 version