Last Comment Bug 367121 - self-modifying script detection can be circumvented
: self-modifying script detection can be circumvented
Status: VERIFIED FIXED
[sg:critical]
: verified1.8.0.12, verified1.8.1.4
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Brian Crowder
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-16 06:30 PST by moz_bug_r_a4
Modified: 2008-02-26 07:48 PST (History)
9 users (show)
mconnor: blocking1.8.1.3-
dveditz: blocking1.8.1.4+
dveditz: wanted1.8.1.x+
mconnor: blocking1.8.0.11-
dveditz: blocking1.8.0.12+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (413 bytes, text/html)
2007-01-16 06:33 PST, moz_bug_r_a4
no flags Details
keeps a use-counter on a reserved slot to check when compiling. (7.87 KB, patch)
2007-01-29 14:26 PST, Brian Crowder
brendan: review-
Details | Diff | Review
tries to address potential threading issue, as well (9.26 KB, patch)
2007-01-29 17:19 PST, Brian Crowder
no flags Details | Diff | Review
most of Brendan's issues addressed (9.48 KB, patch)
2007-02-05 08:13 PST, Brian Crowder
no flags Details | Diff | Review
better locking behavior based on last exchange (9.40 KB, patch)
2007-02-08 12:36 PST, Brian Crowder
no flags Details | Diff | Review
script_thaw fixed, and various nits (11.00 KB, patch)
2007-02-08 16:46 PST, Brian Crowder
no flags Details | Diff | Review
Adding CallNewScriptHook (11.23 KB, patch)
2007-02-16 22:54 PST, Brian Crowder
brendan: review+
Details | Diff | Review
ported to 1_8 branch (10.39 KB, patch)
2007-02-27 11:10 PST, Brian Crowder
no flags Details | Diff | Review
ported to 1_8_0 branch (10.69 KB, patch)
2007-02-27 11:12 PST, Brian Crowder
no flags Details | Diff | Review
addressing bug 359024 (for 1_8_BRANCH) (10.45 KB, patch)
2007-03-01 07:44 PST, Brian Crowder
brendan: review-
Details | Diff | Review
addressing bug 359024 (for 1_8_0_BRANCH) (10.76 KB, patch)
2007-03-01 07:47 PST, Brian Crowder
no flags Details | Diff | Review
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8) (10.44 KB, patch)
2007-03-01 12:59 PST, Brian Crowder
brendan: review+
dveditz: approval1.8.1.4+
Details | Diff | Review
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8_0) (10.77 KB, patch)
2007-03-01 13:18 PST, Brian Crowder
crowderbt: review+
dveditz: approval1.8.0.12+
Details | Diff | Review

Description moz_bug_r_a4 2007-01-16 06:30:24 PST
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 2007-01-16 06:33:48 PST
Created attachment 251635 [details]
testcase
Comment 2 Daniel Veditz [:dveditz] 2007-01-16 11:44:56 PST
Added you to bug 355655 (and shutdown here)
Comment 3 Brendan Eich [:brendan] 2007-01-28 21:30:25 PST
Brian, can you try to come up with a branch fix? Thanks,

/be
Comment 4 Brian Crowder 2007-01-28 23:03:48 PST
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 2007-01-29 14:26:40 PST
Created attachment 253231 [details] [diff] [review]
keeps a use-counter on a reserved slot to check when compiling.

This proposed patch defeats the testcase.
Comment 6 Brian Crowder 2007-01-29 14:29:01 PST
Hm, this patch should include removal of the previous "self-modifying script detection" code, too, perhaps?
Comment 7 Brian Crowder 2007-01-29 14:31:48 PST
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] 2007-01-29 14:56:49 PST
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 2007-01-29 17:19:20 PST
Created attachment 253261 [details] [diff] [review]
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] 2007-01-29 18:30:31 PST
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 2007-01-29 19:45:13 PST
(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 2007-02-05 08:13:25 PST
Created attachment 254037 [details] [diff] [review]
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] 2007-02-05 11:41:12 PST
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 2007-02-08 12:36:11 PST
Created attachment 254440 [details] [diff] [review]
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] 2007-02-08 15:46:31 PST
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 2007-02-08 16:46:21 PST
Created attachment 254470 [details] [diff] [review]
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] 2007-02-15 17:24:15 PST
(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 2007-02-16 22:54:38 PST
Created attachment 255437 [details] [diff] [review]
Adding CallNewScriptHook

Adding the CallNewScriptHook fix here, too.
Comment 19 Brian Crowder 2007-02-26 16:48:07 PST
brendan:  ping?
Comment 20 Brendan Eich [:brendan] 2007-02-26 16:56:32 PST
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 2007-02-26 20:36:35 PST
jsscript.c: 3.136
js.msg:     3.75
Comment 22 Brian Crowder 2007-02-27 11:10:53 PST
Created attachment 256658 [details] [diff] [review]
ported to 1_8 branch

No major changes, but worth eyeballing, I think.  1_8_0 version coming next.
Comment 23 Brian Crowder 2007-02-27 11:12:06 PST
Created attachment 256659 [details] [diff] [review]
ported to 1_8_0 branch

Same; please glance over these and then I will nominate for branches.
Comment 24 Brian Crowder 2007-03-01 00:04:51 PST
Need to fix my branch patches to include the mod from bug 359024
Comment 25 Brian Crowder 2007-03-01 07:44:23 PST
Created attachment 256912 [details] [diff] [review]
addressing bug 359024 (for 1_8_BRANCH)

Includes fixes and feedback from bug 359024
Comment 26 Brian Crowder 2007-03-01 07:47:21 PST
Created attachment 256913 [details] [diff] [review]
addressing bug 359024 (for 1_8_0_BRANCH)
Comment 27 Brendan Eich [:brendan] 2007-03-01 12:17:07 PST
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 2007-03-01 12:59:34 PST
Created attachment 256952 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8)
Comment 29 Brian Crowder 2007-03-01 13:18:11 PST
Created attachment 256955 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8_0)
Comment 30 Brian Crowder 2007-03-01 13:18:58 PST
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] 2007-03-01 13:19:11 PST
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 2007-03-01 13:19:51 PST
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 2007-03-05 00:29:31 PST
This bug deserves sg:critical; it's a trivial freed-memory-read
Comment 34 Brian Crowder 2007-03-05 08:14:04 PST
Nominating for blocking since it has patches.
Comment 35 Mike Connor [:mconnor] 2007-03-05 09:42:51 PST
Not in scope for 1.8.1.3 at this point, should make 1.8.1.4
Comment 36 chris hofmann 2007-03-08 11:05:51 PST
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 2007-03-08 11:23:55 PST
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] 2007-03-16 14:44:25 PDT
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 2007-03-22 09:23:42 PDT
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:] 2007-05-01 15:51:11 PDT
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 2007-08-08 03:25:02 PDT
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 2007-08-31 12:58:51 PDT
brendan/brian, any thoughs on comment 41 ?
Comment 43 Bob Clary [:bc:] 2008-02-07 07:30:24 PST
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367121.js,v  <--  regress-367121.js
initial revision: 1.1
Comment 44 Bob Clary [:bc:] 2008-02-08 12:51:33 PST
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:] 2008-02-26 07:48:19 PST
v

Note You need to log in before you can comment on or make changes to this bug.