Last Comment Bug 568465 - GC hazard with lookupProperty callers with non-native objects
: GC hazard with lookupProperty callers with non-native objects
Status: RESOLVED FIXED
[sg:critical?] keep hidden until 5661...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.9.1 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 569162
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-27 05:51 PDT by Igor Bukanov
Modified: 2010-09-27 18:28 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
unaffected
.9+
.9-fixed
.12+
.12-fixed


Attachments
patch (865 bytes, patch)
2010-05-30 18:47 PDT, Andreas Gal :gal
no flags Details | Diff | Review
patch (2.20 KB, patch)
2010-05-30 19:00 PDT, Andreas Gal :gal
brendan: review+
Details | Diff | Review
fix for 192 (1.60 KB, patch)
2010-08-11 12:03 PDT, Igor Bukanov
gal: review+
christian: approval1.9.2.9+
Details | Diff | Review
fix for 191 (2.32 KB, patch)
2010-08-16 00:57 PDT, Igor Bukanov
igor: review+
christian: approval1.9.1.12+
Details | Diff | Review
1.8.0 version (1.59 KB, patch)
2010-08-26 03:23 PDT, Martin Stránský
igor: review+
Details | Diff | Review
Patch for 1.9.0 (2.45 KB, patch)
2010-09-03 09:55 PDT, Mike Hommey [:glandium]
igor: review+
Details | Diff | Review

Description Igor Bukanov 2010-05-27 05:51:28 PDT
Most if not all all callers of  lookupProperty pass pointer to unrooted JSObject * from the native stack for the objp parameter. The assumption is that objp would be either the original rooted object or would come from the prototype in which case it would be rooted by the original object. But proxy breaks that assumption since the has method implementing lookupProperty can remove proxy from the prototype chain unrooting and garbage-collecting the proxy. The following test demonstrates that:

Object.prototype.__proto__ = Proxy.create({
        get has() {
            Object.prototype.__proto__ = null;
            gc();
            return function(name) {
                print(1);
                return name == 'a';
            }
        },
    });

'a' in {};

It generates:
/home/igor/s/y.js:12: TypeError: proxy was fixed while executing the handler

That error comes from isProxy check to verify that the class is still a proxy. This is potentially exploitable since the script can use the pattern above recursively with 1000 or so proxies per one GC cycle and eventually learn the address of js_ProxyClass and pass isProxy check.

As a workaround the proxy can root the proxy argument to all proxy_ methods implementing JSObjectOps. But the right solution it seems to require a conservative stack scanner for the proxies.
Comment 1 Andreas Gal :gal 2010-05-30 16:25:43 PDT
Anything with a non-trivial lookupProperty can exhibit the same behavior, including liveconnect or other embeddings. Conservative stack scanning will fix this nicely. In the meantime we should root properly when calling lookupProperty.
Comment 2 Andreas Gal :gal 2010-05-30 16:28:55 PDT
In unrelated news, we really should remove setting __proto__. We have Object.create now.
Comment 3 Andreas Gal :gal 2010-05-30 16:36:30 PDT
I will spin off a bug to fix the proxy side to unblock the dependency. This bug should remain closed until we fix the larger issue that also affects liveconnect.
Comment 4 Andreas Gal :gal 2010-05-30 16:43:42 PDT
If we don't ship liveconnect, and we ignore extension-provided non-native objects, and we fix 569162 (patch attached to that bug), then we can unblock on this one for 1.9.3 but we should keep it closed.
Comment 5 Brendan Eich [:brendan] 2010-05-30 17:04:55 PDT
(In reply to comment #2)
> In unrelated news, we really should remove setting __proto__. We have
> Object.create now.

This is false -- "we" meaning anything useful such as the users of writable __proto__ don't have Object.create -- it has not shipped yet, and ES5 is not yet fully implemented so one recommended way to detect whether it is supported does not work.

Writable __proto__ won't go away until a release after the one that ships ES5, at the earliest. Depending on what breaks. We might keep o = {__proto__:p, ...} and other acyclic forms even then.

/be
Comment 6 Andreas Gal :gal 2010-05-30 18:47:33 PDT
Created attachment 448317 [details] [diff] [review]
patch

This patch fixes the specific rooting bug here. I am still auditing the remaining call sites.
Comment 7 Andreas Gal :gal 2010-05-30 19:00:36 PDT
Created attachment 448319 [details] [diff] [review]
patch

Don't rely on obj->getProto() to remain rooted during lookupProperty calls. This patch fixes all 3 sites where we do a proto lookup afaict. Can't wait for stack scanning. Until then this should do.
Comment 8 Brendan Eich [:brendan] 2010-05-30 20:11:56 PDT
Comment on attachment 448319 [details] [diff] [review]
patch

Great, thanks.

/be
Comment 9 Igor Bukanov 2010-05-31 04:56:47 PDT
(In reply to comment #7)
> Created an attachment (id=448319) [details]
> patch
> 
> Don't rely on obj->getProto() to remain rooted during lookupProperty calls.
> This patch fixes all 3 sites where we do a proto lookup afaict. Can't wait for
> stack scanning. Until then this should do.

With the patch once lookupProperty returns the obj2 will be unrooted. Yet the code calls getAttributes on it. So the proxy can still collect it in proxy_getAtributes/proxy_setAttributes. Thus for complete safety the patch should root the obj argument of those 2 methods.
Comment 10 Andreas Gal :gal 2010-06-01 13:49:11 PDT
(looking into #9)
Comment 11 Andreas Gal :gal 2010-06-08 13:19:24 PDT
Almost fell off my radar. Looking again.
Comment 12 christian 2010-06-14 10:08:12 PDT
Andreas, any update?
Comment 13 Andreas Gal :gal 2010-06-14 21:49:23 PDT
We can drop 1.9.3 blocking here. 1.9.3 has a conservative stack scanner, which will detect these roots.
Comment 14 Robert Sayre 2010-06-15 13:28:00 PDT
We should probably check this in before we check in the stack scanner.
Comment 15 Andreas Gal :gal 2010-06-15 13:31:16 PDT
We still need a fix for 192 191, so I am not off the hook here. I just think the 193 schedule won't have to stall on this.
Comment 16 Robert Sayre 2010-06-22 13:17:19 PDT
can we land this?
Comment 17 Robert Sayre 2010-07-13 13:33:47 PDT
gal, what is plan for this patch?
Comment 18 Andreas Gal :gal 2010-07-13 13:54:56 PDT
This no longer needs a fix for 1.9.3 (we have conservative stack scanning now) and is probably very hard to exploit with live connect. Should I try to make a patch for the older branches?
Comment 19 Robert Sayre 2010-07-13 15:41:17 PDT
Igor, what do you think?
Comment 20 Igor Bukanov 2010-07-14 02:43:12 PDT
(In reply to comment #19)
> Igor, what do you think?

It seems that LiveConnect does not expose to Java applets enough functionality to make this exploitable. But I will double-check.
Comment 21 Robert Sayre 2010-07-20 13:27:14 PDT
any verdict?
Comment 22 Igor Bukanov 2010-07-20 13:51:56 PDT
(In reply to comment #21)
> any verdict?

At this point I assume that this is exploitable as there are paths in the liveconnect code that calls JS_GetProperty during lookupProperty. I should have wrote about that earlier. But I have not yet managed to create a test case.
Comment 23 Robert Sayre 2010-08-03 13:20:19 PDT
I guess we should backport it, then
Comment 24 Daniel Veditz [:dveditz] 2010-08-10 13:32:08 PDT
This isn't needed on the trunk so needs to block the branches more directly. A small patch might be easy enough to back-port for this set of releases (code-freeze Thursday Aug 12), otherwise we should get it next time.
Comment 25 christian 2010-08-11 10:25:39 PDT
I'll set it as blocking as we would love a patch on the branches for 1.9.2.9 and 1.9.1.12. Please let me know if this can't be backported by Thursday Aug 12
Comment 26 Igor Bukanov 2010-08-11 12:03:12 PDT
Created attachment 464892 [details] [diff] [review]
fix for 192

Compared with the trunk patch in 192 version I have added an extra rooter for the result of the resolve hook for a full safety.
Comment 27 Igor Bukanov 2010-08-13 02:09:02 PDT
(In reply to comment #26)
> Created attachment 464892 [details] [diff] [review]
> fix for 192

Note about the GC safety of the patches here and why it is not necessary to root the objp at each call site of lookupProperty. By code inspection one can see that at call sites besides implementations of the lookupProperty hook (the exception is not yet fixed bug 566141 and bug 568073 on branches) the GC cannot happen when the caller accesses objp. Thus it is sufficient to fix only the implementation and ensure that the value stored in the mutable proto slot is safe during recursive lookupProperty invocation. This is what the patch fixes.
Comment 28 Andreas Gal :gal 2010-08-13 11:08:45 PDT
Comment on attachment 464892 [details] [diff] [review]
fix for 192

Stealing. Jorendorff isn't available until Monday.
Comment 29 christian 2010-08-13 13:36:38 PDT
Thanks Andreas! Is the 1.9.1 patch more involved?
Comment 30 Brendan Eich [:brendan] 2010-08-13 13:42:21 PDT
I copied the patch to my pastebuffer, refreshed my 1.9.1 tree, and found this:

$ pbpaste | patch --dry-run -p3 --fuzz=3
patching file jsarray.cpp
Hunk #1 succeeded at 736 with fuzz 3.
patching file jsobj.cpp
Hunk #1 succeeded at 3818 with fuzz 3.
Hunk #2 succeeded at 3885 with fuzz 3.
patching file liveconnect/jsj_JavaArray.c

No time to do more, but it looks like it applies.

/be
Comment 31 Igor Bukanov 2010-08-16 00:57:52 PDT
Created attachment 466242 [details] [diff] [review]
fix for 191

191 needs a separated fix to account for OBJ_LOOKUP_PROPERTY change on later branches.
Comment 32 christian 2010-08-16 10:25:08 PDT
a=LegNeato for 1.9.2.9 and 1.9.2.12. We'll let this one land for the current dev release instead of pushing it to the next, so please land on mozilla-1.9.2 default and mozilla-1.9.1 default. Thanks!
Comment 34 [On PTO until 6/29] 2010-08-19 17:45:27 PDT
Looking at this, it isn't clear how to verify this fix. Does someone have a clear set of STR since there are no included tests?
Comment 35 Igor Bukanov 2010-08-20 12:59:38 PDT
(In reply to comment #34)
> Looking at this, it isn't clear how to verify this fix. 

On branches the existence of the bug comes from code analysis that shows that liveconnect can be used in place of Proxy to trigger the same GC hazard.

For example, JavaObject_lookupProperty, http://hg.mozilla.org/releases/mozilla-1.9.2/file/cb079fb7300a/js/src/liveconnect/jsj_JavaObject.c#l819 , calls lookup_member_by_id with non-null vp argument. The latter then calls inherit_props_from_JS_natives if the corresponding Java object is, for example, java.lang.String. That function in turn calls JS_GetProperty on the prototype object and that via a getter can execute arbitrary JS code.
Comment 36 Martin Stránský 2010-08-26 03:23:35 PDT
Created attachment 469408 [details] [diff] [review]
1.8.0 version
Comment 37 Mike Hommey [:glandium] 2010-09-03 09:55:20 PDT
Created attachment 471890 [details] [diff] [review]
Patch for 1.9.0

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