Last Comment Bug 323501 - security check of js_ValueToFunctionObject() can be circumvented
: security check of js_ValueToFunctionObject() can be circumvented
Status: VERIFIED FIXED
[sg:critical][rft-dl] doesn't affect ...
: verified1.8.0.2, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-15 00:43 PST by shutdown
Modified: 2006-10-30 13:37 PST (History)
4 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.31 KB, text/html)
2006-01-15 00:48 PST, shutdown
no flags Details
proposed fix (2.75 KB, patch)
2006-01-15 02:13 PST, Brendan Eich [:brendan]
mrbkap: review-
Details | Diff | Review
safer fix (2.83 KB, patch)
2006-01-24 18:54 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
brendan: review+
dveditz: approval1.8.0.2+
Details | Diff | Review

Description shutdown 2006-01-15 00:43:00 PST
http://lxr.mozilla.org/seamonkey/source/js/src/jsfun.c#2123
2139     caller = JS_GetScriptedCaller(cx, cx->fp);
2140     if (caller &&
2141         !js_CheckPrincipalsAccess(cx, funobj,
2142                                   caller->script->principals,
2143                                   JS_GetFunctionName(fun))) {

This security check can be circumvented by using setTimeout()
or similar that calls a function without any scripted caller.
Comment 1 shutdown 2006-01-15 00:48:30 PST
Created attachment 208544 [details]
testcase

privilege escalation testcase

Works on:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20060114 Firefox/1.6a1
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20060114 Firefox/1.5
Comment 2 Brendan Eich [:brendan] 2006-01-15 02:13:48 PST
Created attachment 208546 [details] [diff] [review]
proposed fix

Clever testcase.  I blame the Array extras added for JS1.6 (half-kidding, shaver ;-).  Compare the Function case (visible in patch due to fix to use the declared string constant), where if there's no scripted caller, principals is null and the check will fail.

In the js_ValueTo{Function,Callable}Object case, we don't give up so easily.  If the callee's object principals (the callee is Array.forEach) subsume those of the callable, we're ok.  So chrome could use setTimeout as this testcase does, and it would work as it should.

I wish eval weren't a universal method, but we can't fix that this time around.

/be
Comment 3 Bob Clary [:bc:] 2006-01-15 03:43:59 PST
tests/mozilla.org/security/323501-01.html
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-01-17 15:52:09 PST
Comment on attachment 208546 [details] [diff] [review]
proposed fix

The extra effort to get the principals from the callee scare me here. This means that if you manage to get your hands on a priviledged Array object, you'd be able to do this same exploit. I know that AJAX people are using this stuff, but setTimeout(Array.forEach, ...) feels like a real edge case. If this starts biting people, I'd argue for adding a belt (or is it braces?) to the array extras.
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-01-17 15:53:48 PST
(In reply to comment #4)
> If this starts biting people, I'd argue for adding a belt (or is it braces?) to the array
> extras.

Of course, I mean in addition to getting the principals from the callee.
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-01-24 18:03:33 PST
Comment on attachment 208546 [details] [diff] [review]
proposed fix

clearing req based on mrbkap's r-.
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-01-24 18:54:51 PST
Created attachment 209540 [details] [diff] [review]
safer fix

This is basically brendan's fix, but without the scary get-principals-from-callee behavior.
Comment 8 Brendan Eich [:brendan] 2006-01-24 19:08:12 PST
Comment on attachment 209540 [details] [diff] [review]
safer fix

> js_ValueToFunctionObject(JSContext *cx, jsval *vp, uintN flags)
> {
>     JSFunction *fun;
>     JSObject *funobj;
>-    JSStackFrame *caller;
>+    JSStackFrame *fp, *caller;
>+    JSPrincipals *principals;
> 
>     if (JSVAL_IS_FUNCTION(cx, *vp))
>         return JSVAL_TO_OBJECT(*vp);
> 
>     fun = js_ValueToFunction(cx, vp, flags);
>     if (!fun)
>         return NULL;
>     funobj = fun->object;
>     *vp = OBJECT_TO_JSVAL(funobj);
> 
>-    caller = JS_GetScriptedCaller(cx, cx->fp);
>-    if (caller &&
>-        !js_CheckPrincipalsAccess(cx, funobj,
>-                                  caller->script->principals,
>-                                  JS_GetFunctionName(fun))) {
>+    fp = cx->fp;
>+    caller = JS_GetScriptedCaller(cx, fp);

Minimize the patch by eliminating fp again -- it's a single-use variable.  Fix that and r=me.

/be

>+    if (caller) {
>+        principals = caller->script->principals;
>+    } else {
>+        /* No scripted caller, don't allow access. */
>+        principals = NULL;
>+    }
>+
>+    /*
>+     * FIXME: Reparameterize so we don't call js_AtomToPrintableString unless
>+     *        there is an error.
>+     */
>+    if (!js_CheckPrincipalsAccess(cx, funobj, principals,
>+                                  fun->atom
>+                                  ? js_AtomToPrintableString(cx, fun->atom)
>+                                  : js_anonymous_str)) {
>         return NULL;
>     }
>     return funobj;
> }
> 
> JSObject *
> js_ValueToCallableObject(JSContext *cx, jsval *vp, uintN flags)
> {
>     JSObject *callable;
>
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-01-25 11:56:39 PST
Fix checked into trunk.
Comment 10 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-02-22 15:58:16 PST
Comment on attachment 209540 [details] [diff] [review]
safer fix

I think we want this on the 1.8 branches, but I need approval.
Comment 11 Daniel Veditz [:dveditz] 2006-02-23 11:29:57 PST
Comment on attachment 209540 [details] [diff] [review]
safer fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 12 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-02-23 15:53:28 PST
Fix checked into the 1.8 branches.
Comment 13 Jay Patel [:jay] 2006-03-01 18:08:15 PST
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060301 Firefox/1.5.0.1, anonymous function call blocked in testcase.
Comment 14 Bob Clary [:bc:] 2006-05-17 15:29:28 PDT
testcase does not alert in Bon Echo 20060517 / winxp or Minefield 20060517 / winxp  and does show the error message.
Comment 15 chris hofmann 2006-07-12 10:58:36 PDT
shutdown,  can you contact me at chofmann@mozilla.org?  Having a problem getting mail though to you.  thanks.

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