Closed Bug 568465 Opened 10 years ago Closed 10 years ago

GC hazard with lookupProperty callers with non-native objects

Categories

(Core :: JavaScript Engine, defect)

1.9.1 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
status2.0 --- unaffected
blocking1.9.2 --- .9+
status1.9.2 --- .9-fixed
blocking1.9.1 --- .12+
status1.9.1 --- .12-fixed

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: [sg:critical?] keep hidden until 566141 is fixed [ccbr][qa-needs-STR])

Attachments

(4 files, 2 obsolete files)

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.
blocking2.0: ? → final+
Whiteboard: [sg:critical?]
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.
In unrelated news, we really should remove setting __proto__. We have Object.create now.
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.
Summary: GC hazard with lookupProperty callers with Proxies → GC hazard with lookupProperty callers with non-native objects
Blocks: 569162
No longer blocks: harmony:proxies
No longer blocks: 569162
Depends on: 569162
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.
blocking1.9.1: --- → ?
status1.9.1: ? → ---
(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
Attached patch patch (obsolete) — Splinter Review
This patch fixes the specific rooting bug here. I am still auditing the remaining call sites.
Assignee: general → gal
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #448317 - Attachment is obsolete: true
Attachment #448319 - Flags: review?(brendan)
Comment on attachment 448319 [details] [diff] [review]
patch

Great, thanks.

/be
Attachment #448319 - Flags: review?(brendan) → review+
(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.
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
(looking into #9)
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:patch][ccbr]
Almost fell off my radar. Looking again.
Andreas, any update?
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
We can drop 1.9.3 blocking here. 1.9.3 has a conservative stack scanner, which will detect these roots.
We should probably check this in before we check in the stack scanner.
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.
can we land this?
gal, what is plan for this patch?
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?
Igor, what do you think?
blocking2.0: final+ → ---
(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.
Assignee: gal → igor
any verdict?
(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.
blocking2.0: --- → -
I guess we should backport it, then
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.
blocking1.9.1: needed → ?
blocking1.9.2: needed → ?
Version: Other Branch → 1.9.1 Branch
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
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .9+
Attached patch fix for 192Splinter Review
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.
Attachment #448319 - Attachment is obsolete: true
Attachment #464892 - Flags: review?(brendan)
Attachment #464892 - Flags: review?(brendan) → review?(jorendorff)
(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.
Whiteboard: [sg:critical?][critsmash:patch][ccbr] → [sg:critical?][needs r=jorendorff][critsmash:patch][ccbr]
Comment on attachment 464892 [details] [diff] [review]
fix for 192

Stealing. Jorendorff isn't available until Monday.
Attachment #464892 - Flags: review?(jorendorff) → review+
Thanks Andreas! Is the 1.9.1 patch more involved?
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
Attachment #464892 - Flags: approval1.9.2.10?
Attachment #464892 - Flags: approval1.9.1.13?
Attached patch fix for 191Splinter Review
191 needs a separated fix to account for OBJ_LOOKUP_PROPERTY change on later branches.
Attachment #464892 - Attachment is obsolete: true
Attachment #466242 - Flags: review+
Attachment #466242 - Flags: approval1.9.1.13?
Attachment #464892 - Flags: approval1.9.2.10?
Attachment #464892 - Flags: approval1.9.1.13?
Attachment #464892 - Attachment is obsolete: false
Attachment #464892 - Flags: approval1.9.2.10?
Attachment #464892 - Flags: approval1.9.2.10? → approval1.9.2.9+
Attachment #466242 - Flags: approval1.9.1.13? → approval1.9.1.12+
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!
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?
Whiteboard: [sg:critical?][needs r=jorendorff][critsmash:patch][ccbr] → [sg:critical?][needs r=jorendorff][critsmash:patch][ccbr][qa-needs-STR]
(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.
Attachment #469408 - Flags: review?(igor) → review+
Whiteboard: [sg:critical?][needs r=jorendorff][critsmash:patch][ccbr][qa-needs-STR] → [sg:critical?] keep hidden until 566141 is fixed [ccbr][qa-needs-STR]
Attached patch Patch for 1.9.0Splinter Review
Attachment #471890 - Flags: review?(igor)
Attachment #471890 - Flags: review?(igor) → review+
Group: core-security
You need to log in before you can comment on or make changes to this bug.