568465 - GC hazard with lookupProperty callers with non-native objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]

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]

In unrelated news, we really should remove setting __proto__. We have Object.create now.

Comment 3

[Andreas Gal :gal]

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]

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]]

(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]

Attachment: patch

This patch fixes the specific rooting bug here. I am still auditing the remaining call sites.

Comment 7

[Andreas Gal :gal]

Attachment: 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]]

Comment on attachment 448319 [details] [diff] [review]
patch

Great, thanks.

/be

Comment 9

[Igor Bukanov]

(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]

(looking into #9)

Comment 11

[Andreas Gal :gal]

Almost fell off my radar. Looking again.

Comment 12

[LegNeato]

Andreas, any update?

Comment 13

[Andreas Gal :gal]

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]

We should probably check this in before we check in the stack scanner.

Comment 15

[Andreas Gal :gal]

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]

can we land this?

Comment 17

[Robert Sayre]

gal, what is plan for this patch?

Comment 18

[Andreas Gal :gal]

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]

Igor, what do you think?

Comment 20

[Igor Bukanov]

(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]

any verdict?

Comment 22

[Igor Bukanov]

(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]

I guess we should backport it, then

Comment 24

[Daniel Veditz [:dveditz]]

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

[LegNeato]

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]

Attachment: 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]

(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]

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

Stealing. Jorendorff isn't available until Monday.

Comment 29

[LegNeato]

Thanks Andreas! Is the 1.9.1 patch more involved?

Comment 30

[Brendan Eich [:brendan]]

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]

Attachment: fix for 191

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

Comment 32

[LegNeato]

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 33

[Igor Bukanov]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9efaf1bce1d7

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3cdb069181db

Comment 34

[Al Billings [:abillings - ex-MoCo]]

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]

(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ý [:stransky] (ni? me)]

Attachment: 1.8.0 version

Comment 37

[Mike Hommey [:glandium]]

Attachment: Patch for 1.9.0