316885 - CVE-2006-0292 Unrooted access in jsinterp.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

The implementation of post++/-- in jsinterp.c contains the following code:

#define NONINT_INCREMENT_OP_MIDDLE()                                          \
    JS_BEGIN_MACRO                                                            \
        VALUE_TO_NUMBER(cx, rval, d);                                         \
        if (cs->format & JOF_POST) {                                          \
            rtmp = rval;                                                      \
            if (!JSVAL_IS_NUMBER(rtmp)) {                                     \
                ok = js_NewNumberValue(cx, d, &rtmp);                         \
                if (!ok)                                                      \
                    goto out;                                                 \
            }                                                                 \
            (cs->format & JOF_INC) ? d++ : d--;                               \
            ok = js_NewNumberValue(cx, d, &rval);                             \
        } else {                                                              \
            (cs->format & JOF_INC) ? ++d : --d;                               \
            ok = js_NewNumberValue(cx, d, &rval);                             \
            rtmp = rval;                                                      \
        }                                                                     \
        if (!ok)                                                              \
            goto out;                                                         \
    JS_END_MACRO

                NONINT_INCREMENT_OP_MIDDLE();
            }

            fp->flags |= JSFRAME_ASSIGNING;
            CACHED_SET(OBJ_SET_PROPERTY(cx, obj, id, &rval));
            fp->flags &= ~JSFRAME_ASSIGNING;
            if (!ok)
                goto out;
            sp += i;
            PUSH_OPND(rtmp);


This is not GC-safe when rval is not a number but is convertable to number that does not fit int JSVAL. In this case js_NewNumberValue(cx, d, &rtmp) would store newly allocated double in rtmp. Later js_NewNumberValue(cx, d, &rval) would displace the double from newborn array by another double value. 

Now if OBJ_SET_PROPERTY would trigger GC, then the double referenced by rtmp would be GC-ed and later stored on the stack. 

The following example demonstartes  how the bug can crash js shell:

var str_with_num = "0.1";

var obj = { 
	elem getter: function() {
		return str_with_num;
	},
	elem setter: function(value) {
		gc();
	}

};

var expected = Number(str_with_num);
var actual = obj.elem++;
gc();
if (actual !== expected)
	print("BAD result: "+actual);

Comment 1

[Igor Bukanov]

Here is  another bug in jsinterp.c. The macro RELATIONAL_OP contains the lines:

            VALUE_TO_PRIMITIVE(cx, lval, JSTYPE_NUMBER, &lval);               \
            VALUE_TO_PRIMITIVE(cx, rval, JSTYPE_NUMBER, &rval);               \
            if (JSVAL_IS_STRING(lval) && JSVAL_IS_STRING(rval)) {             \
                str  = JSVAL_TO_STRING(lval);                                 \
                str2 = JSVAL_TO_STRING(rval);                                 \
                cond = js_CompareStrings(str, str2) OP 0;                     \
            } else {                                                          \
                VALUE_TO_NUMBER(cx, lval, d);                                 \
                VALUE_TO_NUMBER(cx, rval, d2);                                \
                cond = JSDOUBLE_COMPARE(d, OP, d2, JS_FALSE);                 \
            }                                                                 \

If rval->number conversion can call arbitrary JS that would force GC of lval and potentially crash the engine like the following demo for JS shell/browser:

var str = "test"

var lval = { 
	valueOf: function() {
		return str+"1";
	}
};

var ONE = 1;

var rval = { 
	valueOf: function() {
		// Make sure that the result of the previous lval.valueOf 
		// is not GC-rooted.
		var tmp = "x"+lval;
		if (typeof gc == "function")
			gc();
		for (var i = 0; i != 40000; ++i) {
			tmp = 1e100 / ONE;
		}
		return str;
	}
};

var expected = (str+"1" > str);
var actual = (lval > rval);
if (expected !== actual)
	print("BUG");

Comment 2

[Igor Bukanov]

The problematic pattern of double-call to VALUE_TO_PRIMITIVE is also present in JSOP_ADD with a test case that is almost carbon copy of the above but with ">" replaced by "+" at the end:

var str = "test"

var lval = { 
	valueOf: function() {
		return str+"1";
	}
};

var ONE = 1;

var rval = { 
	valueOf: function() {
		// Make sure that the result of the previous lval.valueOf 
		// is not GC-rooted.
		var tmp = "x"+lval;
		if (typeof gc == "function")
			gc();
		for (var i = 0; i != 40000; ++i) {
			tmp = 1e100 / ONE;
		}
		return str;
	}
};

var expected = (str+"1")+str;
var actual = lval+rval;
if (expected !== actual)
	print("BUG");

Comment 3

[Igor Bukanov]

> [sg:critical]

AFAICS the bugs together can be used to execute arbitrary code on platforms without non-exec heap protection. 

The bug from comment 1 allows to subvert a reference to a double value to point to string and read JSString bits via interpreting the double value. When used repeatedly it allows to build up a chain of strings that would resemble JSObject structure.

Then the second bug is used to subvert an object reference to point to the designed string triggering arbitrary code execution when VALUE_TO_NUMBER would run just build code through defaultValue hook. 

For details of reference subversion see https://bugzilla.mozilla.org/show_bug.cgi?id=311497#c10.

Comment 4

[Brendan Eich [:brendan]]

Bob, let's get this and anything like it for JS1.6 RC1.  Thanks,

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fixes

Minimal patch, let me know if I missed anything.  Thanks,

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 203484 [details] [diff] [review]
fixes

>@@ -2901,26 +2902,27 @@ js_Interpret(JSContext *cx, jsbytecode *
>                 ok = ops->concatenate(cx, obj2, rval, &rval);
>                 if (!ok)
>                     goto out;
>                 sp--;
>                 STORE_OPND(-1, rval);
>                 break;
>             }
> #endif
>-            VALUE_TO_PRIMITIVE(cx, lval, JSTYPE_VOID, &ltmp);
>-            VALUE_TO_PRIMITIVE(cx, rval, JSTYPE_VOID, &rtmp);
>-            if ((cond = JSVAL_IS_STRING(ltmp)) || JSVAL_IS_STRING(rtmp)) {
>+            VALUE_TO_PRIMITIVE(cx, lval, JSTYPE_VOID, &lval);
>+            sp[-2] = lval;      /* protect from GC */
>+            VALUE_TO_PRIMITIVE(cx, rval, JSTYPE_VOID, &rval);
>+            if ((cond = JSVAL_IS_STRING(lval)) || JSVAL_IS_STRING(rval)) {
>                 SAVE_SP(fp);
>                 if (cond) {
>-                    str = JSVAL_TO_STRING(ltmp);
>-                    ok = (str2 = js_ValueToString(cx, rtmp)) != NULL;
>+                    str = JSVAL_TO_STRING(lval);
>+                    ok = (str2 = js_ValueToString(cx, rval)) != NULL;
>                 } else {
>-                    str2 = JSVAL_TO_STRING(rtmp);
>-                    ok = (str = js_ValueToString(cx, ltmp)) != NULL;
>+                    str2 = JSVAL_TO_STRING(rval);
>+                    ok = (str = js_ValueToString(cx, lval)) != NULL;
>                 }
>                 if (!ok)
>                     goto out;
>                 str = js_ConcatStrings(cx, str, str2);
>                 if (!str) {
>                     ok = JS_FALSE;
>                     goto out;
>                 }

See ECMA-262 Edition 3 11.6.1 The Addition Operator ( + ), Steps 8 and 9.  We've had a bug forever where the results of ToPrimitive were not passed to ToNumber -- instead the original left and right operand values were reconverted.  Eek.  I hope no one was counting on this.  I propose to fix it now in this patch, for both GC safety and ECMA-262 conformance.

>@@ -3218,39 +3220,49 @@ js_Interpret(JSContext *cx, jsbytecode *
>                     (cs->format & JOF_INC) ? (rval += 2) : (rval -= 2);
>                     rtmp = rval;
>                 }
>             } else {
> 
> /*
>  * Initially, rval contains the value to increment or decrement, which is not
>  * yet converted.  As above, the expression result goes in rtmp, the updated
>- * value goes in rval.
>+ * value goes in rval.  Our caller must set vp to point at a GC-rooted jsval
>+ * in which we home rtmp, to protect it from GC in case the unconverted rval
>+ * is not a number.
>  */
> #define NONINT_INCREMENT_OP_MIDDLE()                                          \
>     JS_BEGIN_MACRO                                                            \
>         VALUE_TO_NUMBER(cx, rval, d);                                         \
>         if (cs->format & JOF_POST) {                                          \
>             rtmp = rval;                                                      \
>             if (!JSVAL_IS_NUMBER(rtmp)) {                                     \
>                 ok = js_NewNumberValue(cx, d, &rtmp);                         \
>                 if (!ok)                                                      \
>                     goto out;                                                 \
>+                *vp = rtmp;                                                   \
>             }                                                                 \
>             (cs->format & JOF_INC) ? d++ : d--;                               \
>             ok = js_NewNumberValue(cx, d, &rval);                             \
>         } else {                                                              \
>             (cs->format & JOF_INC) ? ++d : --d;                               \
>             ok = js_NewNumberValue(cx, d, &rval);                             \
>             rtmp = rval;                                                      \
>         }                                                                     \
>         if (!ok)                                                              \
>             goto out;                                                         \
>     JS_END_MACRO
> 
>+                if (i == 0) {
>+                    /* Must push early to protect expression result from GC. */
>+                    ++sp;
>+                    i = -1;
>+                    SAVE_SP(fp);
>+                }
>+                vp = sp - 1;
>                 NONINT_INCREMENT_OP_MIDDLE();
>             }
> 
>             fp->flags |= JSFRAME_ASSIGNING;
>             CACHED_SET(OBJ_SET_PROPERTY(cx, obj, id, &rval));
>             fp->flags &= ~JSFRAME_ASSIGNING;
>             if (!ok)
>                 goto out;

The subtlety here is that, if *vp is set to protect rtmp, we overwrite either idle stack space (in the JSOP_INCNAME etc. cases, which push their result after popping nothing, so we know there's at least one stack slot available), or the object in which the literal id names a property to increment or decrement (JSOP_INCPROP etc., oops...), or the stacked id-as-jsval of the element being incremented or decremented (JSOP_INCELEM etc.).

I knew I had something not quite right (see "oops" above), but I needed to attach the patch and add these comments to find it.  New patch coming in bit.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: fixes, v2

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 203500 [details] [diff] [review]
fixes, v2

Wrong patch.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: fixes, v3

Comment 10

[Brendan Eich [:brendan]]

Attachment: fixes, v4

The JSOP_ADD case must protect ToPrimitive of rval as well as of lval.  I decided to make parallel changes in the other cases.

/be

Comment 11

[Igor Bukanov]

Comment on attachment 203608 [details] [diff] [review]
fixes, v4

I am curious why with the patch v3 unprotected rval in those cases is GC unsafe? AFAICS this is only so if defaultValue hook for lval would return an object. But this is a violation of of defaultValue contract, right? If so perhaps an assert can be added (later!) to insist on this?

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 203608 [details] [diff] [review]
fixes, v4

sr=shaver

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (From update of attachment 203608 [details] [diff] [review] [edit])
> I am curious why with the patch v3 unprotected rval in those cases is GC
> unsafe? AFAICS this is only so if defaultValue hook for lval would return an
> object. But this is a violation of of defaultValue contract, right? If so
> perhaps an assert can be added (later!) to insist on this?

The reason was for JSOP_ADD, where rval might be converted from an object value to string, but not be rooted by sp[-1].  If lval were not a string, it would be converted, which might trigger a GC that would collect the rval conversion.

I decided to make the pattern match throughout the VALUE_TO_PRIMITIVE cases.

/be

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> The reason was for JSOP_ADD, where rval might be converted from an object value
> to string, but not be rooted by sp[-1].

But rval would be rooted either through newborn array or via lastInternalResult. Since lval is not an object, its conversion to string can cause no more then one string allocation. This would make rval unrooted but there is no unrootes access here since js_ConcatStrings does not access rval after allocating the result.

Note that this is not an argument against rooting rval, since this has added benefits of removing no longer used references from GC-scannable area ASAP besides making code bulletproof. I just would like to have test case if there is a bug so it would not be forgotten.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (In reply to comment #13)
> > The reason was for JSOP_ADD, where rval might be converted from an object value
> > to string, but not be rooted by sp[-1].
> 
> But rval would be rooted either through newborn array or via
> lastInternalResult.

I know, but we have had bugs where JS_ClearNewbornRoots was called from a finalizer, IIRC (bug 308678, treated by first patch in bug 307317).  The number of hooks and APIs, their age, and the size of the community of embedders makes me want to be paranoid here.

> Note that this is not an argument against rooting rval, since this has added
> benefits of removing no longer used references from GC-scannable area ASAP
> besides making code bulletproof. I just would like to have test case if there
> is a bug so it would not be forgotten.

No bug known.  If you know of other places that are less paranoid, but that seem to have the same "API abuse" hazard, please file bugs.  Thanks,

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: patch i'm checking in

To avoid a spurious gcc warning about "lval might be used uninitialized", I reverted RELATIONAL_OP's macro definition so it does not pass &sp[-2] to VALUE_TO_PRIMITIVE and reload lval from sp[-2] after.  Since that macro body does not need to root rval, I reverted the JSVAL_TO_PRIMITIVE call for rval too.  The warning was peculiar to lval, which is strange.  Could the &sp[-2] (vs. &sp[-1] in the rval case) have mattered?  Yet other such code sequenes did not merit the warning.  Why not?

Anyway, the code is correct, and I'm tired of guessing what bugs GCC might have.

/be

Comment 17

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-316885-01.js

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-316885-02.js

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-316885-03.js

Comment 21

[Bob Clary [:bc] (inactive)]

verified with cvs debug trunk builds on windows xp and linux 2005-11-23

Comment 22

[Bob Clary [:bc] (inactive)]

This caused bug 317714

Comment 23

[Bob Clary [:bc] (inactive)]

testcase+ to get this off my radar. when this is made public, i will check in
the test.

Comment 24

[Daniel Veditz [:dveditz]]

We'd like to approve this, but it apparently requires one of the patches from bug 317714 as well. Assuming the second patch, but they're different enough to want clarity.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 203864 [details] [diff] [review]
patch i'm checking in

a=dveditz for drivers

Comment 26

[Bob Clary [:bc] (inactive)]

v 2006-01-11 1.8.0.1, 1.8.1 windows/linux/mac

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Backport to 1.0.x

Here's my attempt at a backport to aviary 1.0.x.  I've tested and this fixes both the original testcase and the crash regression reported in bug 317714.

I spent a bit of time reading the relevant bits of code and looked through past patches to the affected code, and I don't believe the last hunk of attachment 203864 [details] [diff] [review] is necessary as the do_nonint_fast_global_incop was not introduced until revision 3.137 of jsinterp.c (part of bug 169559) and the aviary branch uses a branch based on 3.136 which doesn't have this code at all.  If you could sanity check this brendan as it was your patch, that would be great.  I'm requesting r+sr=, but feel free to have someone else r= it first if you prefer.

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 210090 [details] [diff] [review]
Backport to 1.0.x

Looks right to me.  Igor if you have time to review, that would be good -- this was a bit of a back-port.

/be

Comment 29

[Igor Bukanov]

Comment on attachment 210090 [details] [diff] [review]
Backport to 1.0.x

No more double unroots for the interpreter in 1.0.8 as well.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 210090 [details] [diff] [review]
Backport to 1.0.x

a=dveditz for a1.0/1.7 branches

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 210090 [details] [diff] [review]
Backport to 1.0.x

checked in to aviary101/moz17 branches

Comment 32

[Rich Painter]

Advisory 2006-01 http://www.mozilla.org/security/announce/mfsa2006-01.html indicates that this flaw is also in Mozilla but no references seem to be here about fixing this.  This Advisory does indicate to disable javascript in Mail which is commonly done and prudent.  However, it does not address this for Browser and javascript is needed by just about everyone to access at least one web site.  Hence, disabling this in Browser to work around this flaw is impractical.

Is someone fixing this for Mozilla Suite and when will it be available?

thanks,
r

Comment 33

[Bob Clary [:bc] (inactive)]

verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac

Comment 34

[Bob Clary [:bc] (inactive)]

v moz 1.7.13 window 20060221

Comment 35

[Bob Clary [:bc] (inactive)]

Checking in regress-316885-01.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-316885-01.js,v  <--  regress-316885-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-316885-02.js,v
done
Checking in regress-316885-02.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-316885-02.js,v  <--  regress-316885-02.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-316885-03.js,v
done
Checking in regress-316885-03.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-316885-03.js,v  <--  regress-316885-03.js
initial revision: 1.1
done