The default bug view has changed. See this FAQ.

GC hazard in MarkSharpObjects

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .11+, status1.9.2 .11-fixed, blocking1.9.1 .14+, status1.9.1 .14-fixed)

Details

(Whiteboard: [sg:critical][critsmash:investigating] fixed-in-tracemonkey)

Attachments

(5 attachments)

(Assignee)

Description

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

Updated

7 years ago
Whiteboard: [sg:critical?]

Comment 1

7 years ago
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.
Whiteboard: [sg:critical?] → [sg:investigate?]
"we are eliminating shared objects"? You mean eliminating JSProperty * from the JSObjectOps hooks?

/be
Group: core-security
Whiteboard: [sg:investigate?]

Comment 3

7 years ago
We are eliminating sharing native objects across threads.
gal, we don't use [sg:investigate] anymore. Better to just leave it blank (and the bug security-sensitive) if you're not sure.
(Assignee)

Comment 5

7 years ago
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.
Group: core-security
(Assignee)

Comment 6

7 years ago
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.
Summary: New proxy code would expose a hazard in MarkSharpObjects → GC hazard in MarkSharpObjects
(Assignee)

Comment 7

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

Comment 8

7 years ago
(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.
Duplicate of this bug: 523933
Igor, so would you say this is not sg:critical based on Comment 8 ?
Whiteboard: [sg:critical?][critsmash:investigating]
(Assignee)

Comment 11

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

Comment 12

7 years ago
(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.
Who should own this?  Need an owner here.

Thanks a bunch, Igor.
Whiteboard: [sg:critical?][critsmash:investigating] → [sg:critical][critsmash:investigating]

Comment 14

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

Updated

7 years ago
Assignee: general → igor
Blocking 1.9.3 final as it's an sg:crit.
blocking2.0: --- → final+
(Assignee)

Updated

7 years ago
Blocks: 566818
(Assignee)

Updated

7 years ago
No longer blocks: 566818
(Assignee)

Comment 16

7 years ago
Created attachment 446189 [details] [diff] [review]
v1

This is what I am testing
(Assignee)

Updated

7 years ago
Attachment #446189 - Flags: review?(brendan)
(Assignee)

Comment 17

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

Comment 18

7 years ago
brendan: review ping
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
Attachment #446189 - Flags: review?(brendan) → review+
(Assignee)

Comment 20

7 years ago
http://hg.mozilla.org/tracemonkey/rev/54acf298872b - with the inverted isNative test.

Comment 21

7 years ago
fixed in tracemonkey?
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:investigating] fixed-in-tracemonkey
(Assignee)

Comment 22

7 years ago
(In reply to comment #21)
> fixed in tracemonkey?

yes

Comment 23

7 years ago
http://hg.mozilla.org/mozilla-central/rev/54acf298872b
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Depends on: 568485
(Assignee)

Comment 24

7 years ago
This bug should be a blocker on branches. AFAICS with liveconnect one can build an exploit here.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
(Assignee)

Comment 25

7 years ago
Created attachment 465981 [details] [diff] [review]
fix for 192

192 backport follows the logic of TM patch adjusted for older internal API.
Attachment #465981 - Flags: review?(gal)

Updated

7 years ago
Attachment #465981 - Flags: review?(gal) → review+
(Assignee)

Comment 26

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

191 needs a separated fix to account for OBJ_LOOKUP_PROPERTY change on later
branches.
Attachment #466243 - Flags: review+
Attachment #466243 - Flags: approval1.9.1.13?
(Assignee)

Updated

7 years ago
Attachment #466243 - Attachment description: fi for 191 → fix for 191
(Assignee)

Comment 27

7 years ago
Created attachment 466247 [details]
plain diff between 192 and 191 patches
Blocking for the next set of branch releases, the current ones are in (slushy) code-freeze as we try to wrap up.
blocking1.9.1: ? → .13+
blocking1.9.2: ? → .10+
status1.9.1: --- → wanted
status1.9.2: --- → wanted
(Assignee)

Comment 29

7 years ago
(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 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?
Attachment #466243 - Flags: approval1.9.1.13? → approval1.9.1.13+
(Assignee)

Updated

7 years ago
Attachment #465981 - Flags: approval1.9.2.10?
Comment on attachment 465981 [details] [diff] [review]
fix for 192

Approved for 1.9.2.10, a=dveditz for release-drivers
Attachment #465981 - Flags: approval1.9.2.10? → approval1.9.2.10+
(Assignee)

Comment 32

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f070c6116d9d

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a3974b15591b
status1.9.1: wanted → .14-fixed
status1.9.2: wanted → .11-fixed

Comment 33

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

Updated

7 years ago
Attachment #481169 - Flags: review?(igor) → review+
Group: core-security
You need to log in before you can comment on or make changes to this bug.