367911 - (xow) Implement cross-origin wrappers (to deal with allAccess properties)

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

While discussing the solution for bug 344495, we decided that in order to deal with allAccess properties on window and friends, we could use wrappers instead of special checks in CAPS. Basically, this would mean wrapping any object that had an allAccess property in a wrapper that would only expose the right amount of information to cross-origin scripts, but provide direct access to the object when the accesses were same-origin.

This would mean wrapping things like window (instead of providing direct access) for calls like window.open in a wrapper - but hopefully we'll be able to use the raw window as the global object for fast variable lookup, etc.

There were two questions:
*) How do we determine which objects to wrap (on creation?)? Because the list of allAccess properties is determined by prefs.js, we'll probably have to talk to CAPS when creating and handing back objects (and the wrapper will have to know what should be exposed).
*) Can we use jst's new safe object wrapper (bug 355766) to share code? I think that we can use the same structure, but as the cross-access wrapper should only be giving access to allAccess properties, I'm not sure about its utility.

Comments are appreciated.

Comment 1

[Boris Zbarsky [:bzbarsky]]

First of all, I don't really think we should be tied to the prefs.js representation of allAccess.  I'm pretty happy removing support for dynamically adding allAccess via prefs altogether (so requiring code changes to do allAccess stuff).

Second, this sounds pretty similar to what Brendan and I were talking about for the proposed glue to replace XPConnect -- have all cross-site access happen through a wrapper that does security checks and have same-site access go direct.  So I'm all for heading in that direction.  ;)

I think we could use the safe object wrapper here if we basically skipped the CanAccess checks for particular properties (however determined) and still wrapped the return value.

Comment 2

[chris hofmann]

another good one for upcoming arch discussions

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

These wrappers might also allow us to remove the security checks currently done by nsWindowSH, but I'm not sure yet.

Comment 4

[Brian Crowder]

While I'm not fully convinced this has performance implications, it certainly seems like it might (and maybe big ones).  Blake: if your plate is too full, can you give a rundown on what you'd want done to implement this?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

I think that I've actually cleared my buglist to the point where I can start working on this (which I've been hoping to do for quite some time).

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: WIP

There are still several XXXs in this patch and it is not even close to having been tested fully. This is the direction that I've been going in, however. Basically, this wrapper acts like an XPCNativeWrapper when you're accessing things cross-origin, but it acts like no wrapper when you're accessing things from the same origin.

Some notes:
-- The function wrapper is really ugly, as it directly calls the native.
-- I haven't special-cased the window expando properties, yet.
-- There's a small bug (untested), where if you have a cross origin wrapper on which you've resolved a property "foo", and you do |"foo" in w| even after w points across origins, we won't throw, but will resolve the property. This isn't a major deal, since you can't actually access the property (the getter will throw).
-- There's still lots of cleanup to do.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Oh, and I still have to hook into the WrapNative code to create these, and remove my hack in the SafeJSObject registering code.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: WIP

I've added some code to wrap values in xpcconvert.cpp. Currently, we wrap too many things (returning them immediately into XPCNativeWrapper, which I've made unwrap them). The current problem I'm looking at is that I don't currently wrap document, location, or navigator (which I think nsWindowSH::{GetProperty,NewResolve} needs to do), which re-opens bug 294978.

11 files changed, 1554 insertions(+), 772 deletions(-)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

I need to test this more before requesting review, but this is something like what I had in mind.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1.1

The previous version's FunctionWrapper didn't quite work right.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Note to self: have to deal with setting __proto__ on the wrapper.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.5

This implements the window craziness. As far as I know, all that's left after this patch is to implement caching of these wrappers (perhaps per-scope) and to avoid wrapping unnecessary things (like random DOM elements).

Comment 13

[Daniel Veditz [:dveditz]]

This needs to block 1.9 -- it's the best hope for fixing a whole class of bugs.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

So, this seems to do what I want. There are some parts that might need cleaning (sXOWRisk in nsDOMClassInfo, for instance), but I think this is pretty close to the truth. I've been browsing with the previous patches in my tree for a while now, and I haven't run into any troubles yet.

I still wrap too many objects, but I'm hoping that caching the wrappers in the wrapped natives should mitigate that problem. If not, then I'll come up with a better way of figuring out which objects to wrap. I realized that I never got rid of the strcmps in xpcconvert.cpp, so perhaps I'll need another iteration on that bit.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2.5

This patch only creates same-origin wrappers for document, location, and window. We always create wrappers cross-origin.

Comment 16

[Damon Sicore (:damons)]

Targeting B1 per conversation with Blake.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2.7

This patch is updated to trunk and fixes a bug where crossOriginWin.document.open() wouldn't work.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2.8

Updated to trunk.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Oh, that patch also fixes JS_CheckAccess crashing on every call.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixing some small issues

This is an interdiff over attachment 271158 [details] [diff] [review] that fixes some small problems that jst pointed out to me.

Comment 21

[Johnny Stenback (:jst)]

nsIXPConnect::WrapInXOW() could be named something that would follow the other related names in nsIXPConnect (i.e. GetCrossOriginWrapper(), or GetCrossOriginWrapperOfJSObject() :) )

@@ -6007,6 +5902,7 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
       nsCOMPtr<nsIXPConnectJSObjectHolder> holder;
       rv = WrapNative(cx, obj, document, NS_GET_IID(nsIDOMDocument), &v,
                       getter_AddRefs(holder));
+
       NS_ENSURE_SUCCESS(rv, rv);

Why? :)

XPCWrapper class name, but XPCWrapperCommon file name. Wanna change one of those so they match?

+// TODO, this should not be a real constructor.
+JSBool
+XPC_XOW_WrapObject(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
+                   jsval *rval);

Wanna deal with that TODO?

+JSObject *
+GetWrappedObject(JSContext *cx, JSObject *wrapper)
[...]
+  if (JSVAL_IS_PRIMITIVE(v)) {
+    return nsnull;
+  }
+
+  return JSVAL_TO_OBJECT(v);

That could be if (!JSVAL_IS_OBJECT(v)) return nsnull;... maybe a bit less code.

+IsValFrame(jsval v, XPCWrappedNative *wn)
+{
+  nsCOMPtr<nsIDOMWindow> domwin(do_QueryWrappedNative(wn));
+  if (!domwin) {
+    return JS_FALSE;

Maybe a fast path out of this would be worth it? I.e. check if the JSClass name doesn't start with 'W' or what not.

+  nsCOMPtr<nsIDOMWindowCollection> col;
+  domwin->GetFrames(getter_AddRefs(col));

It'd be nice to be able to find a child frame from C++ code w/o the need of the new collection object that this will create (and cache). Maybe file a followup bug to expose this functionality through nsPIDOMWindow or something?

+IsWrapperSameOrigin(JSContext *cx, JSObject *wrappedObj)
[...]
+  nsCOMPtr<nsIPrincipal> systemPrin;
+  rv = ssm->GetSystemPrincipal(getter_AddRefs(systemPrin));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // If we somehow end up being called from chrome, just allow full access.
+  // This can happen from components with xpcnativewrappers=no.
+  if (subjectPrin == systemPrin) {

This could be done faster and with less code ssm->IsSystemPrincipal().

+XPC_XOW_WrapObject(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
+                   jsval *rval)
+{
+  // Our argument should be a wrapped native object.
+  JSObject *wrappedObj = JSVAL_TO_OBJECT(argv[0]);
+  XPCWrappedNative *wn;
+  if (JSVAL_IS_PRIMITIVE(argv[0]) ||
+      !(wn = XPCWrappedNative::GetWrappedNativeOfJSObject(cx, wrappedObj))) {

That looks a bit iffy, doing JSVAL_TO_OBJECT(argv[0]) first and then checking if it's acutally an object, but sure :)

And maybe rename wrappedObj to make it more different from wrappedObj (or vise versa). objToWrap might work, or not.

+  JSBool sameOrigin = IsWrapperSameOrigin(cx, wrappedObj) == NS_OK;
+  if (sameOrigin) {
[...]
+
+  JSObject *parent = JS_GetParent(cx, wrappedObj);

IsWrapperSameOrigin() can fail for a number of reasons other than not-same-origin. Should probably check for that.

+XPC_XOW_AddProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
+  nsresult rv = IsWrapperSameOrigin(cx, wrappedObj);
+  if (rv == NS_ERROR_DOM_PROP_ACCESS_DENIED) {
+    // Can't override properties on foreign objects.
+    return ThrowException(rv, cx);
+  }
+  if (NS_FAILED(rv)) {
+    return JS_FALSE;
+  }

Wanna move the rv == NS_ERROR_DOM_PROP_ACCESS_DENIED check inside the if (NS_FAILED(rv)) check to avoid doing two checks in the same-origin case here? Same thing in XPC_XOW_DelProperty().

+XPC_XOW_GetOrSetProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp,
+                         PRUint32 action)

+  nsresult rv = IsWrapperSameOrigin(cx, wrappedObj);
+  if (NS_FAILED(rv) && rv != NS_ERROR_DOM_PROP_ACCESS_DENIED) {
+    return JS_FALSE;
+  }
+  if (NS_FAILED(rv)) {

Similar thing here, would it make sense to move the rv != ... check inside the second NS_FAILED() check here to avoid two NS_FAILED() checks here?

+    JSBool isSet = action == nsIXPCSecurityManager::ACCESS_SET_PROPERTY;
+    if (!XPCWrapper::GetOrSetNativeProperty(cx, obj, wn, id, vp, isSet,
+                                            JS_FALSE)) {

Would it make sense here to pass the action along to GetOrSetNativeProperty() rather than converting it to a boolean here?

That's all for now, XPC_XOW_Enumerate() is next...

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Addressing jst's first round of comments

Comment 23

[Johnny Stenback (:jst)]

+XPCWrapper::ResolveNativeProperty():

+    if (isNativeWrapper) {
+      if (!::JS_GetReservedSlot(cx, wrapperObj, 0, &oldFlags) ||
+          !::JS_SetReservedSlot(cx, wrapperObj, 0,
+                                INT_TO_JSVAL(JSVAL_TO_INT(oldFlags) |
+                                             FLAG_RESOLVING))) {
+        return JS_FALSE;
+      }
+    } else {
+      if (!::JS_GetReservedSlot(cx, wrapperObj, sResolvingSlot, &oldFlags) ||
+          !::JS_SetReservedSlot(cx, wrapperObj, sResolvingSlot, JSVAL_TRUE)) {
+        return JS_FALSE;
+      }
+    }

Please file a followup bug to make all types of wrappers that use this use the same mechanism (and slot) for marking whether they're resolving or not (and consolidate other flags as well?).

XPC_NW_toString():

-  if (!EnsureLegalActivity(cx, obj)) {
-    return JS_FALSE;
-  }

Bad merge?

That's it for the main patch, I'll look over the followups and the coming ones and stamp it once it's all ready :)

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updates

This is a series of patches implementing updates to the patch. With these patches, we pass all of the mochitests. I am currently in the process of running Tp and Txul. I expect one more patch on top of these to always cache wrappers in the right scope. After that, we should be good. An interesting datapoint is that my opt build with wrappers beat out a 1.8 build on Txul, but that's really comparing apples to oranges.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Last fix

This fixes a (possible) proliferation of wrappers for the same objects when those objects are being accessed cross origin.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 273059 [details] [diff] [review]
Updates

- In nsIXPConnect.idl

+     * @param aJSContext A context to use to create objects.
+     * @param aWrappedObj The object to wrap.
+     * @param aParent The parent to create the wrapper with.
+     */
+    [noscript] JSVal getCrossOriginWrapperForObject(in JSContextPtr aJSContext,
+                                                    in JSObjectPtr aParent,
+                                                    in JSObjectPtr aWrappedObj);

@param list order doesn't match actual argument order.

+    XPCCallContext ccx(NATIVE_CALLER, cx);
+    scope = XPCWrappedNativeScope::FindInJSObjectScope(ccx, parent);
+
+    { // Scoped lock
+      XPCAutoLock al(rt->GetMapLock());
+
+      if (outerObj) {
+        outerObj = scope->GetWrapperMap()->Add(wn, outerObj);
+        wn->SetWrapper(nsnull);
+      } else {
+        outerObj = scope->GetWrapperMap()->Find(wn);
+      }
+    }
+
+    if (outerObj) {
+      NS_ASSERTION(JS_GET_CLASS(cx, outerObj) == &sXPC_XOW_JSClass.base,
+                   "What crazy object are we getting here?");
+#ifdef DEBUG_mrbkap
+      printf("But found a wrapper in the map %p!\n", (void *)outerObj);
+#endif
+      wn->SetWrapper(outerObj);
       *vp = OBJECT_TO_JSVAL(outerObj);
       return JS_TRUE;
     }

This code with different return types appears in two places XPCNativeWrapper::GetNewOrUsed() and XPC_XOW_WrapObject(). Maybe worth splitting this out into a function callable from those places?

r=jst with that.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 273199 [details] [diff] [review]
Last fix

- In XPC_XOW_WrapObject():

-  if (sameOrigin) {
+  if (!sameOrigin) {
+    map->Add(wrappedObj, outerObj);

You'll need to lock here when adding to the map, no?

r=jst

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated patch

This addresses everything except the factorization mentioned in comment 26. I'll do that in a separate patch.

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 273554 [details] [diff] [review]
Updated patch

rs=me.

/be

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch as checked in

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

There didn't appear to be any noticeable effects on any of the numbers. Note that the patch in comment 30 is a lot bigger than the rest because of additional context (thanks, CVS diff) and not because of any large changes.

Comment 32

[Daniel Veditz [:dveditz]]

Big patch, fair number of regressions.
 - Is it reasonably safe to fix this on the branch?
 - If so who would create the branch version with mrbkap back in school?
 - Will this break any extensions? (I know we'll need _MOZILLA_1_8_BRANCH
   versions of the nsIXPConnect .idl change)

Comment 33

[Johnny Stenback (:jst)]

If mrbkap can't port this, I can probably help. But I think for us to take this on the branch we'd want to see this out in the hands of a lot more people than what nightly builds get us. I.e. I'd say we should wait until we've seen results from an official Firefox 3 beta for some time before we go ahead and ship this on the branch.

My 2 cents.

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

I can do the backport. School started this week, so I'll be working on balancing schoolwork and Mozilla work, but once I finish getting through the regressions, I'll cut a branch patch (even if it's just because it looks nice).

Comment 35

[Boris Zbarsky [:bzbarsky]]

So... We've actually been trying to eliminate the CheckSameOriginPrincipal() thing this patch uses.  Please either use nsIPrincipal::Equals or nsIPrincipal::Subsumes, depending on which is relevant here.

Also, why does IsWrapperSameOrigin return true for system but not UniversalXPConnect?  I'd expect it to return false for both, frankly, with system then later passing the CheckPropertyAccess check, etc.

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #35)
> So... We've actually been trying to eliminate the CheckSameOriginPrincipal()
> thing this patch uses.  Please either use nsIPrincipal::Equals or
> nsIPrincipal::Subsumes, depending on which is relevant here.

It turns out that the current API isn't good enough -- I totally forgot to take noAccess properties into account. So I'm going to add a new API to nsIScriptSecurityManager that deals with all of these pesky details. My problem is that ::Equals and ::Subsumes don't quite do what I want, I really do want to know if the two principals involved are same origin -- not if they're equal.

> Also, why does IsWrapperSameOrigin return true for system but not
> UniversalXPConnect?  I'd expect it to return false for both, frankly, with
> system then later passing the CheckPropertyAccess check, etc.

The UniversalXPConnect inconsistency is an oversight. In general, I see XOWs degenerating to (basically) XPCSafeJSObjectWrappers in the general case. Once native frames have principals, it shouldn't be necessary to construct the scripted frame like SJOWs do now. IMO, if a piece of chrome code wants XPCNativeWrapper-like access to an object, it should use XPCNW.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> I really do want to know if the two principals involved are same origin -- not
> if they're equal.

Uh...  I'd expect Equals() to tell you exactly that, modulo certificate principals.  How does it not do what you want?

> The UniversalXPConnect inconsistency is an oversight.

Want a bug on this?

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #37)
> Uh...  I'd expect Equals() to tell you exactly that, modulo certificate
> principals.  How does it not do what you want?

Oh. Huh. I was expecting equals to do an equality check. How 'bout that.

> Want a bug on this?

Sure, it's a one line fix that'll be subsumed when I get a chance to fix the noAccess stuff.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Oh. Huh. I was expecting equals to do an equality check

It's equality of principals (that is, actors), yes.  Which for us right now means same-origin.  It's also a guaranteed-symmetric comparison, which may not end up being true of CheckSameOriginPrincipal in 1.9.

> Sure,

Filed bug 396851.

Comment 40

[Daniel Veditz [:dveditz]]

Too scary for 1.8.1.8 (still unfixed regressions on trunk), moving blocking request to 1.8.1.9 and we'll check again to see if this is stable enough.