jsxml.c: Bad assumptions about js_ConstructObject

VERIFIED FIXED in mozilla1.9alpha1

Status

()

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

People

(Reporter: Igor Bukanov, Assigned: mrbkap)

Tracking

({verified1.8.0.5, verified1.8.1})

Trunk
mozilla1.9alpha1
verified1.8.0.5, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.14 -
blocking-aviary1.0.9 -
blocking1.8.1 +
blocking1.8.0.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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.
Assuming this can be abused
Assignee: general → brendan
Whiteboard: [sg:critical?]
"(I am) assuming..." to be clear, not doubting.

(Assignee)

Comment 3

12 years ago
I'll take this.
Assignee: brendan → mrbkap
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
(Reporter)

Comment 4

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

Updated

12 years ago
Status: NEW → ASSIGNED
Whiteboard: [sg:critical?] → [sg:critical?][patch]
(Assignee)

Comment 6

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

Comment 8

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


> 

(Assignee)

Comment 9

12 years ago
Created attachment 210902 [details] [diff] [review]
Attempt 2

How's this?
Attachment #210552 - Attachment is obsolete: true
Attachment #210902 - Flags: superreview?(brendan)
Attachment #210902 - Flags: review?(igor.bukanov)
Attachment #210552 - Flags: review?(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
(Reporter)

Comment 11

12 years ago
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
Attachment #210902 - Flags: review?(igor.bukanov) → review-
(Assignee)

Comment 12

12 years ago
Created attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time
Attachment #210902 - Attachment is obsolete: true
Attachment #210985 - Flags: superreview?(brendan)
Attachment #210985 - Flags: review?
Attachment #210902 - Flags: superreview?(brendan)
(Reporter)

Comment 13

12 years ago
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!
Attachment #210985 - Flags: review? → review-
(Reporter)

Comment 14

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

Forget my last comment, everything was OK!
Attachment #210985 - Flags: review- → review+
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
Attachment #210985 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 16

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 17

12 years ago
Created attachment 211556 [details]
e4x/Regress/regress-325425.js

Updated

12 years ago
Flags: testcase+

Comment 18

12 years ago
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
                                                   ^^^^^^^^^^
Attachment #211556 - Attachment is obsolete: true

Comment 19

12 years ago
Created attachment 211999 [details]
e4x/Regress/regress-325425.js

Comment 20

12 years ago
(In reply to comment #19)
this has the wrong bug number in the test. fixed locally. 
(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

12 years ago
verified fixed trunk 20060328 win/mac/linux
Status: RESOLVED → VERIFIED
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
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
Flags: blocking1.7.14-
Flags: blocking-aviary1.0.9-
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5+
(Assignee)

Updated

11 years ago
Attachment #210985 - Flags: approval1.8.0.5?
Attachment #210985 - Flags: approval-branch-1.8.1?(shaver)
Comment on attachment 210985 [details] [diff] [review]
Not forgetting js.msg this time

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210985 - Flags: approval1.8.0.5?
Attachment #210985 - Flags: approval1.8.0.5+
Attachment #210985 - Flags: approval-branch-1.8.1?(shaver)
Attachment #210985 - Flags: approval-branch-1.8.1+
(Assignee)

Comment 25

11 years ago
Fix checked into the 1.8 branches.
Keywords: fixed1.8.0.5, fixed1.8.1
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

11 years ago
verified fixed no crash 1.8.0.5, 1.8.1, 1.9a1 all platforms
Keywords: fixed1.8.0.5, fixed1.8.1 → verified1.8.0.5, verified1.8.1

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.1
Group: security

Comment 28

11 years ago
/cvsroot/mozilla/js/tests/e4x/Regress/regress-325425.js,v  <--  regress-325425.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.