Closed Bug 311025 Opened 14 years ago Closed 14 years ago

chrome XBL method.eval.call exposes privileged Function constructor

Categories

(Core :: JavaScript Engine, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8rc1

People

(Reporter: sync2d, Assigned: mrbkap)

References

Details

(Keywords: js1.6, verified1.7.13, verified1.8, Whiteboard: [sg:critical] privilege escalation [patch])

Attachments

(6 files, 1 obsolete file)

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.
Attached file 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: sbb?
Status: NEW → ASSIGNED
Keywords: js1.6
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta5
Attached patch proposed fix (obsolete) — Splinter Review
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)
Comment on attachment 198522 [details] [diff] [review]
proposed fix

r=mrbkap
Attachment #198522 - Flags: review?(mrbkap) → review+
(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+
moving out to RC1.
Flags: blocking1.8rc1?
Flags: blocking1.8b5-
Flags: blocking1.8b5+
(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+
Attached patch fixed fixSplinter Review
Attachment #198616 - Flags: superreview?(shaver)
Attachment #198616 - Flags: review?(mrbkap)
Component: Security → JavaScript Engine
QA Contact: toolkit → general
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+
Fix checked in on trunk. I'm leaving this bug open to explore whether |new
Script| neeeds equal treatment here.
Flags: blocking1.8rc1? → blocking1.8rc1+
Thanks!!!

/be
Assignee: brendan → mrbkap
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Target Milestone: mozilla1.8beta5 → mozilla1.8rc1
Attached patch possible portSplinter Review
I think this is a straightforward eval -> Script fix.
Attachment #199395 - Flags: review?(brendan)
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-
Flags: testcase?
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
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #198616 - Flags: approval1.8rc1?
Attached patch rolled-up patchSplinter Review
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+
Fix checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8
Attachment #198616 - Flags: approval1.8rc1? → approval1.8rc1+
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)?
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.8verified1.8
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+
Comment on attachment 211315 [details] [diff] [review]
Backported roll-up patch

Sure, bring on part deux!

/be
Attachment #211315 - Flags: review?(brendan) → review+
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
Group: security
You need to log in before you can comment on or make changes to this bug.