chrome XBL method.eval.call exposes privileged Function constructor

VERIFIED FIXED in mozilla1.8rc1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: shutdown, Assigned: mrbkap)

Tracking

({js1.6, verified1.7.13, verified1.8})

Trunk
mozilla1.8rc1
js1.6, verified1.7.13, verified1.8
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8b5 -
blocking1.8rc1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] privilege escalation [patch])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 198426 [details]
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
Assignee: dveditz → brendan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b5+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:critical] privilege escalation
Blocks: 256195

Updated

12 years ago
Status: NEW → ASSIGNED
Keywords: js1.6
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta5
Created attachment 198522 [details] [diff] [review]
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
Attachment #198522 - Flags: superreview?(shaver)
Attachment #198522 - Flags: review?(mrbkap)
(Assignee)

Comment 3

12 years ago
Comment on attachment 198522 [details] [diff] [review]
proposed fix

r=mrbkap
Attachment #198522 - Flags: review?(mrbkap) → review+

Comment 4

12 years ago
(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 on attachment 198522 [details] [diff] [review]
proposed fix

sr=shaver
Attachment #198522 - Flags: superreview?(shaver) → superreview+

Comment 6

12 years ago
moving out to RC1.
Flags: blocking1.8rc1?
Flags: blocking1.8b5-
Flags: blocking1.8b5+
(Assignee)

Comment 7

12 years ago
(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 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
Attachment #198522 - Attachment is obsolete: true
Attachment #198522 - Flags: superreview+
Attachment #198522 - Flags: review+
Created attachment 198616 [details] [diff] [review]
fixed fix
Attachment #198616 - Flags: superreview?(shaver)
Attachment #198616 - Flags: review?(mrbkap)

Updated

12 years ago
Component: Security → JavaScript Engine
QA Contact: toolkit → general
(Assignee)

Comment 10

12 years ago
Comment on attachment 198616 [details] [diff] [review]
fixed fix

Yeah, sorry I missed this the first time around.
Attachment #198616 - Flags: review?(mrbkap) → review+
Comment on attachment 198616 [details] [diff] [review]
fixed fix

ulp! sr=shaver

(I am so bad at vacation)
Attachment #198616 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 12

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

Updated

12 years ago
Flags: blocking1.8rc1? → blocking1.8rc1+
Thanks!!!

/be
Assignee: brendan → mrbkap
Status: ASSIGNED → NEW
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Target Milestone: mozilla1.8beta5 → mozilla1.8rc1
(Assignee)

Comment 14

12 years ago
Created attachment 199395 [details] [diff] [review]
possible port

I think this is a straightforward eval -> Script fix.
Attachment #199395 - Flags: review?(brendan)
(Assignee)

Updated

12 years ago
Whiteboard: [sg:critical] privilege escalation → [sg:critical] privilege escalation [patch]
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
Attachment #199395 - Flags: review?(brendan) → review-

Updated

12 years ago
Flags: testcase?
(Assignee)

Comment 16

12 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #198616 - Flags: approval1.8rc1?
(Assignee)

Comment 17

12 years ago
Created attachment 199611 [details] [diff] [review]
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).
Attachment #199611 - Flags: review?(brendan)
Attachment #199611 - Flags: approval1.8rc1?
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
Attachment #199611 - Flags: review?(brendan)
Attachment #199611 - Flags: review+
Attachment #199611 - Flags: approval1.8rc1?
Attachment #199611 - Flags: approval1.8rc1+
(Assignee)

Comment 19

12 years ago
Fix checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8

Updated

12 years ago
Attachment #198616 - Flags: approval1.8rc1? → approval1.8rc1+

Comment 20

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

12 years ago
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
Keywords: fixed1.8 → verified1.8

Comment 22

12 years ago
Created attachment 206879 [details]
test.*.com/tests/mozilla.org/security/311025.html

Updated

12 years ago
Flags: testcase? → testcase+
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.
Attachment #199611 - Flags: approval1.7.13+
Attachment #199611 - Flags: approval-aviary1.0.8+
(Assignee)

Comment 24

11 years ago
Created attachment 211315 [details] [diff] [review]
Backported roll-up patch
Attachment #211315 - Flags: review?(brendan)
Comment on attachment 211315 [details] [diff] [review]
Backported roll-up patch

Sure, bring on part deux!

/be
Attachment #211315 - Flags: review?(brendan) → review+
Keywords: fixed-aviary1.0.8, fixed1.7.13
Blake checked in the backported patch to the aviary101/moz17 branches earlier today
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
Status: RESOLVED → VERIFIED
Keywords: fixed-aviary1.0.8, fixed1.7.13 → verified-aviary1.0.8, verified1.7.13
Group: security
You need to log in before you can comment on or make changes to this bug.