CVE-2006-0292 Unrooted access in jsinterp.c

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: brendan)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha1
js1.6, verified1.7.13, verified1.8.0.1, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8.0.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] Which 317714 patch?)

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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);
Flags: blocking1.8.0.1+
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
Whiteboard: [sg:critical]
(Reporter)

Comment 1

12 years ago
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");
(Reporter)

Comment 2

12 years ago
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");

(Reporter)

Comment 3

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

Comment 4

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

/be
Assignee: general → brendan
Keywords: js1.6
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 5

12 years ago
Created attachment 203484 [details] [diff] [review]
fixes

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

/be
Attachment #203484 - Flags: superreview?(shaver)
Attachment #203484 - Flags: review?(igor.bukanov)
(Assignee)

Updated

12 years ago
Blocks: 309169
(Assignee)

Comment 6

12 years ago
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
Attachment #203484 - Attachment is obsolete: true
Attachment #203484 - Flags: superreview?(shaver)
Attachment #203484 - Flags: review?(igor.bukanov)
(Assignee)

Comment 7

12 years ago
Created attachment 203500 [details] [diff] [review]
fixes, v2
Attachment #203500 - Flags: superreview?(shaver)
Attachment #203500 - Flags: review?(igor.bukanov)
(Assignee)

Comment 8

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

Wrong patch.

/be
Attachment #203500 - Attachment is obsolete: true
Attachment #203500 - Flags: superreview?(shaver)
Attachment #203500 - Flags: review?(igor.bukanov)
(Assignee)

Comment 9

12 years ago
Created attachment 203502 [details] [diff] [review]
fixes, v3
Attachment #203502 - Flags: superreview?(shaver)
Attachment #203502 - Flags: review?(igor.bukanov)
(Reporter)

Updated

12 years ago
Attachment #203502 - Flags: review?(igor.bukanov) → review+
(Assignee)

Updated

12 years ago
Attachment #203502 - Flags: superreview?(shaver)
Attachment #203502 - Flags: review+
(Assignee)

Comment 10

12 years ago
Created attachment 203608 [details] [diff] [review]
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
Attachment #203608 - Flags: superreview?(shaver)
Attachment #203608 - Flags: review?(igor.bukanov)
(Reporter)

Comment 11

12 years ago
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?
Attachment #203608 - Flags: review?(igor.bukanov) → review+
Comment on attachment 203608 [details] [diff] [review]
fixes, v4

sr=shaver
Attachment #203608 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 13

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

Comment 14

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

(Assignee)

Comment 15

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

Comment 16

12 years ago
Created attachment 203864 [details] [diff] [review]
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
Attachment #203864 - Flags: superreview+
Attachment #203864 - Flags: review+
(Assignee)

Comment 17

12 years ago
Fixed.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 18

12 years ago
Created attachment 204094 [details]
js1_5/Regress/regress-316885-01.js

Comment 19

12 years ago
Created attachment 204095 [details]
js1_5/Regress/regress-316885-02.js

Comment 20

12 years ago
Created attachment 204096 [details]
js1_5/Regress/regress-316885-03.js

Comment 21

12 years ago
verified with cvs debug trunk builds on windows xp and linux 2005-11-23
Status: RESOLVED → VERIFIED
Flags: testcase?

Comment 22

12 years ago
This caused bug 317714
Blocks: 317714
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+

Comment 23

12 years ago
testcase+ to get this off my radar. when this is made public, i will check in
the test.
Flags: testcase? → testcase+
(Assignee)

Updated

12 years ago
Attachment #203864 - Flags: approval1.8.1?
Attachment #203864 - Flags: approval1.8.0.1?
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.
Whiteboard: [sg:critical] → [sg:critical] Which 317714 patch?
Comment on attachment 203864 [details] [diff] [review]
patch i'm checking in

a=dveditz for drivers
Attachment #203864 - Flags: approval1.8.1?
Attachment #203864 - Flags: approval1.8.1+
Attachment #203864 - Flags: approval1.8.0.1?
Attachment #203864 - Flags: approval1.8.0.1+
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.0.1, fixed1.8.1

Comment 26

12 years ago
v 2006-01-11 1.8.0.1, 1.8.1 windows/linux/mac
Keywords: fixed1.8.0.1, fixed1.8.1 → verified1.8.0.1, verified1.8.1

Updated

12 years ago
Summary: Unrooted access in jsinterp.c → CVE-2006-0292 Unrooted access in jsinterp.c
Created attachment 210090 [details] [diff] [review]
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.
Attachment #210090 - Flags: superreview?(brendan)
Attachment #210090 - Flags: review?(brendan)
(Assignee)

Comment 28

12 years ago
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
Attachment #210090 - Flags: superreview?(brendan)
Attachment #210090 - Flags: superreview+
Attachment #210090 - Flags: review?(igor.bukanov)
Attachment #210090 - Flags: review?(brendan)
Attachment #210090 - Flags: review+
(Reporter)

Comment 29

12 years ago
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.
Attachment #210090 - Flags: review?(igor.bukanov) → review+
Comment on attachment 210090 [details] [diff] [review]
Backport to 1.0.x

a=dveditz for a1.0/1.7 branches
Attachment #210090 - Flags: approval1.7.13+
Attachment #210090 - Flags: approval-aviary1.0.8+
Keywords: fixed-aviary1.0.8, fixed1.7.13
Comment on attachment 210090 [details] [diff] [review]
Backport to 1.0.x

checked in to aviary101/moz17 branches
Group: security

Comment 32

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

12 years ago
verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8

Comment 34

12 years ago
v moz 1.7.13 window 20060221
Keywords: fixed1.7.13 → verified1.7.13

Comment 35

11 years ago
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
You need to log in before you can comment on or make changes to this bug.