Bug 367911 (xow)

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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
Security
P1
normal
RESOLVED FIXED
11 years ago
2 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

({arch})

Trunk
mozilla1.9alpha8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
wanted1.8.0.x ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate] would fix sg:critical and sg:high issues)

Attachments

(2 attachments, 13 obsolete attachments)

124.96 KB, patch
mrbkap
: review+
brendan
: superreview+
Details | Diff | Splinter Review
152.01 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

11 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.
Whiteboard: [sg:investigate] would fix sg:critical and sg:high issues
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.

Updated

11 years ago
Keywords: arch

Comment 2

10 years ago
another good one for upcoming arch discussions
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
(Assignee)

Comment 3

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

Updated

10 years ago
Blocks: 376291

Comment 4

10 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?
(Assignee)

Comment 5

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

Updated

10 years ago
Alias: cow
Summary: Use special wrappers to deal with allAccess properties → Implement cross-origin wrappers (to deal with allAccess properties)
(Assignee)

Updated

10 years ago
Alias: cow → xow
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha6
(Assignee)

Comment 6

10 years ago
Created attachment 266512 [details] [diff] [review]
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.
(Assignee)

Comment 7

10 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.
(Assignee)

Comment 8

10 years ago
Created attachment 266661 [details] [diff] [review]
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(-)
Attachment #266512 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
Created attachment 266847 [details] [diff] [review]
patch v1

I need to test this more before requesting review, but this is something like what I had in mind.
Attachment #266661 - Attachment is obsolete: true
(Assignee)

Comment 10

10 years ago
Created attachment 266848 [details] [diff] [review]
patch v1.1

The previous version's FunctionWrapper didn't quite work right.
Attachment #266847 - Attachment is obsolete: true
(Assignee)

Comment 11

10 years ago
Note to self: have to deal with setting __proto__ on the wrapper.
(Assignee)

Comment 12

10 years ago
Created attachment 267314 [details] [diff] [review]
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).
Attachment #266848 - Attachment is obsolete: true
This needs to block 1.9 -- it's the best hope for fixing a whole class of bugs.
Flags: blocking1.9?
(Assignee)

Comment 14

10 years ago
Created attachment 267644 [details] [diff] [review]
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.
Attachment #267314 - Attachment is obsolete: true
Attachment #267644 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Blocks: 369334
(Assignee)

Comment 15

10 years ago
Created attachment 268286 [details] [diff] [review]
patch v2.5

This patch only creates same-origin wrappers for document, location, and window. We always create wrappers cross-origin.
Attachment #267644 - Attachment is obsolete: true
Attachment #268286 - Flags: review?(jst)
Attachment #267644 - Flags: review?(jst)
Flags: blocking1.9? → blocking1.9+
Targeting B1 per conversation with Blake.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
(Assignee)

Comment 17

10 years ago
Created attachment 270062 [details] [diff] [review]
patch v2.7

This patch is updated to trunk and fixes a bug where crossOriginWin.document.open() wouldn't work.
Attachment #268286 - Attachment is obsolete: true
Attachment #270062 - Flags: review?(jst)
Attachment #268286 - Flags: review?(jst)
(Assignee)

Comment 18

10 years ago
Created attachment 271158 [details] [diff] [review]
patch v2.8

Updated to trunk.
Attachment #270062 - Attachment is obsolete: true
Attachment #271158 - Flags: review?(jst)
Attachment #270062 - Flags: review?(jst)
(Assignee)

Comment 19

10 years ago
Oh, that patch also fixes JS_CheckAccess crashing on every call.
(Assignee)

Comment 20

10 years ago
Created attachment 271938 [details] [diff] [review]
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.
Attachment #271938 - Flags: review?(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...
(Assignee)

Comment 22

10 years ago
Created attachment 272113 [details] [diff] [review]
Addressing jst's first round of comments
+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 :)
(Assignee)

Comment 24

10 years ago
Created attachment 273059 [details] [diff] [review]
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.
Attachment #273059 - Flags: review?(jst)
(Assignee)

Comment 25

10 years ago
Created attachment 273199 [details] [diff] [review]
Last fix

This fixes a (possible) proliferation of wrappers for the same objects when those objects are being accessed cross origin.
Attachment #273199 - Flags: review?(jst)

Updated

10 years ago
Attachment #271938 - Flags: review?(jst) → review+
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.
Attachment #273059 - Flags: review?(jst) → review+
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
Attachment #273199 - Flags: review?(jst) → review+

Updated

10 years ago
Attachment #271158 - Flags: review?(jst) → review+
(Assignee)

Comment 28

10 years ago
Created attachment 273554 [details] [diff] [review]
Updated patch

This addresses everything except the factorization mentioned in comment 26. I'll do that in a separate patch.
Attachment #271158 - Attachment is obsolete: true
Attachment #271938 - Attachment is obsolete: true
Attachment #272113 - Attachment is obsolete: true
Attachment #273059 - Attachment is obsolete: true
Attachment #273199 - Attachment is obsolete: true
Attachment #273554 - Flags: superreview?(brendan)
Attachment #273554 - Flags: review+
Comment on attachment 273554 [details] [diff] [review]
Updated patch

rs=me.

/be
Attachment #273554 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 30

10 years ago
Created attachment 274029 [details] [diff] [review]
Patch as checked in
(Assignee)

Comment 31

10 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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 389796
Depends on: 389878

Updated

10 years ago
Depends on: 390042

Updated

10 years ago
Depends on: 390083
(Assignee)

Updated

10 years ago
Depends on: 389753
(Assignee)

Updated

10 years ago
Depends on: 390001

Updated

10 years ago
Blocks: 350433
(Assignee)

Updated

10 years ago
Blocks: 361961

Updated

10 years ago
Depends on: 390626

Updated

10 years ago
Depends on: 390560

Updated

10 years ago
Depends on: 390946

Updated

10 years ago
Depends on: 391055
Depends on: 391195
Flags: blocking1.8.1.7?
Blocks: 391497
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)
No longer blocks: 391497
Flags: blocking1.8.1.7? → blocking1.8.1.7+
Blocks: 391497
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.
(Assignee)

Comment 34

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

Updated

10 years ago
Depends on: 394815
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.
(Assignee)

Comment 36

10 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.
Depends on: 396849
> 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?
(Assignee)

Comment 38

10 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.
Depends on: 396851
> 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.

Updated

10 years ago
Depends on: 397071
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.
Flags: blocking1.8.1.8+ → blocking1.8.1.9?
Depends on: 397791
Depends on: 398109
Depends on: 405488
No longer depends on: 405488
Depends on: 403005
Flags: blocking1.8.1.12? → blocking1.8.1.13?
(Assignee)

Updated

10 years ago
Depends on: 412462

Updated

10 years ago
Blocks: 416931
Flags: blocking1.8.1.13?
Depends on: 418776
(Assignee)

Updated

9 years ago
Duplicate of this bug: 354277

Updated

6 years ago
Blocks: 412781

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.