325425 - jsxml.c: Bad assumptions about js_ConstructObject

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

1. In several places when constructing QName and Namespace instances the code form jsxml.c assumes that the result of js_ConstructObject is proper QName/Namespace\
 object. But since QName and Namespace constructors can be replaced or hidden via "with" statement, that assumption is wrong. Here is a trivial example that demon\
strates the bug:

js> QName = function() { }; <xml/>.elements("")
Segmentation fault

The bug allows, for example, for a script for cast as a pointer to JSXMLNamespace or JSXMLQName a pointer to JSScript. This would lead to reading of pointer to in\
terpreter bytecode as a pointer to JSObject. Given the rather dense nature of the bytecode, this provides a wide range of possible value.

2. ToXMLName calls js_ConstructObject with a pointer to jsval as argument array. Since jsval is constructed from ValueToString, it can be rooted only through last\
InternalResult. With prototype getter defined that can be made unrooted and GC-ed after which it would be passed to the constructor.

Comment 1

[Daniel Veditz [:dveditz]]

Assuming this can be abused

Comment 2

[Daniel Veditz [:dveditz]]

"(I am) assuming..." to be clear, not doubting.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I'll take this.

Comment 4

[Igor Bukanov]

Here are more problems with js_ConstructObject usage when code from jsxml.c calls it passing unrooted arguments:

1. xml_setNamespace calls js_ConstructObject with unrooted nsobj obtained from the previous call to js_ConstructObject.

2. The interpreter calls js_SetDefaultXMLNamespace with jsval popped up from the stack and cut from its roots. The value is then passed to js_ConstructObject.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Here are more problems with js_ConstructObject usage when code from jsxml.c
> calls it passing unrooted arguments:

js_ConstructObject should use a JSTempValueRooter to protect its argv.  I think that would fix these cases.  Any other hazards would have to exist in the callers of js_ConstructObject, independent of its hazards.

Blake and I talked about this bug, it bites more than jsxml.c.  It's an old API bug: JS_ConstructObject should throw a TypeError if the constructor substitutes an object of a different class from the clasp passed into it.  Otherwise many users of {js,JS}_ConstructObject are hacked.  Changing the API to throw changes it in an incompatible way, but fixes bad exploitable bugs, so it's a good and necessary compatibility break.

Thanks for finding this.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix both cases

Brendan discussed rooting argv in js_ConstructObject (freeing callers from having to do the same) but I think that an argv parameter is implicity rooted by name elsewhere, so it should be up to the caller in this case to ensure that argv is in fact rooted.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 210552 [details] [diff] [review]
Fix both cases

>@@ -2158,31 +2158,50 @@ js_ConstructObject(JSContext *cx, JSClas
>             goto out;
>         }
>         if (JSVAL_IS_OBJECT(rval))
>             proto = JSVAL_TO_OBJECT(rval);
>     }
> 
>     obj = js_NewObject(cx, clasp, proto, parent);
>     if (!obj)
>         goto out;
> 
>-    if (!js_InternalConstruct(cx, obj, cval, argc, argv, &rval)) {
>-        cx->newborn[GCX_OBJECT] = NULL;
>-        obj = NULL;
>+    if (!js_InternalConstruct(cx, obj, cval, argc, argv, &rval))
>+        goto bad;
>+
>+    if (JSVAL_IS_PRIMITIVE(rval))
>         goto out;
>+    obj = JSVAL_TO_OBJECT(rval);

Note that obj is by definition non-null here.

>+
>+    /*
>+     * If the given class has both the JSCLASS_HAS_PRIVATE and the
>+     * JSCLASS_CONSTRUCT_PROTOTYPE flags, then the class should have its private
>+     * data set. If it doesn't, then it means the constructor was replaced, and
>+     * we should throw a typerr.
>+     */
>+    if ((obj && OBJ_GET_CLASS(cx, obj) != clasp) ||

So don't null check here.

>+        (!(~clasp->flags & (JSCLASS_HAS_PRIVATE |
>+                            JSCLASS_CONSTRUCT_PROTOTYPE)) &&
>+         !JS_GetPrivate(cx, obj))) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                             JSMSG_WRONG_CONSTRUCTOR, clasp->name);
>+        goto bad;
>     }
> js_SetDefaultXMLNamespace(JSContext *cx, jsval v)
> {
>     jsval argv[2];
>     JSObject *nsobj, *varobj;
>     JSStackFrame *fp;
>+    JSTempValueRooter tvr;

Nit: declare after argv, to match use order.

>+    JSBool ok;
> 
>     argv[0] = STRING_TO_JSVAL(cx->runtime->emptyString);
>     argv[1] = v;
>+
>+    JS_PUSH_TEMP_ROOT(cx, sizeof argv / sizeof argv[0], argv, &tvr);

Nit: squish argv init and JS_PUSH_TEMP_ROOT here.

>+
>     nsobj = js_ConstructObject(cx, &js_NamespaceClass.base, NULL, NULL,
>                                2, argv);
>-    if (!nsobj)
>-        return JS_FALSE;
>+    if (!nsobj) {
>+        ok = JS_FALSE;
>+        goto out;
>+    }
>     v = OBJECT_TO_JSVAL(nsobj);
> 
>     fp = cx->fp;
>     varobj = fp->varobj;
>     if (varobj) {
>         if (!OBJ_DEFINE_PROPERTY(cx, varobj, JS_DEFAULT_XML_NAMESPACE_ID, v,
>                                  JS_PropertyStub, JS_PropertyStub,
>                                  JSPROP_PERMANENT, NULL)) {
>-            return JS_FALSE;
>+            ok = JS_FALSE;

Nit: set ok = OBJ_DEFINE_PROPERTY(...) to save setting it false on failure.

>+            goto out;
>         }
>     } else {
>         JS_ASSERT(fp->fun && !(fp->fun->flags & JSFUN_HEAVYWEIGHT));

Move the ok = JS_TRUE below to this else clause.

>     }
>     fp->xmlNamespace = JSVAL_TO_OBJECT(v);
>-    return JS_TRUE;
>+    ok = JS_TRUE;
>+
>+out:
>+    JS_POP_TEMP_ROOT(cx, &tvr);
>+    return ok;
> }

Are there other js_ConstructObject calls in jsxml.c or elsewhere that need similar love?

/be

Comment 8

[Igor Bukanov]

(In reply to comment #6)
> Created an attachment (id=210552) [edit]
> Fix both cases

This does not fix xml_setNamespace or ToXMLName cases.

> 
> so it should be up to the caller in this case to ensure that
> argv is in fact rooted.

But then by the same logic it should be interpreter, not js_SetDefaultXMLNamespace, who roots the value. AFAICS popping the value after it is passed to js_SetDefaultXMLNamespace would require less code besides ensuring that the value never looses its roots.


> 

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Attempt 2

How's this?

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 210902 [details] [diff] [review]
Attempt 2

Much as I love tvr as a name (http://www.tvr-eng.co.uk/ -- Travolta's character had one in "Swordfish"), argvtvr might rather be argvr or argtvr -- hey, the last keeps the "tvr" magic and avoids the doubled "v"!

/be

Comment 11

[Igor Bukanov]

Comment on attachment 210902 [details] [diff] [review]
Attempt 2

The patch does not compile:
jsobj.c: In function ‘js_ConstructObject’:
jsobj.c:2191: error: ‘JSMSG_WRONG_CONSTRUCTOR’ undeclared (first use in this function)

Plus that obj = NULL after JS_PUSH_SINGLE_TEMP_ROOT can be moved after failed OBJ_GET_PROPERTY

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Not forgetting js.msg this time

Comment 13

[Igor Bukanov]

Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

The last attachment only incorporates Brendan-induced renames, it does not include js.msg!

Comment 14

[Igor Bukanov]

Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

Forget my last comment, everything was OK!

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

> js_ConstructObject(JSContext *cx, JSClass *clasp, JSObject *proto,
>                    JSObject *parent, uintN argc, jsval *argv)
> {
>     jsval cval, rval;
>-    JSTempValueRooter tvr;
>+    JSTempValueRooter tvr, argtvr;

Nit: argtvr, tvr should come first to match init order below.

>     JSObject *obj, *ctor;
> 
>-    if (!js_FindConstructor(cx, parent, clasp->name, &cval))
>+    JS_PUSH_TEMP_ROOT(cx, argc, argv, &argtvr);

sr=me with that nit picked -- thanks!

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 17

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-325425.js

Comment 18

[Bob Clary [:bc] (inactive)]

Comment on attachment 211556 [details]
e4x/Regress/regress-325425.js

This test has an error but there is a spelling issue as well:
e4x/Regress/regress-325425.js:50: TypeError: wrong construtor called for QName
                                                   ^^^^^^^^^^

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-325425.js

Comment 20

[Bob Clary [:bc] (inactive)]

(In reply to comment #19)
this has the wrong bug number in the test. fixed locally. 

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #18)
> (From update of attachment 211556 [details] [edit])
> This test has an error but there is a spelling issue as well:
> e4x/Regress/regress-325425.js:50: TypeError: wrong construtor called for QName
>                                                    ^^^^^^^^^^

Fixed by work-in-progress patches for bug 326466.

/be

Comment 22

[Bob Clary [:bc] (inactive)]

verified fixed trunk 20060328 win/mac/linux

Comment 23

[Daniel Veditz [:dveditz]]

This is marked fixed based on bug 326466, but don't we want this on the 1.8/1.8.0 branches as well? I doubt we the full Pythonic generators patch.

Please ask for approval on the patch if this is branch-appropriate

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

approved for 1.8.0 branch, a=dveditz for drivers

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 26

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

We are intending to take the Pythonic generators on 1.8.1, last I heard.  Given that, I don't _think_ we need this patch on 1.8.1 separately.

Comment 27

[Bob Clary [:bc] (inactive)]

verified fixed no crash 1.8.0.5, 1.8.1, 1.9a1 all platforms

Comment 28

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-325425.js,v  <--  regress-325425.js
initial revision: 1.1