The default bug view has changed. See this FAQ.

GC hazards in JS_SetWatchPoint with proxies

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

Firefox Tracking Flags

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

Details

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

Attachments

(4 attachments)

(Assignee)

Description

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

Comment 1

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

Updated

7 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 2

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

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

Comment 4

7 years ago
http://hg.mozilla.org/tracemonkey/rev/ea4ad4f59286
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey

Updated

7 years ago
blocking2.0: ? → beta1+

Updated

7 years ago
Whiteboard: [sg:critical?] fixed-in-tracemonkey → [sg:critical?] fixed-in-tracemonkey [critsmash:patch]
blocking1.9.1: --- → needed
blocking1.9.2: ? → needed
status1.9.1: ? → wanted
status1.9.2: --- → wanted

Comment 5

7 years ago
http://hg.mozilla.org/mozilla-central/rev/ea4ad4f59286
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
blocking1.9.1: needed → .13+
blocking1.9.2: needed → .10+
(Assignee)

Comment 6

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

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

Comment 7

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

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

Comment 10

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

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

Comment 11

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

Updated

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