Content scripts now (since SDK 1.4) execute with exactly same priviledges than the webpage it targets. Here is some history around content scripts: Before 1.0, they were using system principal. Then we used webpage one but content proxy code was still using system principal (as content proxy was a regular CommonJS module, hosted in system principal sandbox). We ended up fixing this through bug 679363 [main breakage point for addons]. And more recently we even went futhur in bug 718666. Now really everything is hosted in sanboxes with same priviledges than the website (even `self.*` APIs). So our content script can now be considered as safier than before. But we ended up cuting any kind of priviledges they had. We discussed about this during the last work week. There is many usecases and it is really helpfull to split each of them. First we have to separate discussion between giving more priviledges to content script (this bug), and giving more priviledges to addons documents (bug 722232). In term of security issue, the difference is quite big. We have on first hand some arbitrary remote code (the web page attached to the content script), whereas on the second hand we only have reviewed addon code (addon documents and JS code). During our meeting we clearly identified that `unsafeWindow` may be incompatible with such feature. I wouldn't declare `unsafeWindow` immediatly dead, as there is multiple way to ease unsafeWindow-related usages. In anycase, if we want to get rid of it, we will have to explain how to implement features that need access to Javascript values. Then we may split this bug in multiple ones, as we may work on each usage independently. But as there may be some way to address all of them at once, I'll just describe these usages first: A) Access `contentWindow` on iframe on another domain (bugs 723434, 723627) Currently throws because `contentWindow is a COW wrapper. <iframe id="iframe" src="http://another-domain-than-the-current-webpage.com"/> document.getElementById("iframe").contentWindow.accessJSValues B) Do cross domain XHR Currently fails as in a regular web page. C) Be able to use canvasContext.toDataURI() on a tinted canvas. Currently throws as in a regular web page. Chrome implemented cross domain XHR in May: http://code.google.com/p/chromium/issues/detail?id=18857#c124 And had intense discussion with developers about providing cross domain XHR and unsafeWindow support. I've opened an etherpad note with similar content. It may be easier to build a proposal there, but feel free to give advices in bug too! https://jetpack.etherpad.mozilla.org/content-scripts-priviledges

A) I think it is a CrossOriginWrapper (XOW) but that does not change much in the accessibility part... So B) and C) I assume could be addressed in the SDK with some work around. But I'm worried about the first one. XOW won't be that simple to workaround... I doubt there is a proper solution for that problem in javascript, and a fix for that would as a consequence likely solve B) and C)... Now I think giving chrome privileges to content-script is dangerous indeed. It put's way too much responsibility on the addon reviewers. There are just too many extra caps for a content-script, and it's too risky that a reviewer skipping over some leaked out chrome capabilities. There was a proposal during our discussion about a special principal. The principal would have instead of a single URI a white list of URIs for same origin policy. So this principal would be somewhere between a regular URI based principal and the system principal... The nice thing about this solution is that it would be a lot simpler to review a code, since the addon developer has to explicitly enumerate the domains he want to gain same origin policies to, and even if the reviewer makes some mistake, the threat is a lot lower than it is with a system principal. Now the only problem here is that implementing this might be very hard...

Just to be extra clear, if you wanna jump into this bug, please consider reading etherpad note: https://jetpack.etherpad.mozilla.org/content-scripts-priviledges Where I listed some alternatives to platform modifications.

So I've just had a discussion with Blake on irc, and as it turned out this custom principal does not really fit in the plans. They are busy trying to get away from caps, and special principals. But he suggested an alternative solution for the problem, a custom wrapper could hold the white list of domains in its policy. For me this version would be probably even easier to implement (as in: still hard but at least seems feasible with some help). ochameau: sure, I have not started implementing anything just investigating what options we have on platform side, and how difficult it is to implement it. I will add some notes to the etherpad too probably tomorrow.

> The nice thing about this solution is that it would be a lot simpler to review a > code, since the addon developer has to explicitly enumerate the domains he want > to gain same origin policies to, and even if the reviewer makes some mistake, the > threat is a lot lower than it is with a system principal. Agreed. I don't think we should give developers a way to make their scripts run with system principal. Because if we do, they'll use it (because it makes things easy), and then addons will start being very attractive exploit targets. (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3) > So I've just had a discussion with Blake on irc, and as it turned out this > custom principal does not really fit in the plans. They are busy trying to > get away from caps, and special principals. But he suggested an alternative > solution for the problem, a custom wrapper could hold the white list of > domains in its policy. For me this version would be probably even easier to > implement (as in: still hard but at least seems feasible with some help). Yes, I think this is the way to go. I'm happy to provide support and discuss it further if you want.

So it just occurred to me that the wrapper based solution is fine for solving this problem for cases like iframe.contentWindow, where is iframe is from some white listed domain. But for other restricting API like XHR, where the security check is purely principal based, I'm not sure the planned wrapper will be much useful. Another use case is the canvas and toDataURI API when used with content from another domain. I have not checked it but probably another principal based check there... Blake, Bobby: any ideas? I guess if we want to stick to the wrapper based solution maybe the compartment could own the list of domains, or we could use nsWrapperCache, this way we could get access to the white list of domains/principals where we need them.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5) > So it just occurred to me that the wrapper based solution is fine for > solving this problem for cases like iframe.contentWindow, where is iframe is > from some white listed domain. But for other restricting API like XHR, where > the security check is purely principal based, I'm not sure the planned > wrapper will be much useful. Yeah, that could be an issue. Can you dig up the exact security checks performed in each of these situations? Depending on what the precise checks are, we can probably figure something out. We have to be careful not to reinvent the privilege manager, though. ;-)

(In reply to Bobby Holley (:bholley) from comment #6) > Yeah, that could be an issue. Can you dig up the exact security checks > performed in each of these situations? > > Depending on what the precise checks are, we can probably figure something > out. We have to be careful not to reinvent the privilege manager, though. ;-) Yeah... XHR uses principal->CheckMayLoad at some places while XBL uses nsContentUtils::CheckSecurityBeforeLoad, which also based on CheckMayLoad. For XHR the check happens at multiple places for sure... so it's quite messy. As an alternative solution: The content-script let say requires access to domains A,B,C. We use that specific wrapper we talked about, so the content-script can have access to objects from domain A,B,C. So if we give the content-script 3 sandboxes with principals A,B,C, using those boxes it could do cross site xhr just fine, and accessing the results just fine as well. Of course this is the implementation level, on API level this could be hidden, and made user friendly. Bobby: is my logic correct? I guess this way we could avoid doing all kind of nasty hacks all over the place, to pass the various principal based security checks...

Sorry for the late reply here - I was pretty swamped last week with the work week, compartment-per-global, and paris bindings. :\ I think we can do better than the sandbox solution. Fundamentally, we're moving away from principal stuff, so we should move away from these principal-based checks. The principal-based architecture focuses on the question "Can X do Y to Z". In the old world, X and Z are principals. In the new world, X is a compartment. Z is _usually_ also a compartment, but we have to make allowances for the CheckMayLoad scenario, where we don't yet have a compartment for the new resource. The XPConnect security model has two notions of protection. "Can X access Z" (the filtering stuff), but also "Does X need to be protected from Z while accessing it" (the Xray stuff). We already have explicit per-compartment handling for the second case (wantXray). So we just need to also add something for the first case. So I'm envisioning adding a |Dominates| or |Subsumes| API to XPConnect that takes X as a compartment, and Z as either a compartment or a url/principal. For most compartments, this question is answered in the traditional fashion: am I chrome? If not, is it same-origin? But, similar to the wantXray case, we can have per-compartment special-cases. So things like CheckSecurityBeforeLoad can just determine the caller compartment, and call this API. The API could also, conceptually, be formulated as methods on the CompartmentPrivate (or, in the post-compartment-per-global world, the XPCWrappedNativeScope). Thoughts?

(In reply to Bobby Holley (:bholley) from comment #8) > moving away from principal stuff, so we should move away from these > principal-based checks. If these changes are necessary/useful anyway then I'm fine with it > So I'm envisioning adding a |Dominates| or |Subsumes| API to XPConnect that > takes X as a compartment, and Z as either a compartment or a url/principal. I really like this approach. Actually what I have in mind right now is instead of introducing new wrapper for this, (it's just too slow that for each property access we have to check the callers principal against a list of principals), I would put the list of allowed principals on the CompartmentPrivate anyway. And make a special case for this in the WrapperFactory (rewrap) to create simple transparent same origin wrapper (CrosscompartmentWrapper) for the compartments with principal that any of the principals from that list subsumes, and create XOW for any other content that is not on the list.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9) > I really like this approach. Actually what I have in mind right now is > instead of introducing new wrapper for this, (it's just too slow that for > each property access we have to check the callers principal against a list > of principals), I would put the list of allowed principals on the > CompartmentPrivate anyway. And make a special case for this in the > WrapperFactory (rewrap) to create simple transparent same origin wrapper > (CrosscompartmentWrapper) for the compartments with principal that any of > the principals from that list subsumes, and create XOW for any other content > that is not on the list. Exactly. Static security policy FTW.

One another usecase that faces some cross domaine limitation is: document.loadBindingDocument (see bug 717937) Looks like this security check is done here: http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLService.cpp#962

So, jst and I talked about this today. He was of the opinion that all of the extra security information (wantXrays, whitelists, etc) that we want to associated with a compartment should live on a separate object (rather than just sticking everything into the compartment private), which might as well be the principal. We'll still be ripping out most of the old principal functionality, and we'll still be (mostly) using this new information to compute cross-compartment wrappers. But the information would live on the principal object. Blake, does this sound good to you?

This sounds a bit weird to me to be honest. I would really like to hear some reasoning about this to understand it. To me principal is a very simple security token, that should be lightweight and simple as possible. wantXray sounds really not something that belong to this token, and with the white list... I'm not so sure about that either, because that is actually an array of principals, which sounds a bit silly to me that a principal can just own an array like that. Not to mention the hashmap in the principal code that might further complicate this approach... I started to implement this whitelist as a ref countable object, so I can stick it to any object you guys think it should belong to, but right now I feel like the compartment private is the best place to go. Simply because the principal should not contain the logic needed to decide 'what wrapper should we use in this case'. That decisions should not be part of the caps code at all. And I don't see any 3rd object in the picture here just the compartment and the principal. Then again, I might not see the big picture here just want to hear some reasoning to understand it.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13) > This sounds a bit weird to me to be honest. I would really like to hear some > reasoning about this to understand it. Johnny's main reasoning for not just sticking everything on the compartment was two-fold: 1 - It's better software design to keep related things together, rather than sticking them all on a general object. 2 - The notion of compartment is sort of a detail of the JS engine, and it's not the cleanest to design our DOM security architecture around them. In particular, it leads to weird cases where you might want to make a security decision and not have a compartment lying around with those security characteristics (for example: can A load B?) > To me principal is a very simple security token, that should be lightweight and simple as possible. wantXray > sounds really not something that belong to this token, and with the white > list... I'm not so sure about that either, because that is actually an array > of principals Well, this is all just terminology. Really, we've got two things. The first is an origin/URL, which is what nsPrincipal currently is for the most part. The second is a broader set of "security characteristics". These allow us to ask more finely-grained questions than "Are A and B same origin, or is A chrome?", which is largely what our principal checks consist of now. Instead, we can ask questions like "can A access B?" (which might check a whitelist) and "does A need to be protected from B while accessing it?" (i.e., wantXrays). So I think we want to objects: 'origins' and 'security characteristics'. Which one (if either) we use nsIPrincipal for isn't super important. It's mostly relevant for ease of transition. > Simply because the principal should not > contain the logic needed to decide 'what wrapper should we use in this > case'. Well, it wouldn't. It would answer the questions I listed above, and xpconnect/wrappers would then select the appropriate wrapper. Regardless though, I think mrbkap needs to weigh in here and perhaps make the final call. He sometimes gets pretty bogged down with bugmail, so it might be good to ping him tomorrow during your workday and ask him to comment here.

(In reply to Bobby Holley (:bholley) from comment #14) > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13) > > This sounds a bit weird to me to be honest. I would really like to hear some > > reasoning about this to understand it. > > Johnny's main reasoning for not just sticking everything on the compartment > was two-fold: > 1 - It's better software design to keep related things together, rather than > sticking them all on a general object. I agree with this, I just didn't see why does this info belong more to principal than to compartments. > 2 - The notion of compartment is sort of a detail of the JS engine, and it's > not the cleanest to design our DOM security architecture around them. In > particular, it leads to weird cases where you might want to make a security > decision and not have a compartment lying around with those security > characteristics (for example: can A load B?) Well... you can look at compartments as low level memory blocks, but you can also look at them as security boundaries, and it's not necessary DOM specific security it's just general security. I don't really understand your example here, and I assumed (can be wrong) that at least one compartment is always lying around when we want to make a security decision. I fully agree with the rest and thanks for the detailed answer. So one practical reason I was not too happy about adding these extra info to nsPrincipals is this comment: http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1095 The white list I would append to an nsPrincipal and would be used in the sameorigin check would totally break this, which might not be a problem if we dont want to use that principal in a hashtable ever, but it made me a bit nervous... Is this something I should worry about?

Gabor and I just discussed this on IRC. I think it makes the most sense for him to just stick things on the compartment private for now, and leave not make him do a major API refactor. We can revisit this all when we clean up caps.

So implementing that special sandbox with the list of principals, wasn't that hard at all. But then I tried to continue the patch to support the cross origin xhr an I ran into several problems. First of all xhr object stores the principal and bases all the security checks on that. We talked about this, no surprise there. I plan to invent that 'security characteristics' object here, and add some utility function for it in nsContentUtils. And alter the checks to use these new API. The real problem as it turned out that the jetpack content-scripts as they are sandboxes, cannot create their own xhr object, instead they are using the one that was created by the actual content (through it's content window). Now that xhr object stores the security characteristics of the content obviously and will not have a clue about the additional privileges the content-script has, since these information are stored when an instance is created. So we have two options: (1) make it possible to create an xhr from a none chrome sandbox. (2) make it possible to reinit an existing xhr object. The second seems to be way to hacky. For the first, it's not clear how we should do it. Simply importing the createInstance of the xhr (Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance with a bind) does not work since createInstance will throw that we are not calling it from chrome context. I could implement an xhrfactory object with a simple interface with a simple create method, and we could create a factory in chrome context and then import it into the sandbox... but I would be glad to find a simpler way. There is a way to implement a 'static' method on an interface (some DOM hack), which would be probably available from none chrome context too, but I would not like to go there either... So I'm struggling with this. Bobby, if you could put some thoughts in this that would be great, I'm quite blocked now with this.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17) > So I'm struggling with this. Bobby, if you could put some thoughts in this > that would be great, I'm quite blocked now with this. We just talked about this a little bit on IRC. smaug says we really want the origin of an XHR object to be burned into it at init time. So we're going to add a sandbox option that creates a special XHR constructor on the sandbox for a given origin. This sandbox can then check the origin against the whitelist.

Some more talks. Last time I talked to bz and bholley on irc, we agreed to add the list to the principal, more specifically implement a new special nsprincipal with a custom subsumes method. And change some security checks that uses equal instead of subsumes if needed. So there are a few things here, first I would like to hear some response from Blake about this approach. I assume adding more caps code is not what you dreamed about, or anyone for that mater. But I really see no other option to address all the issues we have (XHR for example where the principal is stored and used at random times for security checks). So here is a proto version of the patch. It really needs some more work, like I should move the new principal in some new file, then change the CreateXMLHttpRequest function I add to the sandbox into a constructor (I don't know how to do that yet...) and make it optional (another option for sandbox). Maybe changing the way I create/init the nsExtendedPrincipal. And finally extending the tests, and break the patch into sub-patches. Anyway, I would really like to hear some feedback on it from you guys.

Comment on attachment 618693 [details] [diff] [review] proto As usual, I think the patch should be broken up before the final review ;-). IMO, 'extended principal' is kind of vague, and could be better. Maybe MultiPrincipal? PrincipalWithWhitelist? ExpandedPrincipal? ExpandedPrincipal might be a good compromise between sounding nice and hinting at its purpose. One important consideration is what happens when somebody else (of same primary origin) tries to subsume _us_. I think that this should fail if their whitelist is not a superset of ours. Otherwise, they could target other origins via the whitelist principal. But this means that nsPrincipal::Subsumes has to be able to tell if the other principal has a whitelist. The best way to do this IMO is with a QI to a separate interface that has a GetWhiteList method. This means that nsPrincipals::Subsumes becomes whitelist-aware (or we move that code into a common helper somewhere), and the subclass doesn't need to implement Subsumes at all - it just implements GetWhiteList. Thoughts? > class nsIObjectInputStream; > class nsIObjectOutputStream; >+class DomainPolicy; Curious what this is about. > protected: > virtual ~nsPrincipal(); >@@ -151,16 +152,42 @@ protected: > nsCOMPtr<nsIURI> mDomain; > bool mTrusted; > bool mInitialized; > // If mCodebaseImmutable is true, mCodebase is non-null and immutable > bool mCodebaseImmutable; > bool mDomainImmutable; I think all this stuff should live in the superclass. >+NS_IMETHODIMP >+nsExtendedPrincipal::GetHashValue(PRUint32 *result) >+{ >+ // extended principal should never be used as key in a has map >+ return NS_ERROR_FAILURE; >+} >+ Why? >+nsresult >+nsExtendedPrincipal::Init(nsIURI *aCodebase) >+{ >+ NS_ENSURE_STATE(!mInitialized); >+ mInitialized = true; >+ >+ mCodebase = NS_TryToMakeImmutable(aCodebase); >+ mCodebaseImmutable = URIIsImmutable(mCodebase); >+ return NS_OK; >+} >+ Why override this? >diff --git a/js/xpconnect/src/XPCComponents.cpp b/js/xpconnect/src/XPCComponents.cpp >--- a/js/xpconnect/src/XPCComponents.cpp >+++ b/js/xpconnect/src/XPCComponents.cpp >@@ -56,16 +56,17 @@ > #include "XrayWrapper.h" > #include "nsNullPrincipal.h" > #include "nsJSUtils.h" > #include "mozJSComponentLoader.h" > #include "nsContentUtils.h" > #include "jsgc.h" > #include "jsfriendapi.h" > #include "mozilla/dom/bindings/Utils.h" >+#include "nsPrincipal.h" > > using namespace mozilla; > using namespace js; > > using mozilla::dom::bindings::DestroyProtoOrIfaceCache; > > /***************************************************************************/ > // stuff used by all >@@ -2870,16 +2871,28 @@ SandboxImport(JSContext *cx, unsigned ar > if (!JS_ValueToId(cx, STRING_TO_JSVAL(funname), &id)) > return false; > > JS_SET_RVAL(cx, vp, JSVAL_VOID); > return JS_SetPropertyById(cx, thisobj, id, &argv[0]); > } > > static JSBool >+CreateXMLHttpRequest(JSContext *cx, unsigned argc, jsval *vp) >+{ >+ JSObject *thisobj = JS_THIS_OBJECT(cx, vp); >+ JSObject *global = JS_GetGlobalForObject(cx, thisobj); >+ nsCOMPtr<nsISupports> inst; >+ nsresult rv; >+ inst = do_CreateInstance("@mozilla.org/xmlextras/xmlhttprequest;1", &rv); >+ nsContentUtils::WrapNative(cx, global, inst, vp); >+ return true; >+} As you mentioned in the comment, we should probably find some way to make this act like a real DOM constructor, so that addons can just do "new XMLHttpRequest". >+ if (!JS_HasProperty(cx, optionsObject, "allowedDomains", &found)) >+ return NS_ERROR_INVALID_ARG; >+ Yeah, this should definitely be optional. >+ if (found) { >+ if (!JS_GetProperty(cx, optionsObject, "allowedDomains", &option) || >+ JSVAL_IS_PRIMITIVE(option)) { >+ return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); >+ } >+ uint32_t length; >+ JSObject *arrayObj = JSVAL_TO_OBJECT(option); >+ if (!JS_IsArrayObject(cx, arrayObj) || !JS_GetArrayLength(cx, arrayObj, &length) || !length) >+ return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); >+ >+ nsTArray<nsCOMPtr<nsIPrincipal>> allowedDomains(length); >+ allowedDomains.SetLength(length); >+ for (uint32_t i = 0; i < length; ++i) { >+ jsval urlVal; >+ if (!JS_GetElement(cx, arrayObj, i, &urlVal) || !JSVAL_IS_STRING(urlVal)) >+ return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); >+ >+ JSString *urlStr = JSVAL_TO_STRING(urlVal); >+ nsCOMPtr<nsIPrincipal> urlPrincipal; >+ rv = GetPrincipalFromString(cx, urlStr, getter_AddRefs(urlPrincipal)); >+ if (NS_FAILED(rv)) { >+ return ThrowAndFail(rv, cx, _retval); >+ } >+ >+ allowedDomains[i] = urlPrincipal; >+ } >+ extended = new nsExtendedPrincipal(); >+ nsCOMPtr<nsIURI> baseURI; >+ rv = principal->GetURI(getter_AddRefs(baseURI)); >+ extended->Init(baseURI); >+ extended->Extend(allowedDomains); >+ prinOrSop = extended; >+ } >+ This would be good to split up into some helpers, with comment. ;-) We should also make it possible to construct the subclass from an instance of nsPrincipal, rather than stringifying things. The subclass is just an nsPrincipal + a whitelist, so that should be fine.

(In reply to Bobby Holley (:bholley) from comment #20) > Comment on attachment 618693 [details] [diff] [review] > > MultiPrincipal? PrincipalWithWhitelist? ExpandedPrincipal? ExpandedPrincipal ExpandedPrincipal works for me. I could be convinced about the the other two as well. > somewhere), and the subclass doesn't need to implement Subsumes at all - it > just implements GetWhiteList. Thoughts? So I would not like to merge this logic into nsPrincipal subsume if I don't have to... I would just say that an EP is only subsumed by system principal or another EP, and never by a regular nsPrincipal. QI to another interface to decide if it's a an EP or not is probably the way to go. So the regular nsPrincipal subsume will be changed only to check if the other principal QIs to this special principal and if it is we just return false. The subsume of EP should be changed to handle other EPs too. Would that work for you? > >+class DomainPolicy; > > Curious what this is about. It was a compiler error I had from a code that is probably removed by now... or at least I cannot see anything that would need it now, so I will just try to get rid of it. > > > > protected: > > virtual ~nsPrincipal(); > >@@ -151,16 +152,42 @@ protected: > > nsCOMPtr<nsIURI> mDomain; > > bool mTrusted; > > bool mInitialized; > > // If mCodebaseImmutable is true, mCodebase is non-null and immutable > > bool mCodebaseImmutable; > > bool mDomainImmutable; > > I think all this stuff should live in the superclass. It is already the case isn't it? > > >+NS_IMETHODIMP > >+nsExtendedPrincipal::GetHashValue(PRUint32 *result) > >+{ > >+ // extended principal should never be used as key in a has map > >+ return NS_ERROR_FAILURE; > >+} > >+ > This is a part I still not have figured out completely. So it all starts here: http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1087 I'm not sure yet if this is a problem or not. But I don't know yet how should the CheckSameOriginPrincipal work for ExpandedPrincipals. And we need to define it. And how to generate hash for them... it can be problematic. There can be two different EQ with different origin, and slightly different whitelists (order and stuff) and it might be difficult to find a good hash method that does not break this rule, given two equivalent EQs like that. Also I don't really see the use of it right now. > > >+nsresult > >+nsExtendedPrincipal::Init(nsIURI *aCodebase) > >+{ > >+ NS_ENSURE_STATE(!mInitialized); > >+ mInitialized = true; > >+ > >+ mCodebase = NS_TryToMakeImmutable(aCodebase); > >+ mCodebaseImmutable = URIIsImmutable(mCodebase); > >+ return NS_OK; > >+} > >+ I just wanted to have a simpler init function I can call manually... I'm not happy with the current way to instantiate and init a new EP... Not sure how it should work, maybe a constructor that takes an nsIPrincipal* and a whitelist. > This would be good to split up into some helpers, with comment. ;-) Alright. > > We should also make it possible to construct the subclass from an instance > of nsPrincipal, rather than stringifying things. The subclass is just an > nsPrincipal + a whitelist, so that should be fine. You mean when the new sandbox option is set from javascript that array should optionally contain principals as well not just strings? (or windows even...) Makes sense. Also it should throw for special principals, adding a null or system principal to the list makes no sense. One more question, what do you think about the way currently the white list is set? That I swap the input array with the member in the nsExtendedPrincipal::Extend method (which should be renamed ofc)? Is it ok to do it like this, or should I copy the array instead?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #21) > So I would not like to merge this logic into nsPrincipal subsume if I don't > have to... I would just say that an EP is only subsumed by system principal > or another EP, and never by a regular nsPrincipal. QI to another interface > to decide if it's a an EP or not is probably the way to go. So the regular > nsPrincipal subsume will be changed only to check if the other principal QIs > to this special principal and if it is we just return false. The subsume of > EP should be changed to handle other EPs too. Would that work for you? Sounds like a plan. > > I think all this stuff should live in the superclass. > > It is already the case isn't it? Oh, I misread the diff, thinking the context lines were +-ed. Sorry. > > > > > >+NS_IMETHODIMP > > >+nsExtendedPrincipal::GetHashValue(PRUint32 *result) > > >+{ > > >+ // extended principal should never be used as key in a has map > > >+ return NS_ERROR_FAILURE; > > >+} > > >+ > > > > This is a part I still not have figured out completely. So it all starts > here: > http://mxr.mozilla.org/mozilla-central/source/caps/src/ > nsScriptSecurityManager.cpp#1087 I'm not sure yet if this is a problem or > not. But I don't know yet how should the CheckSameOriginPrincipal work for > ExpandedPrincipals. And we need to define it. CheckSameOriginPrincipals is in the public API, but it shouldn't be. So you should probably make a patch underneath this that stops exposing it and makes it an internal computation to nsPrincipal. Right now the only nontrivial use is in nsPrincipal to determine whether two principals are equal. Presumably we'll already be handling EPs there, so we shouldn't have to worry about what it means for an EP to be same-origin. > And how to generate hash for them... it can be problematic. There can be two > different EQ with different origin, and slightly different whitelists (order > and stuff) I think we're definitely going to want an Equals() method that does something similar to Subsumes (and have nsPrincipal::Equals bork if it QIs to nsIExpandedPrincipal). We don't want somebody thinking that our EP is equal to a regular principal just because the primary origins are the same. To do this, we'll need sort the whitelists into a canonical order, which makes hashing easier. Anyway, I'm fine with not implementing hashes if they don't appear to be necessary. Just MOZ_ASSERT(false) in the method. > > > > > >+nsresult > > >+nsExtendedPrincipal::Init(nsIURI *aCodebase) > > >+{ > > >+ NS_ENSURE_STATE(!mInitialized); > > >+ mInitialized = true; > > >+ > > >+ mCodebase = NS_TryToMakeImmutable(aCodebase); > > >+ mCodebaseImmutable = URIIsImmutable(mCodebase); > > >+ return NS_OK; > > >+} > > >+ > > I just wanted to have a simpler init function I can call manually... I'm not > happy with the current way to instantiate and init a new EP... Not sure how > it should work, maybe a constructor that takes an nsIPrincipal* and a > whitelist. Well, I don't think this really helps. This is just reimplementing a subset of what nsPrincipal::Init does. We should keep all the nsPrincipal::Init stuff, and possibly have an nsExpandedPrincipal::Init that does other stuff and then calls into nsPrincipal::Init. > > > > We should also make it possible to construct the subclass from an instance > > of nsPrincipal, rather than stringifying things. The subclass is just an > > nsPrincipal + a whitelist, so that should be fine. > > You mean when the new sandbox option is set from javascript that array > should optionally contain principals as well not just strings? (or windows > even...) So, I think that if nsEP inherits nsPrincipal, it should behave like an nsPrincipal, which means that it needs to have a 'primary' origin. If it does, then the primary principal would continued to be passed in the sandbox constructor as it is currently (either as an origin or an nsIPrincipal), meaning that we can reuse that code. The whitelist is then an optional argument with 'extra' origins that we can impersonate. Now, maybe it doesn't make sense for there to be a primary principal. If it doesn't, then we should just make nsEP not inherit nsPrincipal. Maybe we should, in fact? We certainly want them to behave differently in all the core cases... > Makes sense. Also it should throw for special principals, adding a null or > system principal to the list makes no sense. > > One more question, what do you think about the way currently the white list > is set? That I swap the input array with the member in the > nsExtendedPrincipal::Extend method (which should be renamed ofc)? Is it ok > to do it like this, or should I copy the array instead? Copy. Also, I think that principals should probably be immutable after creation. So we should probably pass the whitelist in the constructor/initializer instead.

(In reply to Bobby Holley (:bholley) from comment #22) > So, I think that if nsEP inherits nsPrincipal, it should behave like an > nsPrincipal, which means that it needs to have a 'primary' origin. If it > does, then the primary principal would continued to be passed in the sandbox > constructor as it is currently (either as an origin or an nsIPrincipal), > meaning that we can reuse that code. The whitelist is then an optional > argument with 'extra' origins that we can impersonate. > > Now, maybe it doesn't make sense for there to be a primary principal. If it > doesn't, then we should just make nsEP not inherit nsPrincipal. Maybe we > should, in fact? We certainly want them to behave differently in all the > core cases... I've been thinking about this from the beginning which is the better. This was the simpler for the proto... So principals have certificates and capabilities. So I had to implement a lot more functions, which I did not know what to do about. Now I think that it just don't make any sense to have that primary principal. Nor do we really need caps and certificates for it so I can just leave them empty like for system pincipal, and even if we do I can still abstract those functions out into a shared base class. But if we don't have a main principal how about instead of adding an extra option to the sandbox we just let the first argument be an array. So instead of cu.Sandbox(primaryPrincipal, {allowedDomains: [...some white list...]}) it would be simply cu.Sandbox([...some white list...]); > > > Also, I think that principals should probably be immutable after creation. > So we should probably pass the whitelist in the constructor/initializer > instead. I agree. I want them to be immutable as well.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23) > Nor do we really need caps and certificates for it > so I can just leave them empty like for system pincipal, and even if we do I > can still abstract those functions out into a shared base class. Yeah, Hm. What does jetpack do with SSL? Do we want addons to be able to run with SSL principals? Maybe not. > it would be simply cu.Sandbox([...some white list...]); This sounds like the optimal API IMO.

Comment on attachment 618693 [details] [diff] [review] proto Review of attachment 618693 [details] [diff] [review]: ----------------------------------------------------------------- I read through the patch pretty quickly... Here are a few things that caught my attention right away. Overall, this seems like a decent approach to me, though. ::: caps/include/nsPrincipal.h @@ +176,5 @@ > + NS_IMETHODIMP CheckMayLoad(nsIURI* uri, bool aReport); > + NS_IMETHODIMP GetHashValue(PRUint32 *result); > + > + // Note that this function calls SwapElements on the input array > + void Extend(nsTArray<nsCOMPtr<nsIPrincipal>>& aWhiteList); Are we requiring C++0x yet? If not, this (and the list below) should add a space between the two >s. ::: caps/src/nsPrincipal.cpp @@ +1270,5 @@ > + > +nsExtendedPrincipal::~nsExtendedPrincipal() > +{ > + SetSecurityPolicy(nsnull); > + delete mCapabilities; Given that nsExtendedPrincipal inherits from nsPrincipal, it seems like this is redundant and will cause a double deletion of mCapabilities if it's ever non-null. @@ +1292,5 @@ > +NS_IMETHODIMP > +nsExtendedPrincipal::Subsumes(nsIPrincipal *other, bool *result) > +{ > + nsresult rv; > + nsPrincipal::Subsumes(other, result); Missing "rv =" here, which should be done as part of the declaration of rv. ::: js/xpconnect/src/XPCComponents.cpp @@ +2882,5 @@ > + JSObject *global = JS_GetGlobalForObject(cx, thisobj); > + nsCOMPtr<nsISupports> inst; > + nsresult rv; > + inst = do_CreateInstance("@mozilla.org/xmlextras/xmlhttprequest;1", &rv); > + nsContentUtils::WrapNative(cx, global, inst, vp); Error checking! @@ +3161,5 @@ > > // Make sure to set up principals on the sandbox before initing classes > nsCOMPtr<nsIScriptObjectPrincipal> sop; > nsCOMPtr<nsIPrincipal> principal; > + nsCOMPtr<nsExtendedPrincipal> extended; This should be an nsRefPtr, as it's a pointer to a concrete class. @@ +3259,5 @@ > + return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); > + } > + uint32_t length; > + JSObject *arrayObj = JSVAL_TO_OBJECT(option); > + if (!JS_IsArrayObject(cx, arrayObj) || !JS_GetArrayLength(cx, arrayObj, &length) || !length) Why disallow 0-length arrays here? @@ +3266,5 @@ > + nsTArray<nsCOMPtr<nsIPrincipal>> allowedDomains(length); > + allowedDomains.SetLength(length); > + for (uint32_t i = 0; i < length; ++i) { > + jsval urlVal; > + if (!JS_GetElement(cx, arrayObj, i, &urlVal) || !JSVAL_IS_STRING(urlVal)) IMO this list should also take windows or principal objects so that pages which use document.domain will work correctly. @@ +3280,5 @@ > + allowedDomains[i] = urlPrincipal; > + } > + extended = new nsExtendedPrincipal(); > + nsCOMPtr<nsIURI> baseURI; > + rv = principal->GetURI(getter_AddRefs(baseURI)); What about document.domain here? Is one not allowed to pass a document.domain'd principal and have an extended principal? It seems like you want an Init function on nsExtendedPrincipal that takes an nsPrincipal and really clones it.

> >+class DomainPolicy; > > Curious what this is about. So this is because I include nsPrincipal in XPCComponents.cpp and in that object the class is not predefined yet. I think I addressed more or less all the issues you guys pointed out in one way or another, except this version differs from the previous version so much that some of them is not an issue any more. Conceptually it did not make much sense for the ExpandedPrincipal to have a main principal so I just don't inherit it from nsPrincipal. What I did is that I broke nsPrincipal into two classes. One abstract class that takes care of caps and certificates: nsBasePrincipal (I guess a rename should be good here...) and the subclasses are nsPrincipal and nsExpandedPrincipal dealing with things like Equal, Subsumes. Based on the comments maybe even the system principal could be inherited from nsBasePrincipal but not sure if it adds any real value. What is still missing is a better Equal/Subsume function for nsExpandedPrincipal that works nicely for two ExpandedPrincipals, move the tests from xpconnect to caps, and add moar tests. Anyway, a feedback would be great before I continue since it was a bit more work than I expected to get this working, so I'm wondering what you guys think so far.

Ok. So this one is not finished obviously, since it's not optional. What I really dislike is adding more and more option flags and then cary over them to xpc_CreateSandboxObject. How about I just pass the option object and fetch all these custom option flags (wantThis, wantThat, etc...) in xpc_CreateSandboxObject where they are actually used? Would that work for you?

And the last one is only for feedback as well not for review ofc...

> What I did is that I broke nsPrincipal into two classes. One abstract class > that takes care of caps and certificates: nsBasePrincipal (I guess a rename > should be good here...) and the subclasses are nsPrincipal and > nsExpandedPrincipal dealing with things like Equal, Subsumes. Sounds good. > Based on the > comments maybe even the system principal could be inherited from > nsBasePrincipal but not sure if it adds any real value. IMO everything that lives in nsBasePrincipal is stuff that we want to kill, and just don't have the resources to do yet. So if nsSystemPrincipal works fine without it, let's just keep it that way.

Comment on attachment 622826 [details] [diff] [review] Bug 734891 - Decoupling URI based logic from caps/certificate related logic of nsPrincipal As discussed on IRC, this would be easier to review of the reordering of method definitions happened in a separate patch.

Comment on attachment 622829 [details] [diff] [review] part 2: adding ExpandedPrincipal support Seems reasonable in general. I'd like to split out the "implement nsEP" and "use nsEP in XPCComponents" into separate patches, though.

Comment on attachment 622833 [details] [diff] [review] part3: adding XHR constructor to sandbox Hmm, CreateXMLHttpRequest doesn't seem to be used anywhere. How do the tests pass?

(In reply to Bobby Holley (:bholley) from comment #34) > Comment on attachment 622833 [details] [diff] [review] > part3: adding XHR constructor to sandbox > > Hmm, CreateXMLHttpRequest doesn't seem to be used anywhere. How do the tests > pass? Ah yeah... I wanted to make the constructor optional, then realized that for that I would like to do some more changes that should be a separated patch. So I just put the line back for this patch except that I forgot to qref... sorry about that. Anyway, what do you think about passing the options object to xpc_CreateSandboxObject instead of these wantXray, wantComponents, wantXHRConstructor, etc flags?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #35) > (In reply to Bobby Holley (:bholley) from comment #34) > > Comment on attachment 622833 [details] [diff] [review] > > part3: adding XHR constructor to sandbox > > > > Hmm, CreateXMLHttpRequest doesn't seem to be used anywhere. How do the tests > > pass? > > Ah yeah... I wanted to make the constructor optional, then realized that for > that I would like to do some more changes that should be a separated patch. > So I just put the line back for this patch except that I forgot to qref... > sorry about that. Anyway, what do you think about passing the options object > to xpc_CreateSandboxObject instead of these wantXray, wantComponents, > wantXHRConstructor, etc flags? I think we should have a separate function to parse the options object, and have some sort of: struct SandboxOptions { bool wantXrays : 1, bool wantComponents : 1, bool wantXHRConstructor : 1 }; And just pass that around.

This patch is just for the easier review. No real changes just swapping around the methods to match the order they will have after the splitting up of NSPrincipal.

I removed an assertion, because that flag does not belong now to nsBasePrincipal... if you think that was an important one I'll come up with something.

More or less the old stuff, except now subsumes works for the expanded/expanded case too. I don't think it would help if I ordered the principals in the array.

Sandbox creation got a bit messy with all the new options adding up. This way it will be simpler to add new option flags (wantXHRConstructor, wantComponents, and I'm pretty sure many will follow) I hope the code is also a bit more readable now. But you'll let me know.

I've added a few more tests too.

I've added optional constructor and async test. I'm not saying that everything is perfect but I think it's kind of ready for a thorough review. Ms2ger: ofc you are welcome to check the rest of the patches too, just I promised you to put you on r? for the refactoring one...

Comment on attachment 624757 [details] [diff] [review] part 1: Decoupling URI based logic from caps/certificate related logic of nsPrincipal Review of attachment 624757 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/include/nsPrincipal.h @@ +73,5 @@ > + NS_IMETHOD GetPrettyName(nsACString & aPrettyName); > + NS_IMETHOD GetSubjectName(nsACString & aSubjectName); > + NS_IMETHOD GetCertificate(nsISupports * *aCertificate); > + NS_IMETHOD GetCsp(nsIContentSecurityPolicy * *aCsp); > + NS_IMETHOD SetCsp(nsIContentSecurityPolicy *aCsp); * and & on the left for all those, please. @@ +161,5 @@ > + NS_IMETHOD SetDomain(nsIURI *aDomain); > + NS_IMETHOD GetOrigin(char * *aOrigin); > + NS_IMETHOD Subsumes(nsIPrincipal *other, bool *_retval NS_OUTPARAM); > + NS_IMETHOD SubsumesIgnoringDomain(nsIPrincipal *other, bool *_retval NS_OUTPARAM); > + NS_IMETHOD CheckMayLoad(nsIURI *uri, bool report); id. @@ +169,5 @@ > + nsresult Init(const nsACString& aCertFingerprint, > + const nsACString& aSubjectName, > + const nsACString& aPrettyName, > + nsISupports* aCert, > + nsIURI *aCodebase); And here ::: caps/src/nsPrincipal.cpp @@ +119,5 @@ > "Installing gCodeBasePrincipalSupport failed!"); > } > } > > +nsBasePrincipal::~nsBasePrincipal(void) Kill the 'void' @@ +257,1 @@ > PRInt16 canEnable) Indentation, also below.

Comment on attachment 624759 [details] [diff] [review] part 2: Adding ExpandedPrincipal support Review of attachment 624759 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/include/nsPrincipal.h @@ +215,5 @@ > + NS_IMETHOD Subsumes(nsIPrincipal *other, bool *_retval NS_OUTPARAM); > + NS_IMETHOD SubsumesIgnoringDomain(nsIPrincipal *other, bool *_retval NS_OUTPARAM); > + NS_IMETHOD CheckMayLoad(nsIURI *uri, bool report); > + > + virtual void GetScriptLocation(nsACString &aStr) MOZ_OVERRIDE; <-- ::: caps/src/nsPrincipal.cpp @@ +1276,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +nsExpandedPrincipal::GetOrigin(char * *aOrigin) Ugh, 'string's... We're killing this soon enough, I hope? @@ +1305,5 @@ > + nsTArray< nsCOMPtr<nsIPrincipal> > *otherList; > + if (expanded) { > + // if aOther is an ExpandedPrincipal too, check if all of its principals are subsumed > + expanded->GetWhiteList(&otherList); > + for (uint32_t i=0; i<otherList->Length(); ++i){ 'i = 0', 'i < otherList->Length()' @@ +1306,5 @@ > + if (expanded) { > + // if aOther is an ExpandedPrincipal too, check if all of its principals are subsumed > + expanded->GetWhiteList(&otherList); > + for (uint32_t i=0; i<otherList->Length(); ++i){ > + rv = Subsumes((*otherList)[i], aResult); Declare rv here @@ +1308,5 @@ > + expanded->GetWhiteList(&otherList); > + for (uint32_t i=0; i<otherList->Length(); ++i){ > + rv = Subsumes((*otherList)[i], aResult); > + if (NS_FAILED(rv)) > + return rv; NS_ENSURE_SUCCESS(rv, rv); @@ +1319,5 @@ > + // for a regular aOther, one of our principals must subsume it > + for (uint32_t i=0; i<mPrincipals.Length(); ++i){ > + rv = mPrincipals[i]->Subsumes(aOther, aResult); > + if (NS_FAILED(rv)) > + return rv; Same × 3 @@ +1331,5 @@ > +} > + > +NS_IMETHODIMP > +nsExpandedPrincipal::SubsumesIgnoringDomain(nsIPrincipal *aOther, bool *aResult) > +{ Again. I assume de-duplicating this code is hard-ish @@ +1361,5 @@ > +NS_IMETHODIMP > +nsExpandedPrincipal::CheckMayLoad(nsIURI* uri, bool aReport) > +{ > + nsresult rv; > + for (uint32_t i=0; i<mPrincipals.Length(); ++i){ for (uint32_t i = 0; i < mPrincipals.Length(); ++i) { @@ +1364,5 @@ > + nsresult rv; > + for (uint32_t i=0; i<mPrincipals.Length(); ++i){ > + rv = mPrincipals[i]->CheckMayLoad(uri, aReport); > + if (!NS_FAILED(rv)) > + return rv; . @@ +1374,5 @@ > +NS_IMETHODIMP > +nsExpandedPrincipal::GetHashValue(PRUint32 *result) > +{ > + // extended principal should never be used as key in a hash map > + MOZ_ASSERT(false, "nsExpandedPrincipal::GetHashValue"); MOZ_NOT_REACHED @@ +1389,5 @@ > +NS_IMETHODIMP > +nsExpandedPrincipal::GetWhiteList(nsTArray<nsCOMPtr<nsIPrincipal> > **aWhiteList) > +{ > + *aWhiteList = &mPrincipals; > + return NS_OK; Indentation ::: caps/src/nsScriptSecurityManager.cpp @@ +1450,5 @@ > + nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aPrincipal); > + if (expanded) { > + nsTArray< nsCOMPtr<nsIPrincipal> > *whiteList; > + expanded->GetWhiteList(&whiteList); > + for (uint32_t i=0; i<whiteList->Length(); ++i) { .

Comment on attachment 624762 [details] [diff] [review] part 3: Cleaning up sandbox creation Review of attachment 624762 [details] [diff] [review]: ----------------------------------------------------------------- You asked :) ::: js/xpconnect/src/XPCComponents.cpp @@ +3166,5 @@ > return true; > } > > nsresult > +xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop, SandboxOptions& options) * to the... Right, I guess. @@ +3226,1 @@ > if (!xpc::WrapperFactory::WaiveXrayAndWrap(cx, &v)) Can has an overload that takes JSObject**? Followup :) @@ +3314,5 @@ > +// for sandbox constructor the first argument can be a URI string in which case > +// we use the related Codebase Principal for the sandbox > +nsresult > +GetPrincipalFromString(JSContext *cx, jsval from, nsIPrincipal **principal) > +{ MOZ_ASSERT(principal); @@ +3315,5 @@ > +// we use the related Codebase Principal for the sandbox > +nsresult > +GetPrincipalFromString(JSContext *cx, jsval from, nsIPrincipal **principal) > +{ > + JSString *codebaseStr = from.toString(); I'd prefer passing a JSString* for type-safety. @@ +3317,5 @@ > +GetPrincipalFromString(JSContext *cx, jsval from, nsIPrincipal **principal) > +{ > + JSString *codebaseStr = from.toString(); > + size_t codebaseLength; > + nsresult rv; Move to its first use @@ +3324,5 @@ > + if (!codebaseChars) > + return NS_ERROR_FAILURE; > + > + > + nsAutoString codebase(codebaseChars, codebaseLength); nsDependentJSString, I think @@ +3328,5 @@ > + nsAutoString codebase(codebaseChars, codebaseLength); > + nsCOMPtr<nsIURI> uri; > + rv = NS_NewURI(getter_AddRefs(uri), codebase); > + if (NS_FAILED(rv)) > + return rv; NS_ENSUR... @@ +3334,5 @@ > + nsCOMPtr<nsIScriptSecurityManager> secman = > + do_GetService(kScriptSecurityManagerContractID); > + if (!secman || > + NS_FAILED(rv = secman->GetCodebasePrincipal(uri,principal)) || > + !principal) { !*principal @@ +3337,5 @@ > + NS_FAILED(rv = secman->GetCodebasePrincipal(uri,principal)) || > + !principal) { > + if (NS_SUCCEEDED(rv)) > + rv = NS_ERROR_FAILURE; > + return rv; NS_ENSURE_TRUE(secman, NS_ERROR_FAILURE); rv = secman->GetCodebasePrincipal(uri, principal); NS_ENSURE_SUCCESS(rv, rv); NS_ENSURE_TRUE(*principal, NS_ERROR_FAILURE); @@ +3346,5 @@ > + > +// for sandbox constructor the first argument can be a principal object or > +// a script object principal (Document, Window) > +nsresult > +GetPrincipalOrSOP(JSContext *cx, jsval from, nsISupports **out) Pass a JSObject& @@ +3350,5 @@ > +GetPrincipalOrSOP(JSContext *cx, jsval from, nsISupports **out) > +{ > + MOZ_ASSERT(out); > + nsCOMPtr<nsIScriptObjectPrincipal> sop; > + nsCOMPtr<nsIPrincipal> principal; Move down @@ +3351,5 @@ > +{ > + MOZ_ASSERT(out); > + nsCOMPtr<nsIScriptObjectPrincipal> sop; > + nsCOMPtr<nsIPrincipal> principal; > + *out = nsnull; NULL @@ +3353,5 @@ > + nsCOMPtr<nsIScriptObjectPrincipal> sop; > + nsCOMPtr<nsIPrincipal> principal; > + *out = nsnull; > + > + nsCOMPtr<nsIXPConnect> xpc(do_GetService(nsIXPConnect::GetCID())); nsXPConnect::Get()? @@ +3360,5 @@ > + nsCOMPtr<nsIXPConnectWrappedNative> wrapper; > + xpc->GetWrappedNativeOfJSObject(cx, &from.toObject(), > + getter_AddRefs(wrapper)); > + > + if (wrapper) { NS_ENSURE_TRUE(wrapper, NS_ERROR_INVALID_ARG); @@ +3362,5 @@ > + getter_AddRefs(wrapper)); > + > + if (wrapper) { > + sop = do_QueryWrappedNative(wrapper); > + if (sop) { if (nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryWrappedNative(wrapper)) { sop.forget(out); return NS_OK; } @@ +3369,5 @@ > + } else { > + principal = do_QueryWrappedNative(wrapper); > + if (principal) > + NS_ADDREF(principal); > + *out = principal; nsCOMPtr<nsIPrincipal> principal = do_QueryWrappedNative(wrapper); principal.forget(out); @@ +3378,5 @@ > +} > + > +// helper that tries to get a property form the options object > +nsresult > +GetPropFromOptions(JSContext* cx, JSObject* from, const char* name, jsval& prop, JSBool& found) JSObject&, jsval*, bool* @@ +3391,5 @@ > +} > + > +// helper that tries to get a boolean property form the options object > +nsresult > +GetBoolPropFromOptions(JSContext* cx, JSObject* from, const char* name, bool& prop) JSObject&, bool* @@ +3410,5 @@ > +} > + > +// helper that tries to get an object property form the options object > +nsresult > +GetObjPropFromOptions(JSContext* cx, JSObject* from, const char* name, JSObject* prop) JSObject& from. This doesn't currently work; prop should be JSObject** @@ +3430,5 @@ > + prop = &propVal.toObject(); > + return NS_OK; > +} > + > +// helper that tries to get an object property form the options object s/object/string/ @@ +3432,5 @@ > +} > + > +// helper that tries to get an object property form the options object > +nsresult > +GetStringPropFromOptions(JSContext* cx, JSObject* from, const char* name, nsCString& prop) JSObject& @@ +3461,5 @@ > +{ > + if (!from.isObject()) > + return NS_ERROR_INVALID_ARG; > + > + JSObject *optionsObject = &from.toObject(); JSObject &optionsObject = from.toObject(); @@ +3464,5 @@ > + > + JSObject *optionsObject = &from.toObject(); > + > + if (NS_FAILED(GetObjPropFromOptions(cx, optionsObject, "sandboxPrototype", options.proto))) > + return NS_ERROR_INVALID_ARG; rv = Foo(); NS_ENSURE_SUCCESS(rv, rv); for these @@ +3524,3 @@ > prinOrSop = principal; > + } else if (argv[0].isPrimitive()) { > + return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); This is weird... How about if (isString()) {} else if (isObject()) {} else { Throw } @@ +3527,5 @@ > + } else > + rv = GetPrincipalOrSOP(cx, argv[0], getter_AddRefs(prinOrSop)); > + > + if (NS_FAILED(rv)) > + return ThrowAndFail(rv, cx, _retval); Won't you still stomp over exceptions here? @@ +3537,5 @@ > > // If there is no options object given, or no sandboxName property > // specified, use the caller's filename as sandboxName. > + if (options.sandboxName.IsEmpty() && > + NS_FAILED(GetSandboxNameFromStack(cx, options.sandboxName))) Indentation ::: js/xpconnect/src/xpcprivate.h @@ +4452,5 @@ > { > return js::GetObjectPrivate(obj); > } > > +struct SandboxOptions { Should be namespaced somehow...

Comment on attachment 624763 [details] [diff] [review] part 4: Using ExpandedPrincipal Review of attachment 624763 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +3379,5 @@ > > +// the first parameter of the sandbox constructor might be an array of principals, either in string > +// format or actual objects (see GetPrincipalOrSOP) > +nsresult > +GetExpandedPrincipal(JSContext *cx, jsval from, nsIExpandedPrincipal **out) Pass JSObject& @@ +3386,5 @@ > + JSObject *arrayObj = &from.toObject(); > + nsresult rv; > + uint32_t length; > + nsCOMPtr<nsISupports> prinOrSop; > + nsCOMPtr<nsIPrincipal> principal; Declare those when you need them @@ +3399,5 @@ > + return NS_ERROR_INVALID_ARG; > + } > + > + nsTArray< nsCOMPtr<nsIPrincipal> > allowedDomains(length); > + allowedDomains.SetLength(length); Do you need to tell it the length twice? @@ +3410,5 @@ > + // in case of string let's try to fetch a codebase principal from it > + rv = GetPrincipalFromString(cx, allowed, getter_AddRefs(principal)); > + if (NS_FAILED(rv)) > + return rv; > + } else if (!allowed.isPrimitive()) { allowed.isObject() @@ +3425,5 @@ > + allowedDomains[i] = principal; > + } else { > + return NS_ERROR_INVALID_ARG; > + } > + } else? @@ +3430,5 @@ > + > + allowedDomains[i] = principal; > + } > + *out = new nsExpandedPrincipal(allowedDomains); > + NS_ADDREF(*out); I prefer nsCOMPtr<nsIExpandedPrincipal> result = new nsExpandedPrincipal(allowedDomains); result.forget(out); @@ +3588,2 @@ > } else > rv = GetPrincipalOrSOP(cx, argv[0], getter_AddRefs(prinOrSop)); I forgot this for the last patch: {}

Comment on attachment 624767 [details] [diff] [review] part 5: Adding optional XHR constructor to sandbox Review of attachment 624767 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +2986,5 @@ > + nsCOMPtr<nsISupports> inst; > + nsresult rv; > + inst = do_CreateInstance("@mozilla.org/xmlextras/xmlhttprequest;1", &rv); > + if (NS_FAILED(rv)) > + return false; I'm pretty sure you need to initialize this somehow. @@ +3277,5 @@ > if (NS_FAILED(rv)) > return NS_ERROR_XPC_UNEXPECTED; > + > + if (options.wantXHRConstructor && > + !JS_DefineFunction(cx, sandbox, "XMLHttpRequest", CreateXMLHttpRequest, 0, JSFUN_CONSTRUCTOR)) Indentation. ::: js/xpconnect/src/xpcprivate.h @@ +4465,2 @@ > JSObject* proto; > nsCString sandboxName; These should ideally be ordered with the largest types first

We talked about the nsIPrincipal funcion declarations in nsPrincipal.h with bholley on irc. It would be great to get rid of them and hide behind the regular NS_DECL_ macro. For that the nsIPrincipal should be split into 2 interfaces, one for the simplified nsIPrincipal and one for the rest of the stuff we want to get rid of in the future (caps+cert).It seems like it's a bit more work than it seemed, the only reason I continued this patch this far is that because I thought it gives a good overview about how much stuff will have to be changed to get rid of this part of caps. So the only reason I think it might wort to finish this patch, is if you say that this change will help later. Note, that this is not a finished patch, probably contains bugs, I just barely made it to compile and its only purpose to give an overview.

Comment on attachment 624757 [details] [diff] [review] part 1: Decoupling URI based logic from caps/certificate related logic of nsPrincipal > >-class nsPrincipal : public nsJSPrincipals >+class nsBasePrincipal : public nsJSPrincipals ... > >+#ifdef DEBUG >+ virtual void dumpImpl() MOZ_OVERRIDE; >+#endif >+ Per IRC discussion, this shouldn't live in nsBasePrincipal.

Comment on attachment 624759 [details] [diff] [review] part 2: Adding ExpandedPrincipal support Please add an extensive comment before the declaration of nsIExpandedPrincipal explaining what this is all about. >+{ >+ /** >+ * An array of principals that the expanded principal subsumes. >+ */ >+ [noscript] readonly attribute PrincipalArray whiteList; Mark this [notxpcom] and make an explicit note that the underlying list is shared, not reference-counted, and should only be used ephemerally by callers. > nsExpandedPrincipal::nsExpandedPrincipal(nsTArray<nsCOMPtr <nsIPrincipal> > &aWhiteList) Can we loop over these in debug mode and assert that none of them is also an EP, a system principal, or a null principal? Or maybe null principal is ok. >+NS_IMETHODIMP >+nsExpandedPrincipal::Equals(nsIPrincipal *aOther, bool *aResult) >+{ >+ *aResult = (aOther == this); >+ return NS_OK; >+} This should be this->Subsumes(aOther) && aOther->Subsumes(this). >+nsExpandedPrincipal::EqualsIgnoringDomain(nsIPrincipal *aOther, bool *aResult) And this should be this->SubsumesIgnoringDomain(aOther) && aOther->SubsumesIgnoringDomain(this) This part is very very important. Getting things wrong with respect to document.domain has the potential to break a lot of assumptions in XPConnect and introduce serious security bugs. >+NS_IMETHODIMP >+nsExpandedPrincipal::Subsumes(nsIPrincipal *aOther, bool *aResult) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aOther); >+ nsTArray< nsCOMPtr<nsIPrincipal> > *otherList; This should be declared within the subsequent conditional block. >+ // if it fails for one principal, return false Change this to: // If we don't subsume at least one principal of aOther, return false. >+ >+NS_IMETHODIMP >+nsExpandedPrincipal::SubsumesIgnoringDomain(nsIPrincipal *aOther, bool *aResult) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIExpandedPrincipal> expanded = do_QueryInterface(aOther); >+ nsTArray< nsCOMPtr<nsIPrincipal> > *otherList; >+ if (expanded) { >+ expanded->GetWhiteList(&otherList); >+ for (uint32_t i=0; i<otherList->Length(); ++i){ >+ rv = SubsumesIgnoringDomain((*otherList)[i], aResult); >+ if (NS_FAILED(rv)) >+ return rv; >+ if (!*aResult) { >+ return NS_OK; >+ } >+ } >+ } else { >+ for (uint32_t i=0; i<mPrincipals.Length(); ++i){ >+ rv = mPrincipals[i]->SubsumesIgnoringDomain(aOther, aResult); >+ if (NS_FAILED(rv)) >+ return rv; >+ if (*aResult) >+ return NS_OK; >+ } >+ } >+ return NS_OK; >+} IIUC, this is just the same as above, but with Subsumes replaced with SubsumesIgnoringDomain. Can we combine them into a templatized method? >+ // Is that a good idea to list it's principals? I think the current implementation is fine. Let's remove this comment. >+ >+////////////////////////////////////////// >+// Methods implementing nsISerializable // >+////////////////////////////////////////// >+ >+NS_IMETHODIMP >+nsExpandedPrincipal::Read(nsIObjectInputStream* aStream) >+{ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP >+nsExpandedPrincipal::Write(nsIObjectOutputStream* aStream) >+{ >+ return NS_OK; >+} I think these things definitely need to throw NS_ERROR_NOT_IMPLEMENTED. If they're used, then we need to implement them.

Comment on attachment 624762 [details] [diff] [review] part 3: Cleaning up sandbox creation I'm assuming the code that you factored out into a helper method didn't change significantly. I didn't look at it closely at all. Looks like Ms2ger did, though. ;-)

Adding dev-doc-needed for the API change to sandbox creation.

(In reply to Bobby Holley (:bholley) from comment #51) > Comment on attachment 624762 [details] [diff] [review] > part 3: Cleaning up sandbox creation Oh, also for this one, can you make the |else if| condition |.isObject()| and make the |else| condition throw?

(In reply to Bobby Holley (:bholley) from comment #53) > (In reply to Bobby Holley (:bholley) from comment #51) > > Comment on attachment 624762 [details] [diff] [review] > > part 3: Cleaning up sandbox creation > > Oh, also for this one, can you make the |else if| condition |.isObject()| > and make the |else| condition throw? Oh, looks like Ms2ger already caught that in comment 45.

Comment on attachment 624763 [details] [diff] [review] part 4: Using ExpandedPrincipal I'm assuming you'll be addressing Ms2ger's review comments (for this patch and the others). I won't repeat them here. >+ >+ nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(prinOrSop)); >+ principal = do_QueryInterface(prinOrSop); >+ if (sop) { >+ allowedDomains[i] = sop->GetPrincipal(); >+ } else if (principal) { >+ allowedDomains[i] = principal; >+ } else { >+ return NS_ERROR_INVALID_ARG; >+ } >+ } >+ >+ allowedDomains[i] = principal; I don't like assigning to allowedDomains[i] conditionally and then unconditionally. Make the previous code compute |principal|, and then assign to allowedDomains[i] unconditionally at the bottom. >+ } >+ *out = new nsExpandedPrincipal(allowedDomains); >+ NS_ADDREF(*out); >+ return NS_OK; >+} >+ > // helper that tries to get a property form the options object > nsresult > GetPropFromOptions(JSContext* cx, JSObject* from, const char* name, jsval& prop, JSBool& found) > { > if (!JS_HasProperty(cx, from, name, &found)) > return NS_ERROR_INVALID_ARG; > > if (found && !JS_GetProperty(cx, from, name, &prop)) >@@ -3519,16 +3577,19 @@ nsXPCComponents_utils_Sandbox::CallOrCon > nsCOMPtr<nsISupports> prinOrSop; > nsISupports *identity = nsnull; > > if (argv[0].isString()) { > rv = GetPrincipalFromString(cx, argv[0], getter_AddRefs(principal)); > prinOrSop = principal; > } else if (argv[0].isPrimitive()) { > return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); >+ } else if (JS_IsArrayObject(cx, &argv[0].toObject())) { >+ rv = GetExpandedPrincipal(cx, argv[0], getter_AddRefs(expanded)); >+ prinOrSop = expanded; Where is |expanded| declared? How did this compile? > } else > rv = GetPrincipalOrSOP(cx, argv[0], getter_AddRefs(prinOrSop)); > > if (NS_FAILED(rv)) > return ThrowAndFail(rv, cx, _retval); > > SandboxOptions options; > >diff --git a/js/xpconnect/tests/unit/test_allowedDomains.js b/js/xpconnect/tests/unit/test_allowedDomains.js >new file mode 100644 >--- /dev/null >+++ b/js/xpconnect/tests/unit/test_allowedDomains.js >@@ -0,0 +1,59 @@ >+function run_test() { >+ var cu = Components.utils; >+ var master = cu.Sandbox(["http://www.a.com", >+ "http://www.b.com", >+ "http://www.d.com"]); >+ var subset = cu.Sandbox(["http://www.d.com", >+ "http://www.a.com"]); >+ >+ var a = cu.Sandbox("http://www.a.com"); >+ var b = cu.Sandbox("http://www.b.com"); >+ var c = cu.Sandbox("http://www.c.com"); >+ >+ master.a = cu.evalInSandbox("var obj = {prop1:200}; obj", a); >+ master.b = cu.evalInSandbox("var obj = {prop1:200}; obj", b); >+ master.c = cu.evalInSandbox("var obj = {prop1:200}; obj", c); >+ master.own = cu.evalInSandbox("var obj = {prop1:200}; obj", master); The eval-ed string here can always just be |{prop1:200}|. >+ >+ master.subset = cu.evalInSandbox("var obj = {prop1:200}; obj", subset); >+ a.master = master; >+ subset.master = master; This is confusing, and (I think) incorrect. In all the other examples, |foo.bar| means that |foo| is a sandbox and |bar| is an object evaluated from the sandbox |bar|. In this case, a.master is a sandbox. I think you should change the naming scheme to |foo.obj_bar| or something to avoid this kind of confusion. >+ try { >+ ret = cu.evalInSandbox("c.prop1", master); >+ do_throw("unexpected pass"); >+ } catch (e) { >+ do_check_true(e.message && e.message.indexOf("Permission denied to access property") != -1); >+ } In general, I think this pattern is kind of confusing. I would prefer to just do |do_check_true(false, ...)| if it doesn't throw. That prevents the catch block from being overloaded with both cases. Also, can we move the do_check_true logic into a helper? >+ try { >+ ret = cu.evalInSandbox("master.own", subset); >+ do_check_eq(ret, undefined); >+ do_throw("unexpected pass"); >+ } catch (e) { >+ do_check_true(e.message && e.message.indexOf("Permission denied to access property") != -1); >+ } Since |master| here is a sandbox object, this will fail because subset can't access the global, not because it can't access the object. Is this intentional? If so, I'd like to check both cases. >+ >+ try { >+ ret = cu.evalInSandbox("master.b", subset); >+ do_throw("unexpected pass"); >+ } catch (e) { >+ do_check_true(e.message && e.message.indexOf("Permission denied to access property") != -1); >+ } Same problem here. If you change the eval to "master.a" it will still fail, despite the fact that subset should be able to access a. This needs some fixing in various places, but r+ because I don't really want to look at it again. ;-)

(In reply to Bobby Holley (:bholley) from comment #50) > Comment on attachment 624759 [details] [diff] [review] > part 2: Adding ExpandedPrincipal support Oh, we'll also need a patch to re-implement nsPrincipal::Equals{,IgnoringDomain} in terms of Subsumes. Otherwise, EPs will only work if they're on the left side of an equals comparison, which is wrong. Such a patch blocks landing this stuff IMO.

Comment on attachment 624767 [details] [diff] [review] part 5: Adding optional XHR constructor to sandbox Looks good from my end, but flagging smaug for a review on CreateXMLHttpRequest and the associated test.

Comment on attachment 625575 [details] [diff] [review] splitting nsIPrincipal (after part 2) Thanks for looking into that gabor - it's a very useful data point. Sounds like we'll skip it for the time being. ;-)

Comment on attachment 624767 [details] [diff] [review] part 5: Adding optional XHR constructor to sandbox > static JSBool >+CreateXMLHttpRequest(JSContext *cx, unsigned argc, jsval *vp) >+{ >+ JSObject *global = JS_GetGlobalForScopeChain(cx); >+ MOZ_ASSERT(global); >+ >+ nsCOMPtr<nsISupports> inst; >+ nsresult rv; >+ inst = do_CreateInstance("@mozilla.org/xmlextras/xmlhttprequest;1", &rv); >+ if (NS_FAILED(rv)) >+ return false; >+ >+ rv = nsContentUtils::WrapNative(cx, global, inst, vp); >+ if (NS_FAILED(rv)) >+ return false; >+ >+ return true; >+} So how does this give the right principal to XHR?

(In reply to Olli Pettay [:smaug] from comment #59) > Comment on attachment 624767 [details] [diff] [review] > part 5: Adding optional XHR constructor to sandbox > > > static JSBool > >+CreateXMLHttpRequest(JSContext *cx, unsigned argc, jsval *vp) > >+{ > >+ JSObject *global = JS_GetGlobalForScopeChain(cx); > >+ MOZ_ASSERT(global); > >+ > >+ nsCOMPtr<nsISupports> inst; > >+ nsresult rv; > >+ inst = do_CreateInstance("@mozilla.org/xmlextras/xmlhttprequest;1", &rv); > >+ if (NS_FAILED(rv)) > >+ return false; > >+ > >+ rv = nsContentUtils::WrapNative(cx, global, inst, vp); > >+ if (NS_FAILED(rv)) > >+ return false; > >+ > >+ return true; > >+} > So how does this give the right principal to XHR? It should not give it. The nsXMLHttpRequest::Init receiving the principal from the security manager, so when we call this ctor within an evalInSandbox, it will get the principal of the sandbox.

Do we tests for that behavior? And which principal does the XHR get?

(In reply to Olli Pettay [:smaug] from comment #61) > Do we tests for that behavior? And which principal does the XHR get? I can add a test to try to do a request to another port for example it should not have access... It should not matter where we do the XHR get from, since the nsXHR stores the principal at creation time.

But what kind of principal does the XHR get? I assume some other patch in this bug pushes some special principal to stack.

(In reply to Olli Pettay [:smaug] from comment #63) > But what kind of principal does the XHR get? > I assume some other patch in this bug pushes some special principal to stack. It gets a so called nsExpandedPrincipal. It is basically an array of regular principals. If a principal is in the array, the nsExpandedPrincipal subsumes it, hence it gains same origin access to that origin. So for sandbox the user can specify instead of a single principal, an array of principals now, and the XHR created from that sandbox will be capable of sending requests to those origins.

Comment on attachment 624762 [details] [diff] [review] part 3: Cleaning up sandbox creation Review of attachment 624762 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +3240,1 @@ > sandbox); Indentation ::: js/xpconnect/src/xpcprivate.h @@ +4452,5 @@ > { > return js::GetObjectPrivate(obj); > } > > +struct SandboxOptions { But we don't indent stuff in namespaces. @@ +4454,5 @@ > } > > +struct SandboxOptions { > + SandboxOptions() > + : wantXrays(true) And four spaces, I guess.

Comment on attachment 624763 [details] [diff] [review] part 4: Using ExpandedPrincipal Review of attachment 624763 [details] [diff] [review]: ----------------------------------------------------------------- Moar nits ::: js/xpconnect/src/XPCComponents.cpp @@ +3399,5 @@ > + return NS_ERROR_INVALID_ARG; > + } > + > + nsTArray< nsCOMPtr<nsIPrincipal> > allowedDomains(length); > + allowedDomains.SetLength(length); Still wondering @@ +3409,5 @@ > + if (allowed.isString()) { > + // in case of string let's try to fetch a codebase principal from it > + rv = GetPrincipalFromString(cx, allowed, getter_AddRefs(principal)); > + if (NS_FAILED(rv)) > + return rv; NS_ENSURE_SUCCESS, please ::: js/xpconnect/tests/unit/xpcshell.ini @@ +25,5 @@ > [test_unload.js] > [test_attributes.js] > [test_params.js] > [test_components.js] > +[test_allowedDomains.js] \ No newline at end of file Add a line at the end of the file, please

Ms2ger: > @@ +3226,1 @@ > > if (!xpc::WrapperFactory::WaiveXrayAndWrap(cx, &v)) > > Can has an overload that takes JSObject**? Followup :) Uhh, I guess... can you explain? > > +nsresult > > +GetPrincipalFromString(JSContext *cx, jsval from, nsIPrincipal **principal) > > +{ > > + JSString *codebaseStr = from.toString(); > > I'd prefer passing a JSString* for type-safety. I had that first... Then kind of liked the idea to have all these utility getter functions work with the same jsval args. But as you wish. > > JSObject& from. > > This doesn't currently work; prop should be JSObject** Auch. I wanted to use *& there... anyway I changed it to ** and thanks for this! > @@ +3524,3 @@ > > prinOrSop = principal; > > + } else if (argv[0].isPrimitive()) { > > + return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); > > This is weird... How about > if (isString()) {} else if (isObject()) {} else { Throw } Yeah. So this only made sense with the follow-up patch. Anyway I changed it. > > @@ +3527,5 @@ > > + } else > > + rv = GetPrincipalOrSOP(cx, argv[0], getter_AddRefs(prinOrSop)); > > + > > + if (NS_FAILED(rv)) > > + return ThrowAndFail(rv, cx, _retval); > > Won't you still stomp over exceptions here? Hm... I'm not sure I get what you mean. If the getter throws? I don't think so. > > > > +struct SandboxOptions { > > Should be namespaced somehow... I added namespace but it's weird that the utility function that is using it is not namespaced but this struct is. > @@ +3399,5 @@ > > + return NS_ERROR_INVALID_ARG; > > + } > > + > > + nsTArray< nsCOMPtr<nsIPrincipal> > allowedDomains(length); > > + allowedDomains.SetLength(length); > > Do you need to tell it the length twice? you don't _have_ to. The first is setting the capacity right in the constructor, and then when you set length, there won't be any new allocation. Using dynamic containers I prefer this style if I know the size upfront. > ::: js/xpconnect/src/XPCComponents.cpp > @@ +2986,5 @@ > > + nsCOMPtr<nsISupports> inst; > > + nsresult rv; > > + inst = do_CreateInstance("@mozilla.org/xmlextras/xmlhttprequest;1", &rv); > > + if (NS_FAILED(rv)) > > + return false; > > I'm pretty sure you need to initialize this somehow. I think this guy calls a constructor and the right init for it too. > > ::: js/xpconnect/src/xpcprivate.h > @@ +4465,2 @@ > > JSObject* proto; > > nsCString sandboxName; > > These should ideally be ordered with the largest types first Changed it as you requested. However I prefer avoiding bools as last members in stucts/classes since I've seen some ugly crashes on some crazy platforms because of it. Then again, we are not likely to port ff to psp any time soon :)

bholley: > > IIUC, this is just the same as above, but with Subsumes replaced with > SubsumesIgnoringDomain. Can we combine them into a templatized method? Eeeh. I didn't like them either but I wanted to avoid introducing any code that involves member function pointers. Anyway I did it. If you have any better idea to avoid code duplication here let me know. > >+////////////////////////////////////////// > >+// Methods implementing nsISerializable // > >+////////////////////////////////////////// > >+ > >+NS_IMETHODIMP > >+nsExpandedPrincipal::Read(nsIObjectInputStream* aStream) > >+{ > >+ return NS_OK; > >+} > >+ > >+NS_IMETHODIMP > >+nsExpandedPrincipal::Write(nsIObjectOutputStream* aStream) > >+{ > >+ return NS_OK; > >+} > > I think these things definitely need to throw NS_ERROR_NOT_IMPLEMENTED. If > they're used, then we need to implement them. Yes. I still need to figure out where are these things used. So I can decide if we need them and if yes then at least I would know how to hell can I test them. > > // helper that tries to get a property form the options object > > nsresult > > GetPropFromOptions(JSContext* cx, JSObject* from, const char* name, jsval& prop, JSBool& found) > > { > > if (!JS_HasProperty(cx, from, name, &found)) > > return NS_ERROR_INVALID_ARG; > > > > if (found && !JS_GetProperty(cx, from, name, &prop)) > >@@ -3519,16 +3577,19 @@ nsXPCComponents_utils_Sandbox::CallOrCon > > nsCOMPtr<nsISupports> prinOrSop; > > nsISupports *identity = nsnull; > > > > if (argv[0].isString()) { > > rv = GetPrincipalFromString(cx, argv[0], getter_AddRefs(principal)); > > prinOrSop = principal; > > } else if (argv[0].isPrimitive()) { > > return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); > >+ } else if (JS_IsArrayObject(cx, &argv[0].toObject())) { > >+ rv = GetExpandedPrincipal(cx, argv[0], getter_AddRefs(expanded)); > >+ prinOrSop = expanded; > > Where is |expanded| declared? How did this compile? Auch. In the previous patch... Fixed it. > >+ > >+ master.a = cu.evalInSandbox("var obj = {prop1:200}; obj", a); > >+ master.b = cu.evalInSandbox("var obj = {prop1:200}; obj", b); > >+ master.c = cu.evalInSandbox("var obj = {prop1:200}; obj", c); > >+ master.own = cu.evalInSandbox("var obj = {prop1:200}; obj", master); > > The eval-ed string here can always just be |{prop1:200}|. I'd wish. So I tried it and with your proposal it's not working, prop1 is undefined. Hell, even using "var obj = {prop1:200}, obj" (note the , instead of ;) is broken, but differently, in that case even the object is undefined not only the prop1 on it. So in short, the wtf is strong with this one, I have no explanation here, but this seems completely unrelated to the patch. > > >+ > >+ master.subset = cu.evalInSandbox("var obj = {prop1:200}; obj", subset); > >+ a.master = master; > >+ subset.master = master; > > This is confusing, and (I think) incorrect. In all the other examples, > |foo.bar| means that |foo| is a sandbox and |bar| is an object evaluated > from the sandbox |bar|. In this case, a.master is a sandbox. I think you > should change the naming scheme to |foo.obj_bar| or something to avoid this > kind of confusion. Renamed everything to make it clear. You are totally right. > > >+ try { > >+ ret = cu.evalInSandbox("c.prop1", master); > >+ do_throw("unexpected pass"); > >+ } catch (e) { > >+ do_check_true(e.message && e.message.indexOf("Permission denied to access property") != -1); > >+ } > > In general, I think this pattern is kind of confusing. I would prefer to > just do |do_check_true(false, ...)| if it doesn't throw. That prevents the > catch block from being overloaded with both cases. That was the first version, but I hated that do_check_true is throwing an exception which I catch unwanted and report the same failure twice. Anyway I switched back to it, since this version is not much better either.

(In reply to Ms2ger from comment #66) > Comment on attachment 624763 [details] [diff] [review] > part 4: Using ExpandedPrincipal > > Moar nits Just when I thought I fixed up everything :)

(In reply to Bobby Holley (:bholley) from comment #56) > (In reply to Bobby Holley (:bholley) from comment #50) > > Comment on attachment 624759 [details] [diff] [review] > > part 2: Adding ExpandedPrincipal support > > Oh, we'll also need a patch to re-implement > nsPrincipal::Equals{,IgnoringDomain} in terms of Subsumes. Otherwise, EPs > will only work if they're on the left side of an equals comparison, which is > wrong. > > Such a patch blocks landing this stuff IMO. I'm not so sure about this one. I think if I don't change the current nsPrincipal::Equals (and the other guy) then for the P = Ep will always fail. Which is in only one case should not necessarily, in the trivial case when Ep has only one principal and we compare against that one. I'm not so sure if that is such an important case though... Am I missing something?

> but I wanted to avoid introducing any code that involves member function pointers. Hm, you can't just do a templated member function? C++ forbids that I guess? I'd also be fine with a macro FWIW. (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #70) > I'm not so sure about this one. I think if I don't change the current > nsPrincipal::Equals (and the other guy) then for the P = Ep will always > fail. Which is in only one case should not necessarily, in the trivial case > when Ep has only one principal and we compare against that one. I'm not so > sure if that is such an important case though... Am I missing something? Hm, I guess if we want the invariant than an EP should never equal or be subsumed by a regular principal it doesn't matter that much. Really, Equals and EqualsIgnoringDomain should live in nsBasePrincipal, and be implemented in terms of virtual Subsumes. But that does incur the double cost of Equals for nsPrincipals when it doesn't actually matter. I'm not sure how hot that code is...

I just update this because of the dumpImpl changed place too.

dumpImpl again and fixed nits.

Shared equal on nsBasePrincipal didn't really work out... I got rid of the code duplication, member function pointers... The reason why I use macro for calling those member function pointers, because it's got barely readable without the macros. I did the same thing for the equals function pair. Serialization is still a bit of a question if we need it and if do then for what... For now they simply throw. You suggested to assert in the nsEP ctor that there should not be a system principal in the array, but I do the check in the array parser code instead. I think it's not very likely that anyone will create an nsEP from c++ like that, on the other hand I think we need this check not just in release build too for create sandbox anyway.

Many-many nits fixed, also updated the tree and fixed the conflicts.

Updated the tests to make it less painful to read.

on try: https://tbpl.mozilla.org/?tree=Try&rev=8870d406a325

Comment on attachment 628270 [details] [diff] [review] part 3: Cleaning up sandbox creation Review of attachment 628270 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +3373,5 @@ > + if (NS_FAILED(GetPropFromOptions(cx, from, name, &propVal, &found))) > + return NS_ERROR_INVALID_ARG; > + > + if (!found) > + return NS_OK; Indentation @@ +3391,5 @@ > + jsval propVal; > + JSBool found; > + > + if (NS_FAILED(GetPropFromOptions(cx, from, name, &propVal, &found))) > + return NS_ERROR_INVALID_ARG; Indentation @@ +3436,5 @@ > + "sandboxPrototype", &options.proto); > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = GetBoolPropFromOptions(cx, optionsObject, > + "wantXrays", &options.wantXrays); Check the return value @@ +3510,5 @@ > // If there is no options object given, or no sandboxName property > // specified, use the caller's filename as sandboxName. > + if (options.sandboxName.IsEmpty() && > + NS_FAILED(GetSandboxNameFromStack(cx, options.sandboxName))) > + return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval); {} ::: js/xpconnect/src/nsXPConnect.cpp @@ +1914,5 @@ > > jsval rval = JSVAL_VOID; > AUTO_MARK_JSVAL(ccx, &rval); > > + xpc::SandboxOptions options; Stick a "using namespace xpc;" somewhere at the top if it isn't there yet, then you can just write "SandboxOptions options;"

Comment on attachment 628269 [details] [diff] [review] part 2: Adding ExpandedPrincipal support >+typedef nsresult (NS_STDCALL nsIPrincipal::*nsIPrincipalMemFn)(nsIPrincipal* aOther, bool* aResult); >+#define CALL_MEMBER_FUNCTION(THIS,MEM_FN) ((THIS)->*(MEM_FN)) >+ >+// nsExpandedPrincipal::Equals and nsExpandedPrincipal::EqualsIgnoringDomain shares the same logic. >+// The difference only that Equals requires 'this' and 'aOther' to Subsume each other while EqualsIgnoringDomain >+// requires bidirectional SubsumesIgnoringDomain. These lines are way too wide. >+nsresult >+Equals(nsExpandedPrincipal* aThis, nsIPrincipalMemFn aFn, nsIPrincipal* aOther, bool* aResult) This should be static, no? >+// nsExpandedPrincipal::Subsumes and nsExpandedPrincipal::SubsumesIgnoringDomain shares the same logic. >+// The difference only that Subsumes calls are replaced with SubsumesIgnoringDomain calls in the second case. >+nsresult >+Subsumes(nsExpandedPrincipal* aThis, nsIPrincipalMemFn aFn, nsIPrincipal* aOther, bool* aResult) Same here. I didn't look at the rest of the patch that closely, but it seems to address my review comments. r=bholley

(In reply to Bobby Holley (:bholley) from comment #79) > Comment on attachment 628269 [details] [diff] [review] > This should be static, no? you mean static member function instead of a plain c like funcion? sure... I can do that.

Er, no. I mean static linkage. It's just a regular plain c function, right? If so, it's best to declare it as static to indicate that it's never used outside the file.

Fixed nits.

added 'static' and broke some long lines.

Fixed merge issues after the fix of the nits.

check-in for patch 0-5 I mean, the splitting nsIPrincipal patch will not land.

I took some time to play with this and I found something being broken. When I used an array for first Sandbox argument, it fails adding an iframe to the document. Here is a scratchpad testcase. Load any webpage and run it. You will see that the iframe document doesn't load and the load event never fires. But if you replace `Cu.Sandbox([content])` with `Cu.Sandbox(content)`, everything works fine. That's why all jetpack tests are green. let Cu = Components.utils; let s = Cu.Sandbox([content]); s.document = content.document; s.log = function (s) { Cu.reportError(s); } function Content() { log("start"); let iframe = document.createElement("iframe"); iframe.addEventListener("load", function () { log("load"); }, false); iframe.setAttribute("src", "data:text/html,foo"); document.documentElement.appendChild(iframe); log("end") } Cu.evalInSandbox("new "+Content, s); There should not be any difference between Cu.Sandbox("http://moz.org") and Cu.Sandbox(["http://moz.org"]), right ? If yes, you can modify jetpack codebase to use the second form with array and see if jetpack tests are still green (they are broken with this build and following modification) https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/worker.js#L113 Here you should replace: let apiSanbox = sandbox(window, { wantXrays: true }); with: let apiSanbox = sandbox([window], { wantXrays: true }); https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/worker.js#L126 And here this: let content = this._sandbox = sandbox(window, { with: let content = this._sandbox = sandbox([window], {

(In reply to Alexandre Poirot (:ochameau) from comment #86) > There should not be any difference between Cu.Sandbox("http://moz.org") and > Cu.Sandbox(["http://moz.org"]), right ? There is some. ["http://moz.org"] will subsume "http://moz.org", but not the other way around. But the problem is not really this, the problem is that in a lot of places in the code we use principal::Equals check instead of principal::Subsumes. In this case nsDocShell::CheckLoadingPermissions(). I will require reviews for all these changes and there are quite a few... So the best we can do as a start is that we check the issues we run into with jetpack fix them one by one. Hopefully there aren't many...

https://hg.mozilla.org/integration/mozilla-inbound/rev/708c967d2fd6 https://hg.mozilla.org/integration/mozilla-inbound/rev/e9814ed8d974 https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd8897c873b https://hg.mozilla.org/integration/mozilla-inbound/rev/0e53ca52d4f4 https://hg.mozilla.org/integration/mozilla-inbound/rev/c55c34f2d828 https://hg.mozilla.org/integration/mozilla-inbound/rev/a45c751b89ea

https://hg.mozilla.org/mozilla-central/rev/708c967d2fd6 https://hg.mozilla.org/mozilla-central/rev/e9814ed8d974 https://hg.mozilla.org/mozilla-central/rev/6dd8897c873b https://hg.mozilla.org/mozilla-central/rev/0e53ca52d4f4 https://hg.mozilla.org/mozilla-central/rev/c55c34f2d828 https://hg.mozilla.org/mozilla-central/rev/a45c751b89ea

Comment on attachment 628855 [details] [diff] [review] part 5: Adding optional XHR constructor to sandbox Review of attachment 628855 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +2957,5 @@ > + return false; > + > + rv = nsContentUtils::WrapNative(cx, global, inst, vp); > + if (NS_FAILED(rv)) > + return false; These error cases should probably throw catchable exceptions

And pushed a followup to fix build warnings: https://hg.mozilla.org/integration/mozilla-inbound/rev/c03a04717753

(In reply to Alexandre Poirot (:ochameau) from comment #86) > I took some time to play with this and I found something being broken. Since this bug is already closed and the patches landed, I will just create a new bug for this one. Bug 763496.

I think that this is covered by https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox#The_Expanded_Principal, so marking this as ddc. But please let me know if there's more needed.