GC hazard with lookupProperty callers with non-native objects

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

1.9.1 Branch
Points:
---

Firefox Tracking Flags

(blocking2.0 -, status2.0 unaffected, blocking1.9.2 .9+, status1.9.2 .9-fixed, blocking1.9.1 .12+, status1.9.1 .12-fixed)

Details

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

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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.

Updated

7 years ago
blocking2.0: ? → final+

Updated

7 years ago
Whiteboard: [sg:critical?]

Comment 1

7 years ago
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

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

Comment 3

7 years ago
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.

Updated

7 years ago
Summary: GC hazard with lookupProperty callers with Proxies → GC hazard with lookupProperty callers with non-native objects

Updated

7 years ago
Blocks: 569162

Updated

7 years ago
No longer blocks: 546590

Updated

7 years ago
No longer blocks: 569162
Depends on: 569162

Comment 4

7 years ago
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.

Updated

7 years ago
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

Comment 6

7 years ago
Created attachment 448317 [details] [diff] [review]
patch

This patch fixes the specific rooting bug here. I am still auditing the remaining call sites.
Assignee: general → gal

Comment 7

7 years ago
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.
Attachment #448317 - Attachment is obsolete: true

Updated

7 years ago
Attachment #448319 - Flags: review?(brendan)
Comment on attachment 448319 [details] [diff] [review]
patch

Great, thanks.

/be
Attachment #448319 - Flags: review?(brendan) → review+
(Assignee)

Comment 9

7 years ago
(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.

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]

Comment 10

7 years ago
(looking into #9)
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Updated

7 years ago
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:patch][ccbr]

Comment 11

7 years ago
Almost fell off my radar. Looking again.

Comment 12

7 years ago
Andreas, any update?
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed

Comment 13

7 years ago
We can drop 1.9.3 blocking here. 1.9.3 has a conservative stack scanner, which will detect these roots.
status2.0: --- → unaffected

Comment 14

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

Comment 15

7 years ago
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

7 years ago
can we land this?

Comment 17

7 years ago
gal, what is plan for this patch?

Comment 18

7 years ago
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

7 years ago
Igor, what do you think?
blocking2.0: final+ → ---
(Assignee)

Comment 20

7 years ago
(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

Comment 21

7 years ago
any verdict?
(Assignee)

Comment 22

7 years ago
(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.

Updated

7 years ago
blocking2.0: --- → -

Comment 23

7 years ago
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

Comment 25

7 years ago
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

Updated

7 years ago
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .9+
(Assignee)

Comment 26

7 years ago
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.
Attachment #448319 - Attachment is obsolete: true
Attachment #464892 - Flags: review?(brendan)
(Assignee)

Updated

7 years ago
Attachment #464892 - Flags: review?(brendan) → review?(jorendorff)
(Assignee)

Comment 27

7 years ago
(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 28

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

Stealing. Jorendorff isn't available until Monday.
Attachment #464892 - Flags: review?(jorendorff) → review+

Comment 29

7 years ago
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
(Assignee)

Updated

7 years ago
Attachment #464892 - Flags: approval1.9.2.10?
Attachment #464892 - Flags: approval1.9.1.13?
(Assignee)

Comment 31

7 years ago
Created attachment 466242 [details] [diff] [review]
fix for 191

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?
(Assignee)

Updated

7 years ago
Attachment #464892 - Attachment is obsolete: false
Attachment #464892 - Flags: approval1.9.2.10?

Updated

7 years ago
Attachment #464892 - Flags: approval1.9.2.10? → approval1.9.2.9+

Updated

7 years ago
Attachment #466242 - Flags: approval1.9.1.13? → approval1.9.1.12+

Comment 32

7 years ago
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!
(Assignee)

Comment 33

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3cdb069181db
Status: NEW → RESOLVED
Last Resolved: 7 years ago
status1.9.1: wanted → .12-fixed
status1.9.2: wanted → .9-fixed
Resolution: --- → FIXED
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]
(Assignee)

Comment 35

7 years ago
(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

7 years ago
Created attachment 469408 [details] [diff] [review]
1.8.0 version
Attachment #469408 - Flags: review?(igor)
(Assignee)

Updated

7 years ago
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]
Created attachment 471890 [details] [diff] [review]
Patch for 1.9.0
Attachment #471890 - Flags: review?(igor)
(Assignee)

Updated

7 years ago
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.