311025 - chrome XBL method.eval.call exposes privileged Function constructor

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[shutdown]

You can retrieve the Function constructor from XBL compilation scope by:
  boundElement.xblMethod.eval.call(null, "Function")
Retrieved constructor can be used to execute arbitrary code with
elevated privileges if xblMethod is defined in the chrome XBL file.

Comment 1

[shutdown]

Attachment: testcase

Works on:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.12) Gecko/20051003 Firefox/1.0.7
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

The indirectCall case requires another subsumes test, to check whether the
scriped caller of indirect-eval should have access to the scope object
parameter, which in this bug's testcase comes from call or apply walking up
eval's scope chain to the chrome compilation global object for the XBL.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198522 [details] [diff] [review]
proposed fix

r=mrbkap

Comment 4

[moz_bug_r_a4]

(In reply to comment #2)
I've applied the proposed patch and tested.

That prevents this:
  var fctor = bound.stop.eval.call(null, "Function");

But, that does not prevent this:
  var fctor = bound.stop.eval.call(1, "Function", bound.stop.eval);


@@ -1172,26 +1199,19 @@ obj_eval(JSContext *cx, JSObject *obj, u

-        }
+        ok = CheckEvalAccess(cx, obj, principals);
+        if (!ok)
+            goto out;
     }

Shouldn't this code be the following?

        ok = CheckEvalAccess(cx, scopeobj, principals);

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 198522 [details] [diff] [review]
proposed fix

sr=shaver

Comment 6

[Asa Dotzler [:asa]]

moving out to RC1.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #4)
> Shouldn't this code be the following?
>         ok = CheckEvalAccess(cx, scopeobj, principals);

Yes it should, duh. I thought I'd checked that when I reviewed, but apparently I
missed it :-(.

When I make that change locally, your second testcase is also (correctly) blocked.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 198522 [details] [diff] [review]
proposed fix

Oh for pete's sake!  Copy paste bug.  Sorry, new patch with scopeobj, not obj,
in second call to CheckEvalAccess in a trice.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: fixed fix

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198616 [details] [diff] [review]
fixed fix

Yeah, sorry I missed this the first time around.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 198616 [details] [diff] [review]
fixed fix

ulp! sr=shaver

(I am so bad at vacation)

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in on trunk. I'm leaving this bug open to explore whether |new
Script| neeeds equal treatment here.

Comment 13

[Brendan Eich [:brendan]]

Thanks!!!

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: possible port

I think this is a straightforward eval -> Script fix.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 199395 [details] [diff] [review]
possible port


> static JSBool
>+CheckScriptAccess(JSContext *cx, JSObject *scopeobj, JSPrincipals *principals,
>+                  const char *name)

Subroutine the CheckEvalAccess code in jsobj.c, maybe keeping its name and just
prepending js_.

>     if (!scopeobj) {
>         /* No scope object passed in: try to use the caller's scope chain. */
>         if (caller) {
>             /*
>              * Load caller->scopeChain after the conditional js_GetCallObject
>              * call above, which resets scopeChain as well as varobj.
>              */
>             scopeobj = caller->scopeChain;
>+
>+            if (!CheckScriptAccess(cx, obj, caller->script->principals,

obj is wrong here, don't you mean scopeobj?  If you do mean scopeobj, then
there's no point testing whether caller->script->principals subsumes
caller->scopeChain's object principals, since that must be the case (we could
assert it, but not here -- probably where we set up a frame -- and it would
slow down DEBUG builds mightily, so do it only if DEBUG_iamslow).

>+                                   "Script.prototype.compile")) {

"....exec", and common in a static const char [].

Not sure we need this patch either -- figure out whether you meant obj above,
or scopeobj.

The consolidation via js_CheckEvalAccess would be cool, I think.

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Actually, I don't think that Script needs this patch (shutdown and moz_bug_r_a4
should speak up now if I'm wrong!). Marking this bug as fixed on the trunk. I'm
working on landing these patches on the branch.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: rolled-up patch

This patch is the best-of all of the security fixes that have gone in lately.
It should make eval and script unassailable (until proven differently, of
course).

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 199611 [details] [diff] [review]
rolled-up patch

Good to sync with trunk.  Followup patch to consolidate OBJ_INNER_OBJECT into
js_CheckScopeChainValidity TBD.

/be

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH.

Comment 20

[Ben Bucksch (:BenB)]

Which bugs does the "rolled-up patch" fix apart from this one?
I assume nobody is porting this to 1.7 (gives more failures than applied hunks)?

Comment 21

[Bob Clary [:bc] (inactive)]

tested attachment 198426 [details] in firefox 1.5 rc2 winxp/linux:

Error: function eval must be called directly, and not by way of a function of another name
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=198426
Line: 22

Comment 22

[Bob Clary [:bc] (inactive)]

Attachment: test.*.com/tests/mozilla.org/security/311025.html

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 199611 [details] [diff] [review]
rolled-up patch

aviary101/moz17 landing approval: a=dveditz for drivers. Please add the fixed1.7.13 and fixed-aviary1.0.8 keywords when landed.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Backported roll-up patch

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 211315 [details] [diff] [review]
Backported roll-up patch

Sure, bring on part deux!

/be

Comment 26

[Daniel Veditz [:dveditz]]

Blake checked in the backported patch to the aviary101/moz17 branches earlier today

Comment 27

[Tracy Walker [:tracy]]

verified with:
Windows:
Moz - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214
Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215
Firefox/1.0.8
Macintosh:
Moz - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13)
Gecko/20060215 Firefox/1.0.8
Fx - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13)
Gecko/20060215 Firefox/1.0.8
Linux
Moz - Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060215