self-modifying script detection can be circumvented

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: moz_bug_r_a4, Assigned: Brian Crowder)

Tracking

({verified1.8.0.12, verified1.8.1.4})

Trunk
verified1.8.0.12, verified1.8.1.4
Points:
---
Bug Flags:
blocking1.8.1.3 -
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.11 -
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 9 obsolete attachments)

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
Brian Crowder
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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();
(Reporter)

Comment 1

11 years ago
Created attachment 251635 [details]
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
(Assignee)

Comment 4

11 years ago
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?
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

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

This proposed patch defeats the testcase.
Attachment #253231 - Flags: review?(brendan)
(Assignee)

Comment 6

11 years ago
Hm, this patch should include removal of the previous "self-modifying script detection" code, too, perhaps?
(Assignee)

Comment 7

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

Comment 9

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

Comment 11

11 years ago
(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?
(Assignee)

Comment 12

11 years ago
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?
Attachment #254037 - Flags: review?(brendan)
(Assignee)

Updated

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

Comment 14

11 years ago
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.
Attachment #254037 - Attachment is obsolete: true
Attachment #254440 - Flags: review?
Attachment #254037 - Flags: review?(brendan)
(Assignee)

Updated

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

Comment 16

11 years ago
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?
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
(Assignee)

Comment 18

11 years ago
Created attachment 255437 [details] [diff] [review]
Adding CallNewScriptHook

Adding the CallNewScriptHook fix here, too.
Attachment #254470 - Attachment is obsolete: true
Attachment #255437 - Flags: review?(brendan)
Attachment #254470 - Flags: review?(brendan)
(Assignee)

Comment 19

11 years ago
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+
(Assignee)

Comment 21

11 years ago
jsscript.c: 3.136
js.msg:     3.75
(Assignee)

Comment 22

11 years ago
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.
Attachment #256658 - Flags: review?(brendan)
(Assignee)

Comment 23

11 years ago
Created attachment 256659 [details] [diff] [review]
ported to 1_8_0 branch

Same; please glance over these and then I will nominate for branches.
Attachment #256659 - Flags: review?(brendan)
(Assignee)

Comment 24

11 years ago
Need to fix my branch patches to include the mod from bug 359024
(Assignee)

Comment 25

11 years ago
Created attachment 256912 [details] [diff] [review]
addressing bug 359024 (for 1_8_BRANCH)

Includes fixes and feedback from bug 359024
Attachment #256658 - Attachment is obsolete: true
Attachment #256658 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Attachment #256912 - Attachment description: addressing bug 359024 → addressing bug 359024 (for 1_8_BRANCH)
(Assignee)

Comment 26

11 years ago
Created attachment 256913 [details] [diff] [review]
addressing bug 359024 (for 1_8_0_BRANCH)
Attachment #256659 - Attachment is obsolete: true
Attachment #256913 - Flags: review?(brendan)
Attachment #256659 - Flags: review?(brendan)
(Assignee)

Updated

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

Comment 28

11 years ago
Created attachment 256952 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8)
Attachment #256912 - Attachment is obsolete: true
Attachment #256952 - Flags: review?(brendan)
(Assignee)

Comment 29

11 years ago
Created attachment 256955 [details] [diff] [review]
!JSVAL_IS_VOID instead of JSVAL_IS_INT (for 1_8_0)
Attachment #256913 - Attachment is obsolete: true
Attachment #256913 - Flags: review?(brendan)
(Assignee)

Comment 30

11 years ago
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+
(Assignee)

Comment 32

11 years ago
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+
(Assignee)

Updated

11 years ago
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?
(Assignee)

Updated

11 years ago
Attachment #256955 - Flags: approval1.8.0.11?
(Assignee)

Comment 33

11 years ago
This bug deserves sg:critical; it's a trivial freed-memory-read
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical]
(Assignee)

Updated

11 years ago
Attachment #256952 - Flags: approval1.8.1.4? → approval1.8.1.3?
(Assignee)

Updated

11 years ago
Attachment #256955 - Flags: approval1.8.0.12? → approval1.8.0.11?
(Assignee)

Comment 34

11 years ago
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-

Comment 36

11 years ago
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?
(Assignee)

Comment 37

11 years ago
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+
(Assignee)

Comment 39

11 years ago
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
Last Resolved: 11 years ago
Keywords: fixed1.8.0.12, fixed1.8.1.4
Resolution: --- → FIXED

Comment 40

10 years ago
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.
Keywords: fixed1.8.0.12, fixed1.8.1.4 → verified1.8.0.12, verified1.8.1.4
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?

Comment 42

10 years ago
brendan/brian, any thoughs on comment 41 ?

Comment 43

10 years ago
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367121.js,v  <--  regress-367121.js
initial revision: 1.1
Flags: in-testsuite? → in-testsuite+

Comment 44

10 years ago
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

10 years ago
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.