367121 - self-modifying script detection can be circumvented

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[moz_bug_r_a4]

I'm not sure what bug 355655 is.  But, at least, self-modifying script
detection can be circumvented by using subframe and event.

  <iframe></iframe>

  var d = frames[0].document;
  d.addEventListener("test", function(e) {
    s.compile("");
  }, true);
  var e = d.createEvent("Events");
  e.initEvent("test", true, true);
  var s = new Script("d.dispatchEvent(e);");
  s.exec();

Comment 1

[moz_bug_r_a4]

Attachment: testcase

Comment 2

[Daniel Veditz [:dveditz]]

Added you to bug 355655 (and shutdown here)

Comment 3

[Brendan Eich [:brendan]]

Brian, can you try to come up with a branch fix? Thanks,

/be

Comment 4

[Brian Crowder]

Be glad to take a look.  If anyone can come up with a shell version of this, that would be very helpful.  I haven't looked at it much yet.  I don't suppose any of the bug fixes from 367118, 367119, or 367120 of this series (or all of them conspiring together) magically fixes this?

Comment 5

[Brian Crowder]

Attachment: keeps a use-counter on a reserved slot to check when compiling.

This proposed patch defeats the testcase.

Comment 6

[Brian Crowder]

Hm, this patch should include removal of the previous "self-modifying script detection" code, too, perhaps?

Comment 7

[Brian Crowder]

This patch is tailored for the trunk, btw.  You must re-enable script object in jsconfig.h to test it.  Sorry for the spam.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 253231 [details] [diff] [review]
keeps a use-counter on a reserved slot to check when compiling.

Don't mix other patches in.

The SELF_MODIFYING_SCRIPT error should be renamed (use "that" rather than "which" for a defining clause in the error string), and the old code removed.

The js_Get/SetScriptExecDepth helpers should be static, no js_ prefixes needed.

s/execdepth/execDepth/g

The JS_LOCK_OBJ around calls to the helpers is good for atomicity, but the helpers re-lock, which is supported (nesting of a scope lock, that is) but sub-optimal. Note with comment? In any case, it seems you could have Get and Bump, not Get and Set for less code and (more important) to avoid the ok &= pattern which might store garbage if Get fails but Set succeeds (this is a "can't-happen" in reality, but still -- Bump minimizes the code overall).

/be

Comment 9

[Brian Crowder]

Attachment: tries to address potential threading issue, as well

The idea here is that with locking and use-counting combined, we not only prevent this thread from sabotaging itself with a compile, but also another thread.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 253261 [details] [diff] [review]
tries to address potential threading issue, as well


>+/*
>+ * The locking scheme for the ScriptExecDepth functions is non-optimal, since

s/non-/sub-/

>+ * they may already be called from inside of a lock, and may also acquire an
>+ * additional lock, internally.

Lose the ", and may also acquire..." since they do acquire under JS_GetReservedSlot.

>+    int execDepth;

Nit: s/int/jsint/g (all relevant int uses added by this patch).

>+    JS_LOCK_OBJ(cx, obj);
>+    GetScriptExecDepth(cx, obj, &execDepth);
>+    /*

Nit: prevailing style wants a blank line before the "/*\n" introducing a major comment.

>+     * execDepth must be 0 to allow compilation here, otherwise the JSScript
>+     * object can be released while running

s/object/struct/ -- JSScript is a private data structure, not a GC'ed object.

>+     */
>+    if (execDepth > 0) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                             JSMSG_COMPILE_EXECED_SCRIPT);
>+        JS_UNLOCK_OBJ(cx, obj);

Transpose Report and UNLOCK to avoid deadlock hazards (Report will allocate an exception object).

>+        return JS_FALSE;
>+    }
>+
>     /* Swap script for obj's old script, if any. */
>+    oldscript = (JSScript *) JS_GetPrivate(cx, obj);
>     if (!JS_SetPrivate(cx, obj, script)) {
>         js_DestroyScript(cx, script);
>+        JS_UNLOCK_OBJ(cx, obj);
>         return JS_FALSE;

Ditto, except better: lose the whole error case here -- JS_SetPrivate is infallible.

>     }
>+    JS_UNLOCK_OBJ(cx, obj);
>+
>     if (oldscript)
>         js_DestroyScript(cx, oldscript);

This reminds me: swap oldscript and script declarators in their common declaration line to match use order.

>+    /* Keep track of nesting depth for the script. */
>+    ok = AdjustScriptExecDepth(cx, obj, 1);
>+    if (!ok)
>+        return JS_FALSE;
>+
>+    /* Must get to out label after this */
>     script = (JSScript *) JS_GetPrivate(cx, obj);
>-    if (!script)
>-        return JS_TRUE;
>+    if (!script) {
>+        ok = JS_TRUE;

No need to set ok to true here, it's already true.

>+        goto out;
>+    }
> 
>     /* Belt-and-braces: check that this script object has access to scopeobj. */
>     principals = script->principals;
>     if (!js_CheckPrincipalsAccess(cx, scopeobj, principals,
>                                   CLASS_ATOM(cx, Script))) {
>-        return JS_FALSE;
>+        ok = JS_FALSE;

Prefer to set ok = js_CheckPrincipalsAccess(...); if (!ok) goto out

>+        goto out;
>     }
> 
>-    return js_Execute(cx, scopeobj, script, caller, JSFRAME_EVAL, rval);
>+    ok = js_Execute(cx, scopeobj, script, caller, JSFRAME_EVAL, rval);
>+   
>+out: 
>+    ok &= AdjustScriptExecDepth(cx, obj, -1);
>+    return ok;

Might be better to write return AdjustScriptExecDepth(cx, obj, -1) && ok; on one line -- up to you.

>@@ -905,16 +961,20 @@ Script(JSContext *cx, JSObject *obj, uin
>             return JS_FALSE;
> 
>         /*
>          * script_compile does not use rval to root its temporaries
>          * so we can use it to root obj.
>          */
>         *rval = OBJECT_TO_JSVAL(obj);
>     }
>+
>+    if (!JS_SetReservedSlot(cx, obj, 0, INT_TO_JSVAL(0)))
>+        return JS_FALSE;

Ah, so if the Script constructor always sets the reserved slot to zero, then the static helpers need not use JS_GetReservedSlot at all!  Assert instead that the object has a freeslot at least one greater than the start for the class (the index of the reserved slot).

/be

Comment 11

[Brian Crowder]

(In reply to comment #10)
> Ah, so if the Script constructor always sets the reserved slot to zero, then
> the static helpers need not use JS_GetReservedSlot at all!  Assert instead that
> the object has a freeslot at least one greater than the start for the class
> (the index of the reserved slot).

I'm not sure I understand this remark, do you mind clarifying?

Comment 12

[Brian Crowder]

Attachment: most of Brendan's issues addressed

For a while there I thought I understood your last comment, but in fact I still don't.  How/why do I avoid doing JS_GetReservedSlot() entirely?

Comment 13

[Brendan Eich [:brendan]]

One you call JS_SetReservedSlot for a given slot in the constructor, you can optimize future accesses for all constructed objects as you did for the set case. There's no need to call JS_GetReservedSlot, just in case the slot was not defined yet (so possibly not allocated or initialized).

/be

Comment 14

[Brian Crowder]

Attachment: better locking behavior based on last exchange

LOCKED_OBJ_GET_SLOT does the assertion you're recommending, if I am reading correctly, so this should be okay, I think?  This is a patch against the trunk and it does not go cleanly against the branches.  If this gets r+, I will propose ports for the branches.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 254440 [details] [diff] [review]
better locking behavior based on last exchange

>+/*
>+ * This routine assumes and requires that the script object have been locked

s/the script object/obj
s/have/has

>+ * previously.
>+ */
>+static jsint
>+GetScriptExecDepth(JSContext *cx, JSObject *obj)
>+{
>+    jsval v;
>+
>+    v = LOCKED_OBJ_GET_SLOT(obj, JSSLOT_START(&js_ScriptClass));

No need to assume (so you can lose "assumes and" from the comment too) -- just start the body of this function with JS_ASSERT(JS_IS_OBJ_LOCKED(cx, obj)).

>+    /* Swap script for obj's old script, if any. */
>+    oldscript = (JSScript *) JS_GetPrivate(cx, obj);
>+    JS_SetPrivate(cx, obj, script);
>+    JS_UNLOCK_OBJ(cx, obj);
>+

Suggest LOCKED_OBJ_SET_SLOT rather than JS_SetPrivate, and this made me notice (by grepping JS_SetPrivate) that script_thaw needs the same fix. D'oh!

/be

Comment 16

[Brian Crowder]

Attachment: script_thaw fixed, and various nits

I altered the GetPrivate() calls in these spots to do LOCKED_OBJ_GET_SLOT instead, also.

Should there also be a js_CallNewScriptHook() at the end of script compile, similar to what the script_thaw code has?

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> Should there also be a js_CallNewScriptHook() at the end of script compile,
> similar to what the script_thaw code has?

Yes, there should. Fix here or separate bug?

/be

Comment 18

[Brian Crowder]

Attachment: Adding CallNewScriptHook

Adding the CallNewScriptHook fix here, too.

Comment 19

[Brian Crowder]

brendan:  ping?

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 255437 [details] [diff] [review]
Adding CallNewScriptHook

Looks good, sorry I missed this one in my queue (should have read it; misremembered that I was waiting for you! :-P).

/be

Comment 21

[Brian Crowder]

jsscript.c: 3.136
js.msg:     3.75

Comment 22

[Brian Crowder]

Attachment: ported to 1_8 branch

No major changes, but worth eyeballing, I think.  1_8_0 version coming next.

Comment 23

[Brian Crowder]

Attachment: ported to 1_8_0 branch

Same; please glance over these and then I will nominate for branches.

Comment 24

[Brian Crowder]

Need to fix my branch patches to include the mod from bug 359024

Comment 25

[Brian Crowder]

Attachment: addressing bug 359024 (for 1_8_BRANCH)

Includes fixes and feedback from bug 359024

Comment 26

[Brian Crowder]

Attachment: addressing bug 359024 (for 1_8_0_BRANCH)

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 256912 [details] [diff] [review]
addressing bug 359024 (for 1_8_BRANCH)

Does this need re-review, or just interdiff against the equivalent trunk patch?

Anyway, the "use !JSVAL_IS_VOID(v) not JSVAL_IS_INT(v)" for private slot values still applies. JSVAL_IS_INT tests both !JSVAL_IS_VOID and & JSVAL_INT. The latter tag bit test is unnecessary.

/be

Comment 28

[Brian Crowder]

Attachment: !JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8)

Comment 29

[Brian Crowder]

Attachment: !JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8_0)

Comment 30

[Brian Crowder]

Comment on attachment 256955 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8_0)

interdiff against the trunk patch should be adequate.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 256952 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8)

Preapproving the 1.8.0 branch version if it interdiffs as expected with this patch.

/be

Comment 32

[Brian Crowder]

Comment on attachment 256955 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8_0)

great, thanks

Comment 33

[Brian Crowder]

This bug deserves sg:critical; it's a trivial freed-memory-read

Comment 34

[Brian Crowder]

Nominating for blocking since it has patches.

Comment 35

[Mike Connor [:mconnor]]

Not in scope for 1.8.1.3 at this point, should make 1.8.1.4

Comment 36

[chris hofmann]

might be good to get this landed on the trunk if it hasn't.

comment 21
> jsscript.c: 3.136
> js.msg:     3.75

sounds like it has landed on the trunk.  is that correct?  should this be marked fixed now, and just awaiting opening and approval the next good branch to ship from?

Comment 37

[Brian Crowder]

This is on the trunk, but it doesn't improve the security of the trunk, which no longer has the Script object at all.  I'm only keeping the trunk in-sync to save myself trouble later.  This -must- land on the branches asap (especially since the trunk CVS changes are public).  I'm leaving the bug open until I land the branch patches, since those are where this matters (can't kill the Script object due to compatibility promises).

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 256952 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8)

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 39

[Brian Crowder]

MOZILLA_1_8_BRANCH:
js.msg:      3.43.8.14
jsscript.c:  3.79.2.23

MOZILLA_1_8_0_BRANCH:
js.msg:      3.43.8.2.2.4
jsscript.c:  3.79.2.5.2.7

Comment 40

[Bob Clary [:bc] (inactive)]

verified fixed no crash with attachment 251635 [details] using firefox 1.5.0.12/2.0.0.4 rc1 windows/linux, 1.8.0,1.8.1 debug, 1.9.0 no longer supports Script.

Comment 41

[georgi - hopefully not receiving bugspam]

are self modifying objects safe?

like this:

window.f={get g() {window.f={get g() {return 2}};return(window.f.g)}}
alert(window.f.g);

Comment 42

[chris hofmann]

brendan/brian, any thoughs on comment 41 ?

Comment 43

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367121.js,v  <--  regress-367121.js
initial revision: 1.1

Comment 44

[Bob Clary [:bc] (inactive)]

fix unprotected access to window object in test

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367121.js,v  <--  regress-367121.js
new revision: 1.2; previous revision: 1.1

Comment 45

[Bob Clary [:bc] (inactive)]

v