Last Comment Bug 566141 - GC hazard in MarkSharpObjects
: GC hazard in MarkSharpObjects
Status: RESOLVED FIXED
[sg:critical][critsmash:investigating...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
: 523933 (view as bug list)
Depends on: 568485
Blocks: harmony:proxies
  Show dependency treegraph
 
Reported: 2010-05-15 10:02 PDT by Igor Bukanov
Modified: 2010-10-30 18:13 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.11+
.11-fixed
.14+
.14-fixed


Attachments
v1 (4.45 KB, patch)
2010-05-19 01:55 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
fix for 192 (4.47 KB, patch)
2010-08-14 03:32 PDT, Igor Bukanov
gal: review+
dveditz: approval1.9.2.11+
Details | Diff | Splinter Review
fix for 191 (4.51 KB, patch)
2010-08-16 01:20 PDT, Igor Bukanov
igor: review+
dveditz: approval1.9.1.14+
Details | Diff | Splinter Review
plain diff between 192 and 191 patches (1.38 KB, text/plain)
2010-08-16 01:23 PDT, Igor Bukanov
no flags Details
1.8.0 version (3.96 KB, patch)
2010-10-06 02:57 PDT, Martin Stránský
igor: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2010-05-15 10:02:13 PDT
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 2010-05-15 13:54:33 PDT
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] 2010-05-15 14:06:57 PDT
"we are eliminating shared objects"? You mean eliminating JSProperty * from the JSObjectOps hooks?

/be
Comment 3 Andreas Gal :gal 2010-05-15 14:07:58 PDT
We are eliminating sharing native objects across threads.
Comment 4 Reed Loden [:reed] (use needinfo?) 2010-05-15 14:11:03 PDT
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 2010-05-18 02:01:21 PDT
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 2010-05-18 04:31:15 PDT
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 2010-05-18 09:47:01 PDT
(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 2010-05-18 10:24:26 PDT
(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 9 Jeff Walden [:Waldo] (remove +bmo to email) 2010-05-18 10:58:13 PDT
*** Bug 523933 has been marked as a duplicate of this bug. ***
Comment 10 Damon Sicore (:damons) 2010-05-18 13:17:23 PDT
Igor, so would you say this is not sg:critical based on Comment 8 ?
Comment 11 Igor Bukanov 2010-05-18 13:25:50 PDT
(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 2010-05-18 13:30:38 PDT
(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) 2010-05-18 13:55:10 PDT
Who should own this?  Need an owner here.

Thanks a bunch, Igor.
Comment 14 Andreas Gal :gal 2010-05-18 14:06:20 PDT
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) 2010-05-18 17:56:14 PDT
Blocking 1.9.3 final as it's an sg:crit.
Comment 16 Igor Bukanov 2010-05-19 01:55:46 PDT
Created attachment 446189 [details] [diff] [review]
v1

This is what I am testing
Comment 17 Igor Bukanov 2010-05-19 07:39:37 PDT
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 2010-05-22 12:54:01 PDT
brendan: review ping
Comment 19 Brendan Eich [:brendan] 2010-05-24 15:54:56 PDT
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 2010-05-25 03:57:38 PDT
http://hg.mozilla.org/tracemonkey/rev/54acf298872b - with the inverted isNative test.
Comment 21 Robert Sayre 2010-05-25 13:35:49 PDT
fixed in tracemonkey?
Comment 22 Igor Bukanov 2010-05-25 13:49:07 PDT
(In reply to comment #21)
> fixed in tracemonkey?

yes
Comment 24 Igor Bukanov 2010-08-13 01:56:20 PDT
This bug should be a blocker on branches. AFAICS with liveconnect one can build an exploit here.
Comment 25 Igor Bukanov 2010-08-14 03:32:27 PDT
Created attachment 465981 [details] [diff] [review]
fix for 192

192 backport follows the logic of TM patch adjusted for older internal API.
Comment 26 Igor Bukanov 2010-08-16 01:20:04 PDT
Created attachment 466243 [details] [diff] [review]
fix for 191

191 needs a separated fix to account for OBJ_LOOKUP_PROPERTY change on later
branches.
Comment 27 Igor Bukanov 2010-08-16 01:23:28 PDT
Created attachment 466247 [details]
plain diff between 192 and 191 patches
Comment 28 Daniel Veditz [:dveditz] 2010-08-16 10:44:06 PDT
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 2010-08-16 12:53:23 PDT
(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] 2010-08-30 10:45:11 PDT
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] 2010-09-01 10:39:38 PDT
Comment on attachment 465981 [details] [diff] [review]
fix for 192

Approved for 1.9.2.10, a=dveditz for release-drivers
Comment 33 Martin Stránský 2010-10-06 02:57:45 PDT
Created attachment 481169 [details] [diff] [review]
1.8.0 version

Note You need to log in before you can comment on or make changes to this bug.