Blake Kaplan (:mrbkap) (inactive) Assignee
	Description • 18 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 18 years ago

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.

	chris hofmann
	Comment 2 • 18 years ago

another good one for upcoming arch discussions

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 3 • 17 years ago

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

	Brian Crowder
	Comment 4 • 17 years ago

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?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 5 • 17 years ago

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).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 6 • 17 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 7 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 8 • 17 years ago

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(-)

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 9 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 10 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 11 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 12 • 17 years ago

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).

	Daniel Veditz [:dveditz]
	Comment 13 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 14 • 17 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 15 • 17 years ago

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

	Damon Sicore (:damons)
	Comment 16 • 17 years ago

Targeting B1 per conversation with Blake.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 17 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 18 • 17 years ago

Updated to trunk.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 19 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 20 • 17 years ago

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

	Johnny Stenback (:jst)
	Comment 21 • 17 years ago

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...

	Johnny Stenback (:jst)
	Comment 23 • 17 years ago

+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 :)

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 24 • 17 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 25 • 17 years ago

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

	Johnny Stenback (:jst)
	Comment 26 • 17 years ago

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.

	Johnny Stenback (:jst)
	Comment 27 • 17 years ago

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

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 28 • 17 years ago

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

	Brendan Eich [:brendan]
	Comment 29 • 17 years ago

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

rs=me.

/be

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 31 • 17 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 32 • 17 years ago

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)

	Johnny Stenback (:jst)
	Comment 33 • 17 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 34 • 17 years ago

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).

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 17 years ago

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.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 36 • 17 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 17 years ago

> 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?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 38 • 17 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 17 years ago

> 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.

	Daniel Veditz [:dveditz]
	Comment 40 • 17 years ago

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.