Closed Bug 367121 Opened 17 years ago Closed 17 years ago

self-modifying script detection can be circumvented

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: crowderbt)

Details

(Keywords: verified1.8.0.12, verified1.8.1.4, Whiteboard: [sg:critical])

Attachments

(4 files, 9 obsolete files)

413 bytes, text/html
Details
11.23 KB, patch
brendan
: review+
Details | Diff | Splinter Review
10.44 KB, patch
brendan
: review+
Details | Diff | Splinter Review
10.77 KB, patch
crowderbt
: review+
Details | Diff | Splinter Review
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();
Attached file testcase
Added you to bug 355655 (and shutdown here)
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Brian, can you try to come up with a branch fix? Thanks,

/be
Assignee: general → 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?
Status: NEW → ASSIGNED
This proposed patch defeats the testcase.
Attachment #253231 - Flags: review?(brendan)
Hm, this patch should include removal of the previous "self-modifying script detection" code, too, perhaps?
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 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
Attachment #253231 - Flags: review?(brendan) → review-
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.
Attachment #253231 - Attachment is obsolete: true
Attachment #253261 - Flags: review?(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
(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?
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?
Attachment #254037 - Flags: review?(brendan)
Attachment #253261 - Attachment is obsolete: true
Attachment #253261 - Flags: review?(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
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.
Attachment #254037 - Attachment is obsolete: true
Attachment #254440 - Flags: review?
Attachment #254037 - Flags: review?(brendan)
Attachment #254440 - Flags: review? → review?(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
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?
Attachment #254440 - Attachment is obsolete: true
Attachment #254470 - Flags: review?(brendan)
Attachment #254440 - Flags: review?(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
Adding the CallNewScriptHook fix here, too.
Attachment #254470 - Attachment is obsolete: true
Attachment #255437 - Flags: review?(brendan)
Attachment #254470 - Flags: review?(brendan)
brendan:  ping?
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
Attachment #255437 - Flags: review?(brendan) → review+
jsscript.c: 3.136
js.msg:     3.75
Attached patch ported to 1_8 branch (obsolete) — Splinter Review
No major changes, but worth eyeballing, I think.  1_8_0 version coming next.
Attachment #256658 - Flags: review?(brendan)
Attached patch ported to 1_8_0 branch (obsolete) — Splinter Review
Same; please glance over these and then I will nominate for branches.
Attachment #256659 - Flags: review?(brendan)
Need to fix my branch patches to include the mod from bug 359024
Includes fixes and feedback from bug 359024
Attachment #256658 - Attachment is obsolete: true
Attachment #256658 - Flags: review?(brendan)
Attachment #256912 - Attachment description: addressing bug 359024 → addressing bug 359024 (for 1_8_BRANCH)
Attachment #256659 - Attachment is obsolete: true
Attachment #256913 - Flags: review?(brendan)
Attachment #256659 - Flags: review?(brendan)
Attachment #256912 - Flags: review?(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
Attachment #256912 - Flags: review?(brendan) → review-
Attachment #256912 - Attachment is obsolete: true
Attachment #256952 - Flags: review?(brendan)
Attachment #256913 - Attachment is obsolete: true
Attachment #256913 - Flags: review?(brendan)
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.
Attachment #256955 - Flags: review?(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
Attachment #256952 - Flags: review?(brendan) → review+
Comment on attachment 256955 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8_0)

great, thanks
Attachment #256955 - Flags: review?(brendan) → review+
Attachment #256952 - Attachment description: !JSVAL_IS_VOID instead of JSVAL_IS_INT → !JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8)
Attachment #256952 - Flags: approval1.8.1.3?
Attachment #256955 - Flags: approval1.8.0.11?
This bug deserves sg:critical; it's a trivial freed-memory-read
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical]
Attachment #256952 - Flags: approval1.8.1.4? → approval1.8.1.3?
Attachment #256955 - Flags: approval1.8.0.12? → approval1.8.0.11?
Nominating for blocking since it has patches.
Flags: blocking1.8.1.3?
Flags: blocking1.8.0.11?
Not in scope for 1.8.1.3 at this point, should make 1.8.1.4
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.3?
Flags: blocking1.8.1.3-
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.11?
Flags: blocking1.8.0.11-
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?
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).
Attachment #256952 - Flags: approval1.8.1.3? → approval1.8.1.4?
Attachment #256955 - Flags: approval1.8.0.11? → approval1.8.0.12?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
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
Attachment #256952 - Flags: approval1.8.1.4? → approval1.8.1.4+
Attachment #256955 - Flags: approval1.8.0.12? → approval1.8.0.12+
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.
Group: security
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);
Flags: in-testsuite?
brendan/brian, any thoughs on comment 41 ?
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367121.js,v  <--  regress-367121.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+
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
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.