Closed Bug 1180921 Opened 9 years ago Closed 9 years ago

Implement pattern-based whitelisting mechanism for cross-origin XHR

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: billm, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 2 obsolete files)

Part 1 - Give Optional<T> Maybe<T>-like operator== semantics. v1
775 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 2 - Generalize test_origin to make it easier to add new origin attributes. v1
3.97 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
Part 3 - Add the addonId OriginAttribute. v1
5.93 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
Part 4 - Create a dumping ground for simple services in toolkit/components/utils. v1
5.07 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
Part 5 - Support custom callbacks for allowing access per-addon load access to cross-origin URIs. v1
13.74 KB, patch
bzbarsky
: review+
billm
: review+
Details | Diff | Splinter Review
Basically, we want to implement functionality from here: https://developer.chrome.com/extensions/xhr The approach we discussed is to add an OriginAttributes flag that specifies whether a principal comes from an add-on. In the XHR code, we'll check if the add-on ID is set for the nsIPrincipal tied to the XHR object. If it is, we'll ask some service (possibly the add-on manager) whether the given add-on ID is allowed to access the URI being XHRed. We also talked about extending nsExpandedPrincipal to support pattern matching. However, it seems like a mistake to encode the specifics of pattern matching directly into the security manager: https://developer.chrome.com/extensions/match_patterns In addition, it would be a bit strange if an nsExpandedPrincipal could do XHRs to domains that match the pattern, but it couldn't access windows from domains that match the pattern.
Attached patch WIP (obsolete) — — Splinter Review
I did a small part of the work here. Note that, until bug 1161831 is fixed, we'll need some hacky mechanism of recognizing which principals should be associated with an add-on. This code is based on the patch in bug 1175770.
For what it's worth the idea for this back then was to implement a wildcard principal and combine that with an nsEp. And then we could use that principal to create the XHR.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2) > For what it's worth the idea for this back then was to implement a wildcard > principal and combine that with an nsEp. And then we could use that > principal to create the XHR. Right. But as bill mentioned in comment 0, the problem is that this would require us to encode Chrome's precise wildcard semantics directly into our security manager, which isn't something I think we should commit to supporting.
(In reply to Bobby Holley (:bholley) from comment #3) > Right. But as bill mentioned in comment 0, the problem is that this would > require us to encode Chrome's precise wildcard semantics directly into our > security manager, which isn't something I think we should commit to > supporting. I get it just there are a couple of things I'm trying to understand here. - Is it really a good idea to implement the same pattern matching? As I read it on forums back then people at google regretted it too. And last time we talked about this you only wanted to support this up to effective-TLD. What have changed? Also, what that doc does not say is that it comes with limitations too: https://developer.chrome.com/extensions/faq#faq-dev-16 - How is the OriginAttributes flag a better approach? I guess I'm just missing something, but it sounds to me that with this flagged principal we will be able to do XHR but won't be able to access windows from that scope, what's the point of that? I mean, sure I don't want to do the parsing in our security layer, the parsing would just compose the right principal in JS. Is that a silly idea? > In addition, it would be a bit strange if an nsExpandedPrincipal could do > XHRs to domains that match the pattern, but it couldn't access windows from > domains that match the pattern. Why would this be the case? nsExpandedPrincipal could have access to windows it subsumes, if it has a wild card principal as an entry that subsumes the windows principal it can have access to it, and can XHR to that domain too. Seems to be cleaner to me than flagging a principal and special case the XHR, but then again I might be missing something. Or is that a requirement? That these scopes can only XHR to these domains and cannot access their windows? That would be... odd.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4) > I get it just there are a couple of things I'm trying to understand here. > - Is it really a good idea to implement the same pattern matching? As I read > it on forums back then people at google regretted it too. And last time we > talked about this you only wanted to support this up to effective-TLD. What > have changed? Also, what that doc does not say is that it comes with > limitations too: https://developer.chrome.com/extensions/faq#faq-dev-16 The match_patterns document I mentioned states pretty clearly that the host pattern can only contain a '*' in the first character. Even if the Chrome developers don't like this language anymore, every extension uses it. It would be pretty annoying if we didn't implement and instead asked them to use a slightly different language. We'd have to have a really good reason, which I don't see here. > - How is the OriginAttributes flag a better approach? I guess I'm just > missing something, but it sounds to me that with this flagged principal we > will be able to do XHR but won't be able to access windows from that scope, > what's the point of that? I don't see why access to windows is necessary. The two seem orthogonal. The use case here is that an extension wants to do an XHR to some web service and process the data that comes back. It doesn't need to access any windows from that web service because there aren't any. The data returned from the XHR would live in the compartment of the extension that initiated the request. > > In addition, it would be a bit strange if an nsExpandedPrincipal could do > > XHRs to domains that match the pattern, but it couldn't access windows from > > domains that match the pattern. > > Why would this be the case? nsExpandedPrincipal could have access to windows > it subsumes, if it has a wild card principal as an entry that subsumes the > windows principal it can have access to it, and can XHR to that domain too. > Seems to be cleaner to me than flagging a principal and special case the > XHR, but then again I might be missing something. Or is that a requirement? > That these scopes can only XHR to these domains and cannot access their > windows? That would be... odd. Again, I don't see why the two are related. CORS headers and document.domain are two examples of ways of changing the same-origin restriction in a way that only affects XHRs or windows but not both. This would just be another example of that. In addition, we don't want to grant extension content scripts access to any windows besides the ones they were instantiated in. If we ever implement a way of running different iframes in different processes, it would break.
(In reply to Bill McCloskey (:billm) from comment #5) > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4) > The match_patterns document I mentioned states pretty clearly that the host > pattern can only contain a '*' in the first character. Indeed. > > Even if the Chrome developers don't like this language anymore, every > extension uses it. It would be pretty annoying if we didn't implement and > instead asked them to use a slightly different language. We'd have to have a > really good reason, which I don't see here. I'm not talking about a different API I'm just thinking if we need it all like: http://*/* since we had a talk about this case with Bobby a few years ago and he was against it back then. Then again, only on principal level not XHR level so nevermind. And of course about how/where to implement it. > I don't see why access to windows is necessary. The two seem orthogonal. Alright, I think I get your point with document domain and CORS headers. I think you're right thanks for the explanation. The SDK had somewhat more complex use cases, I think I still were in that mindset. You are right and my version would just add unnecessary complication here. Thanks for helping me wrapping my head around this :) > > In addition, we don't want to grant extension content scripts access to any > windows besides the ones they were instantiated in. If we ever implement a > way of running different iframes in different processes, it would break. Good point as well.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6) > since we had a talk about this case with Bobby a few years ago > and he > was against it back then. Then again, only on principal level not XHR level > so > nevermind. And of course about how/where to implement it. Exactly - I'm against supporting it in the general case of principals (with subsumes etc), but I'm happy to support this simplified use-case here.
This is a WIP on top of some other patches, posting to facilitate discussion with billm.
Attachment #8631282 - Flags: review?(gkrizsanits)
bz on the caps pieces, billm on everything else.
Attachment #8631285 - Flags: review?(wmccloskey)
Attachment #8631285 - Flags: review?(bzbarsky)
Attachment #8630169 - Attachment is obsolete: true
Attachment #8631152 - Attachment is obsolete: true
Comment on attachment 8631280 [details] [diff] [review] Part 1 - Give Optional<T> Maybe<T>-like operator== semantics. v1 r=me
Attachment #8631280 - Flags: review?(bzbarsky) → review+
Comment on attachment 8631285 [details] [diff] [review] Part 5 - Support custom callbacks for allowing access per-addon load access to cross-origin URIs. v1 >+ boolean mayAddonLoadURI(in AUTF8String aAddonId, in nsIURI aURI); addonMayLoadURI may make more sense. I'm not sure how we can make the addon ID be AUTF8String if the thing in the dictionary is a ByteString, unless we have no JS consumers of the dictionary or all the ids are ASCII or something. ByteString is basically ISO-8859-1. We should probably either use a DOMString or USVString in the dictionary and a DOMString here or ... something. >+++ b/caps/nsNullPrincipal.cpp Do we need to allow this? It worries me to have nullprincipal allowed to load something, because of the cases in which we use nullprincipal as the "safe thing to fall back to" thing. r=me modulo those issues.
Attachment #8631285 - Flags: review?(bzbarsky) → review+
Attachment #8631281 - Flags: review?(gkrizsanits) → review+
Attachment #8631282 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8631283 [details] [diff] [review] Part 4 - Create a dumping ground for simple services in toolkit/components/utils. v1 Review of attachment 8631283 [details] [diff] [review]: ----------------------------------------------------------------- Just so I'm following: if we wanted to add other teeny tiny components into simpleServices.js, we'd have to: 1) write the component constructor + prototype with classID, QI implementation + actual interface implementation 2) adjust: 39 this.NSGetFactory = XPCOMUtils.generateNSGetFactory([RemoteTagServiceService]); to 39 this.NSGetFactory = XPCOMUtils.generateNSGetFactory([RemoteTagServiceService, MyFancyNewMiniService]); 3) adjust the utils.manifest file to include component and contract directives with the same file but different CID right? As a separate point, I'm surprised this is all browser/ only (package manifest!) while the code lives in toolkit and the only consumer is in js/ land. Do mobile + thunderbird etc. not want this? Because I think they do (if I understand the "whole point" of doing all this correctly), which means I expect we want to add this to the relevant manifests (and ensure it gets built and so on), even if the JS code won't die if the service doesn't exist. With all that understood correctly, rs=me for moving these bits about. :-)
Attachment #8631283 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #17) > right? Correct, and that's exactly what part 5 does. :-) > As a separate point, I'm surprised this is all browser/ only (package > manifest!) while the code lives in toolkit and the only consumer is in js/ > land. Do mobile + thunderbird etc. not want this? I don't think they do - this is all specifically related to addons, which don't really apply there. Certainly not the CPOW pieces hoisted in this patch, and probably not the chrome-extension pieces in part 5 (at least, not yet). > Because I think they do > (if I understand the "whole point" of doing all this correctly) I don't think that's really the point here. The point of consolidating globals is to avoid needless boilerplate, and, more importantly, avoid the memory overhead. The point of doing this in toolkit is that stuff in toolkit/ is registered in the content process whereas stuff in browser/ isn't. This seems like a somewhat reasonable heuristic on face, though I'm not really the person to ask about it - anyway, out of scope here. > , which means > I expect we want to add this to the relevant manifests (and ensure it gets > built and so on), even if the JS code won't die if the service doesn't exist. Per above, I don't think we necessarily want these elsewhere, and even if doing so makes sense from a higher-level architectural perspective, I think changing that is mostly orthogonal to this bug.
(In reply to Bobby Holley (:bholley) from comment #18) > > As a separate point, I'm surprised this is all browser/ only (package > > manifest!) while the code lives in toolkit and the only consumer is in js/ > > land. Do mobile + thunderbird etc. not want this? > > I don't think they do - this is all specifically related to addons, which > don't really apply there. Certainly not the CPOW pieces hoisted in this > patch, and probably not the chrome-extension pieces in part 5 (at least, not > yet). This was really my point: I presumed we wanted this to be usable on fennec (and to a lesser degree sm/tb) as well. I take it we don't (yet).
(In reply to Boris Zbarsky [:bz] from comment #16) > Comment on attachment 8631285 [details] [diff] [review] > Part 5 - Support custom callbacks for allowing access per-addon load access > to cross-origin URIs. v1 > > >+ boolean mayAddonLoadURI(in AUTF8String aAddonId, in nsIURI aURI); > > addonMayLoadURI may make more sense. Ok. > I'm not sure how we can make the addon ID be AUTF8String if the thing in the > dictionary is a ByteString, unless we have no JS consumers of the dictionary > or all the ids are ASCII or something. ByteString is basically ISO-8859-1. > > We should probably either use a DOMString or USVString in the dictionary and > a DOMString here or ... something. I think the issue is that we generally store addon IDs as UTF8, and so storing them as UTF16 here may cause impedence mismatches down the road. Is that a valid concern? If so, how do I specify WebIDL that leaves me with a UTF8 nsCString? > >+++ b/caps/nsNullPrincipal.cpp > > Do we need to allow this? It worries me to have nullprincipal allowed to > load something, because of the cases in which we use nullprincipal as the > "safe thing to fall back to" thing. Well, the logic is that this flag is about allowing potentially-cross-origin loads based on an OriginAttribute, so it shouldn't really matter what the principal is (since the principal itself wouldn't be allowed to perform these loads in general, regardless of whether it's codebase or nonce). I could go either way though. What do you think, Boris? Bill, what do we want here wrt to the new addon infra?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(bzbarsky)
(Also, FWIW - this wouldn't affect nsNullPrincipals in general, certainly not the ones we mint in the fallback case. It only affects principals with an addonId in its OriginAttributes, which we'll presumably only set when we want this kind of thing).
> If so, how do I specify WebIDL that leaves me with a UTF8 nsCString? You don't. You specify DOMString and then CopyUTF16toUTF8 yourself when storing the data, or do a UTF16toUTF8 copy when handing it out to people or something. > What do you think, Boris? My gut feeling is to lock things down more, but your point about this not affecting our fallback nullprincipals makes sense. So I'm ok with this part of the patch as-is, esp. if we add some comments explaining why we think it's ok.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #22) > > If so, how do I specify WebIDL that leaves me with a UTF8 nsCString? > > You don't. You specify DOMString and then CopyUTF16toUTF8 yourself when > storing the data, or do a UTF16toUTF8 copy when handing it out to people or > something. Well, that doesn't really help us here, because OriginAttributes inherits OriginAttributesDictionary, and serves as our canonical storage for this stuff. So I guess I'll just use nsAString and hope we don't hit too many mismatches. > My gut feeling is to lock things down more, but your point about this not > affecting our fallback nullprincipals makes sense. So I'm ok with this part > of the patch as-is, esp. if we add some comments explaining why we think > it's ok. Ok. Let's see what bill thinks.
Comment on attachment 8631285 [details] [diff] [review] Part 5 - Support custom callbacks for allowing access per-addon load access to cross-origin URIs. v1 Review of attachment 8631285 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsIAddonPolicyService.idl @@ +8,5 @@ > +#include "nsIURI.idl" > + > +/** > + * This interface allows the security manager to query custom per-addon security > + policy. Missing * here. ::: caps/nsNullPrincipal.cpp @@ +129,5 @@ > } > > + // If this principal is associated with an addon, check whether that addon > + // has been given permission to load from this domain. > + if (AddonAllowsLoad(aURI)) { I think it would be fine to remove this. I don't really see any reason why we would attach an add-on ID to the null principal, and it seems safer to avoid this. ::: caps/nsPrincipal.cpp @@ +235,5 @@ > } > > + // If this principal is associated with an addon, check whether that addon > + // has been given permission to load from this domain. > + if (AddonAllowsLoad(aURI)) { This shows my ignorance of caps stuff, but what are some example circumstances where CheckMayLoad is called? We want it for XHR stuff. What else are we getting along with that? ::: caps/tests/mochitest/test_addonMayLoad.html @@ +38,5 @@ > + }); > + return p; > + } > + > + let exampleCom_addonA = new Cu.Sandbox(ssm.createCodebasePrincipal(Services.io.newURI('http://example.com', null, null), {addonId: 'addonA'}), One thing I think we're missing is a way to create a sandbox with expanded principals that uses OriginAttributes. We could just copy the addonId argument from the Sandbox constructor to the OriginAttributes of the expanded principals. Or we could make a constructor for expanded principals. ::: toolkit/components/utils/simpleServices.js @@ +60,5 @@ > + * the given URI. > + * > + * @see nsIAddonPolicyService.mayAddonLoadURI > + */ > + mayAddonLoadURI: function(aAddonId, aURI) { Please use the new ES6 syntax now: mayAddonLoadURI(addonID, uri) {...}
Attachment #8631285 - Flags: review?(wmccloskey) → review+
To answer Gijs's question, we do want this for Fennec, FxOS. I think it's fine not to add it to the manifest yet, but we'll probably want to do it soon.
Flags: needinfo?(wmccloskey)
Comment on attachment 8631285 [details] [diff] [review] Part 5 - Support custom callbacks for allowing access per-addon load access to cross-origin URIs. v1 Review of attachment 8631285 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/tests/mochitest/mochitest.ini @@ +1,3 @@ > [DEFAULT] > support-files = > + file_data.txt Oops, forgot. Shouldn't this go in chrome.ini?
(In reply to Bill McCloskey (:billm) from comment #24) > I think it would be fine to remove this. I don't really see any reason why > we would attach an add-on ID to the null principal, and it seems safer to > avoid this. Ok, sure. > This shows my ignorance of caps stuff, but what are some example > circumstances where CheckMayLoad is called? We want it for XHR stuff. What > else are we getting along with that? CheckMayLoad is also called by the SSM for regular loads (i.e. setting window.location on an iframe) in the case that the URI is marked as only being loadable by subsumers (this happens in CheckLoadURIWithPrincipal). Aside from that, I believe it's generally called in cases like XHR - when the consumer wants to load data from a server, and access it as if it were same-origin. I don't have a list in my head of all the consumers, but grepping indicates that there are many. > One thing I think we're missing is a way to create a sandbox with expanded > principals that uses OriginAttributes. We could just copy the addonId > argument from the Sandbox constructor to the OriginAttributes of the > expanded principals. Or we could make a constructor for expanded principals. Yes, we need this. Please file a followup bug and needinfo me and I'll do it. > Please use the new ES6 syntax now: > mayAddonLoadURI(addonID, uri) {...} Neat! (In reply to Bill McCloskey (:billm) from comment #26) > Oops, forgot. Shouldn't this go in chrome.ini? I don't think so - that makes it available over chrome:// rather than http://, last I checked.
Blocks: 1182347
(Note that I had to self-review part 3, because r=gabor,r=bholley didn't seem to please the DOM peer review checker - Ryan, any idea what's up with that?)
Flags: needinfo?(ryanvm)
According to the commit hook, gabor isn't a valid DOM peer for webidl review. https://hg.mozilla.org/hgcustom/version-control-tools/file/3cc77a4359bf/hghooks/mozhghooks/prevent_webidl_changes.py File a dev services bug if that's not the case.
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31) > According to the commit hook, gabor isn't a valid DOM peer for webidl review. I don't think I'm a DOM peer at all, in fact officially I only have xpconnect peer status. So that's not a dev services bug. Although it might make sense to let peers/module owners request review from people they think are capable of handling a review on their modules from anyone regardless their peer status. Not being a peer on a module does not mean that someone cannot review code in a subset of the module he knows well. Also this is the best strategy to build more peers. (anyway off topic so I'll take this subject to somewhere else)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #31) > According to the commit hook, gabor isn't a valid DOM peer for webidl review. > https://hg.mozilla.org/hgcustom/version-control-tools/file/3cc77a4359bf/ > hghooks/mozhghooks/prevent_webidl_changes.py He's not, no. But per comment 29, the string was "r=gabor,r=bholley", which should have worked. > File a dev services bug if that's not the case. Will do.
Flags: needinfo?(bobbyholley)
Filed bug 1182642.
(In reply to Bill McCloskey (:billm) from comment #25) > To answer Gijs's question, we do want this for Fennec, FxOS. I think it's > fine not to add it to the manifest yet, but we'll probably want to do it > soon. Seconding this: Fennec absolutely wants to play in this garden.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: