311024 - eval(code, window) allows XSS attacks

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[shutdown]

Several months ago, bug 296639 (split-window) fixed bug 296514 and bug 298315.
The problem is solved by linking closures' __parent__ to inner window object
instead of outer window object. SO they get right principal when executing.

However, this restriction can be circumvented by eval(code, window)
because "window" property refers to the outer window object.

Comment 1

[shutdown]

Attachment: XSS testcase

testcase

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

Comment 2

[Brendan Eich [:brendan]]

We probably need an innerObject hook in JSExtendedClass.  jst, mrbkap: any other
ideas?

/be

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> We probably need an innerObject hook in JSExtendedClass.  jst, mrbkap: any other
> ideas?

I have one, and this has been proposed before, bz will recall it: never let
anyone get object principals from an outer window object.  Make them eat
not-a-principal or whatever it's called.

There was some reason, a while back in the split window death march, for
forwarding to the inner window when getting object principals.  I call that a
security hole, and this bug is the proof.

/be

Comment 4

[Daniel Veditz [:dveditz]]

Testcase works on 1.0.7 as well. Is that the same bug, or simply the original
problem split-windows was trying to fix?

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Testcase works on 1.0.7 as well. Is that the same bug, or simply the original
> problem split-windows was trying to fix?

They're the same problem.  Comment 0 contains the key.  In past releases, there
was only one window object, not one outer and one or more inners for each entry
in session history.  So if an attacker could run a lexical closure in the scope
of the window after navigating away from the attacker's document, it would run
with the principals of the new document to which the window was navigated currently.

With split window objects, so long as the outer window supports geting the
object princpals from the current inner window, if an attacker can lexically
capture the outer window in a scope chain, the same exploit works.

The outer window must never return the principals for the current window, or for
any other window in session history.  It should return not-a-principal, or until
we implement that special singleton, an about:blank codebase principal.

/be

Comment 6

[Brendan Eich [:brendan]]

Hoping jst can try again to neuter getting object principals from outer window
objects, and make it work for all cases this time.

/be

Comment 7

[Boris Zbarsky [:bzbarsky]]

> never let anyone get object principals from an outer window object.

We can't really do that, as far as I can see.

More precisely, say I have a window object in my code (outer one, since that's
the only way to really get an explicit window object).  Say someone's trying to
access a property on it.  We need to do a security check.  What principals do
you suggest we use for it?  At the moment we use those of the inner window,
which is what makes the most sense, really...

And this is a use case we really don't want to break, since this is the |var win
= window.open()| use case.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> At the moment we use those of the inner window,
> which is what makes the most sense, really...

This "at the moment" could be read other than how you meant it, as "at the
moment, or moments, when the current inner window is the one to which the script
should have access."

The problem ie lexical environment capture of the outer window.  In talking this
through with jst (who patiently re-reviewed the use-cases for me) and mrbkap, it
became clear that the only fix is to stop environment capture from entraining
the outer window.  So I sketched a plan to add innerObject to JSExtendedClass,
and mrbkap is running with it.  He is indefatigable!

/be

Comment 9

[Asa Dotzler [:asa]]

moving out to RC1.

Comment 10

[Brendan Eich [:brendan]]

'with (window) eval(code)' should have the same effect, btw.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Brendan, is this something like what you had in mind? I'm not sure if the
jsscript.c changes are strictly necessary (or if we need to make
script_{compile,exec} look more like obj_eval wrt the principals changes; if
so, we might consider breaking the common stuff out into common functions).

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 198625 [details] [diff] [review]
patch v1

Drive-by review comments...

+/* NB: The outerObject and innerObjects hooks are assumed to be infallible. */

... but then you've got:

+JS_STATIC_DLL_CALLBACK(JSObject *)
+XPC_WN_InnerObject(JSContext *cx, JSObject *obj)
+{
+    XPCWrappedNative *wrapper =
+	 XPCWrappedNative::GetWrappedNativeOfJSObject(cx, obj);
+    if(!wrapper)
+    {
+	 Throw(NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, cx);
+
+	 return nsnull;
+    }

etc etc.

So this, and the existing outerObject impl here should never return null.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 198625 [details] [diff] [review]
patch v1

>+/* NB: The outerObject and innerObjects hooks are assumed to be infallible. */
> struct JSExtendedClass {
>     JSClass             base;
>     JSEqualityOp        equality;
>     JSObjectOp          outerObject;
>+    JSObjectOp          innerObject;
>     jsword              reserved0;

Don't forget to remove one 0, from JSCLASS_NO_RESERVED_MEMBERS too.

>+    /* Ensure we compile this eval with the right object in the scope chain. */
>+    clasp = OBJ_GET_CLASS(cx, scopeobj);
>+    if (clasp->flags & JSCLASS_IS_EXTENDED) {
>+        JSExtendedClass *xclasp;
>+
>+        xclasp = (JSExtendedClass*)clasp;
>+        if (xclasp->innerObject) {
>+            scopeobj = xclasp->innerObject(cx, scopeobj);
>+            JS_ASSERT(scopeobj);
>+        }
>+    }

If it doesn't win in optimized code space (never mind cycles, they don't matter
with eval) to subroutine this for use here and from jsscript.c, at least
macroize in jsobj.h?

>+JS_STATIC_DLL_CALLBACK(JSObject *)
>+XPC_WN_InnerObject(JSContext *cx, JSObject *obj)
>+{
>+    XPCWrappedNative *wrapper =
>+        XPCWrappedNative::GetWrappedNativeOfJSObject(cx, obj);
>+    if(!wrapper)
>+    {
>+        Throw(NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, cx);
>+
>+        return nsnull;
>+    }
>+
>+    if(!wrapper->IsValid())
>+    {
>+        Throw(NS_ERROR_XPC_HAS_BEEN_SHUTDOWN, cx);
>+
>+        return nsnull;
>+    }

Shouldn't these all return obj and not Throw (at most NS_WARNING)?  As written,
besides botching the JS_ASSERTs you have in the open-coded scopeobj inner-izing
paragraphs, won't they lead to null deref crashes immediately after?

>+
>+    XPCNativeScriptableInfo* si = wrapper->GetScriptableInfo();
>+    if(si && si->GetFlags().WantInnerObject())
>+    {
>+        JSObject *newThis;
>+        nsresult rv =
>+            si->GetCallback()->InnerObject(wrapper, cx, obj, &newThis);
>+
>+        if(NS_FAILED(rv))
>+        {
>+            Throw(rv, cx);
>+
>+            return nsnull;

Same here.  This suggests deCOMTaminating InnerObject.	Is that thinkable?  We
really don't need an nsresult r.v. with a JSObject ** out param, just a
JSObject * r.v.

>@@ -842,21 +881,22 @@ JSExtendedClass XPC_WN_NoHelper_JSClass 
>         nsnull,                         // getObjectOps;
>         nsnull,                         // checkAccess;
>         nsnull,                         // call;
>         nsnull,                         // construct;
>         nsnull,                         // xdrObject;
>         nsnull,                         // hasInstance;
>         XPC_WN_Shared_Mark,             // mark;
>         nsnull                          // spare;
>     },
>     XPC_WN_Equality,
>-    XPC_WN_OuterObject
>+    XPC_WN_OuterObject,
>+    XPC_WN_InnerObject

So you should use JSCLASS_NO_RESERVED_MEMBERS here.

>@@ -6107,20 +6117,50 @@ nsWindowSH::OuterObject(nsIXPConnectWrap
>     // never be called when we have no outer. But just in case, return
>     // null to prevent leaking an inner window to code in a different
>     // window.
> 
>     *_retval = nsnull;

Ok, if outerObject can return null without Throwing, then it's infallible but
nullable.  So innerObject should be the same.  No Throw, null might be ok, but
caller has to defend.

>+NS_IMETHODIMP
>+nsWindowSH::InnerObject(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
>+                        JSObject * obj, JSObject * *_retval)
>+{
>+  nsGlobalWindow *win = nsGlobalWindow::FromWrapper(wrapper);
>+
>+  if (win->IsInnerWindow()) {
>+    // Return the inner window.
>+
>+    *_retval = win->GetGlobalJSObject();
>+  } else {
>+    // Try to find the current inner window. If there isn't one, then we're
>+    // setting up the window (i.e., this is before we even have a document) so
>+    // we can just return the outer window.
>+
>+    nsGlobalWindow *inner = win->GetCurrentInnerWindowInternal();
>+    *_retval = inner
>+               ? inner->GetGlobalJSObject()
>+               : nsnull;

Does not agree with comment or with caller expectations, although nullability
implies defensive callers (but that's more code -- ideally, we'd remove
nullability for both innerObject and outerObject).

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> Shouldn't these all return obj and not Throw (at most NS_WARNING)?  As written,
> besides botching the JS_ASSERTs you have in the open-coded scopeobj inner-izing
> paragraphs, won't they lead to null deref crashes immediately after?

I fixed this in response to comment 12, but this sort of scares me. Perhaps we
should not assume that these operations are infallible. If someone finds a bug
in XPConnect to fail at some point here, it would be exploitable here to grab
references to bad objects (or objects with bad scope chains).

> Same here.  This suggests deCOMTaminating InnerObject.	Is that thinkable?  We
> really don't need an nsresult r.v. with a JSObject ** out param, just a
> JSObject * r.v.

nsIXPCScriptable is an idl interface. This sort of thing is just asking for its
own bug.

> Ok, if outerObject can return null without Throwing, then it's infallible but
> nullable.  So innerObject should be the same.  No Throw, null might be ok, but
> caller has to defend.

I turned this into an NS_ERROR here. This feels like another reason to not force
these hooks to be infallible.

> >+    nsGlobalWindow *inner = win->GetCurrentInnerWindowInternal();
> >+    *_retval = inner
> >+               ? inner->GetGlobalJSObject()
> >+               : nsnull;
> 
> Does not agree with comment or with caller expectations, although nullability
> implies defensive callers (but that's more code -- ideally, we'd remove
> nullability for both innerObject and outerObject).
> 

This one is OK. I felt that setting |obj| twice, so see the if (!*_retval)
clause after this if statement.

I'll attach a new patch tomorrow.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

It turns out that we didn't treat outerObject as being infallible. innerObject
follows in outerObject's footsteps. This patch makes us throw in more cases.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 198850 [details] [diff] [review]
patch v2

Nits only:

1. Invert if/else sense in nsWindowSH::OuterObject to avoid bogo-control-flow:

  if (foo) {
    bar
  } else {
    baz;
    return UNEXPECTED;
  }
  return OK;

2. Use trailing _, not leading, for local names in OBJ_TO_INNER_OBJECT, to
avoid invading the C impl namespace.

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198850 [details] [diff] [review]
patch v2

AIUI the C impl namespace reserves _<CAPITAL> and anything containing __, but
I'll yield.

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 198850 [details] [diff] [review]
patch v2

+nsWindowSH::InnerObject(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
+			 JSObject * obj, JSObject * *_retval)
+{
+  nsGlobalWindow *win = nsGlobalWindow::FromWrapper(wrapper);
+
+  if (win->IsInnerWindow()) {
+    // Return the inner window.
+
+    *_retval = win->GetGlobalJSObject();

That could just be *_retval = obj here.

sr=jst

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198850 [details] [diff] [review]
patch v2

This fixes a security hole. The fix only affects consumers of eval who
explicitly pass in a window as a scope object.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 198850 [details] [diff] [review]
patch v2

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

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 198850 [details] [diff] [review]
patch v2

This patch isn't going to work for moz17/aviary101, is it? Not even with Blake's splitwindow alternative I don't think.

Comment 24

[Daniel Veditz [:dveditz]]

mrbkap says his split-windows alternative does in fact fix this. QA-wanted.

Comment 25

[Daniel Veditz [:dveditz]]

Fixed on the aviary1.0/mozilla1.7 branches by the split-window alternative (bug 316589)

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 198850 [details] [diff] [review]
patch v2

Don't need this on old branches with the split-windows alternative fix.

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/20060215
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