Closed Bug 1311815 Opened 8 years ago Closed 8 years ago

webRequest doesn't check host permissions

Categories

(WebExtensions :: Request Handling, defect, P1)

Product:
WebExtensions
Component:
Request Handling
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: billm, Assigned: zombie)

References

Details

(Keywords: addon-compat, dev-doc-complete, Whiteboard: [webRequest]triaged)

Attachments

(1 file)

Bug 1311815 - Check host permissions for webRequest listeners
59 bytes, text/x-review-board-request
kmag
: review+
Details
It looks like it's supposed to.
We should, incidentally, check permissions for both the URL being requested and the principal that's requesting it.
(In reply to Kris Maglione [:kmag] from comment #1) > We should, incidentally, check permissions for both the URL being requested > and the principal that's requesting it. Based just on the docs, that doesn't appear to be how Chrome works. I think an unfortunate consequence of doing that would be that it just leads to more extensions asking for <all_urls> (and users getting used to granting that permission). We also have an internal compatibility issue here, webextensions written originally for Firefox (as opposed to ported from Chrome) work right now without getting host permissions, they will break when we add this... (That's not an argument against adding it, but a note for addon compatibility notes)
Summary: webRequest doesn't request host permissions → webRequest doesn't check host permissions
(In reply to Andrew Swan [:aswan] from comment #2) > Based just on the docs, that doesn't appear to be how Chrome works. No, it's not how Chrome works, but I know they're considering changing their behavior so that it is. > I think an unfortunate consequence of doing that would be that it just leads > to more extensions asking for <all_urls> (and users getting used to granting > that permission). I suspect it won't make much difference either way.
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Whiteboard: triaged
webextensions: --- → +
(In reply to Kris Maglione [:kmag] from comment #1) > and the principal that's requesting it. Would that be the loadingPrincipal or the triggeringPrincipal? From my reading of [1], almost anything can "trigger" the loading of almost anything else, without getting access to it in any way. This makes the loading principal seem like the right one to check for this permission. But by the same logic, I don't understand why we expose the triggering principal [2] as the `originUrl` property to webReqest listeners, since the document into which the resource is getting loaded seems like the more interesting peace of information to extensions. 1) http://searchfox.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl 2) http://searchfox.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#620
Flags: needinfo?(kmaglione+bmo)
Assignee: kmaglione+bmo → tomica
Status: NEW → ASSIGNED
(In reply to Tomislav Jovanovic :zombie from comment #4) > (In reply to Kris Maglione [:kmag] from comment #1) > > and the principal that's requesting it. > > Would that be the loadingPrincipal or the triggeringPrincipal? From my > reading of [1], almost anything can "trigger" the loading of almost anything > else, without getting access to it in any way. This makes the loading > principal seem like the right one to check for this permission. Probably the loadingPrincipal, but there might be a case for checking the triggeringPrincipal as well. > But by the same logic, I don't understand why we expose the triggering > principal [2] as the `originUrl` property to webReqest listeners, since the > document into which the resource is getting loaded seems like the more > interesting peace of information to extensions. I could make an argument either way, and maybe we should provide both. But I think you're probably right that loadingPrincipal makes more sense there.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners Hey Shane, mind taking a look at this, especially if I should be writing the tests some other (better) way?
Attachment #8831787 - Flags: feedback?(mixedpuppy)
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners https://reviewboard.mozilla.org/r/108326/#review109384 ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_background_events.html:63 (Diff revision 1) > yield registration.unregister(); > yield extension.unload(); > }); > > +// Bug 1271354 - Support moz-extension: urls in MatchPattern > +/* I'm not clear how bug 1271354 applies to this test. ::: toolkit/modules/addons/WebRequest.jsm:754 (Diff revision 1) > windowId: 0, > parentWindowId: 0, > }; > > if (loadInfo) { > - let originPrincipal = loadInfo.triggeringPrincipal; > + let originPrincipal = loadInfo.loadingPrincipal || loadInfo.triggeringPrincipal; originUrl is documented as "URL of the resource that triggered this request.", which directly matches triggeringPrincipal. loadingPrincipal is the document the load is applied to. I think you should just add data.loadingUrl = loadingPrincipal.spec, and use that in your check in ext-webRequest.js. It wont get passed through to the extension since we build a new data object for that purpose in ext-webRequest.js. If we want to change what originUrl means, I'd want to dig into the original purpose for adding it (as it seems to be Firefox only) and make sure we're not breaking something else.
(In reply to Kris Maglione [:kmag] from comment #1) > We should, incidentally, check permissions for both the URL being requested > and the principal that's requesting it. I'm really wondering what we are gaining with this restriction. As aswan mentioned, it's just an another reason to request all_urls. If I want to monitor requests to a specific set of sites (or even just one) that may come from many origins, I'm basically required to expand my permission request. This doesn't feel like it's improving security or privacy.
Flags: needinfo?(kmaglione+bmo)
I also wish OP had been more specific.
If I understand this correctly, landing this will break a lot of add-ons. So if we do land this (looks like its currently under discussion), let's figure out how to do this a decent amount of notice for webextension developers.
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners https://reviewboard.mozilla.org/r/108326/#review109960 I'm still not convinced that we should even be doing this, I was to see that brought to a resolution before this would land. ::: toolkit/components/extensions/ext-webRequest.js:56 (Diff revisions 1 - 2) > > let data2 = { > requestId: data.requestId, > url: data.url, > originUrl: data.originUrl, > + documentUrl: data.documentUrl, I dont think we want to pass this through to the extension.
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners https://reviewboard.mozilla.org/r/108326/#review109384 > I'm not clear how bug 1271354 applies to this test. The issue is that this patch is checking the triggering principal of the request, which was the background page at a `moz-extension:` URI. But after the irc discussion, I agree this could be allowed by default (and without waiting for bug 1271354). > originUrl is documented as "URL of the resource that triggered this request.", which directly matches triggeringPrincipal. loadingPrincipal is the document the load is applied to. I think you should just add data.loadingUrl = loadingPrincipal.spec, and use that in your check in ext-webRequest.js. It wont get passed through to the extension since we build a new data object for that purpose in ext-webRequest.js. > > If we want to change what originUrl means, I'd want to dig into the original purpose for adding it (as it seems to be Firefox only) and make sure we're not breaking something else. I did a bit of investigation, but missed the time and reason why `originUrl` was added, and per agreement with Kris, thought `loadingPrincipal` was a more useful peace of information (most of the time, they are equivalent, so I wasn't worried about breaking stuff). Turns out it was explicitly meant to be `triggeringPrincipal`, see bug 1242871 comment #18.
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners https://reviewboard.mozilla.org/r/108326/#review109960 > I dont think we want to pass this through to the extension. It looks like it was meant to be exposed in bug 1242871 comment #18, and I don't see reasons against.
(In reply to Tomislav Jovanovic :zombie from comment #15) > Comment on attachment 8831787 [details] > Bug 1311815 - Check host permissions for webRequest listeners > > https://reviewboard.mozilla.org/r/108326/#review109960 > > > I dont think we want to pass this through to the extension. > > It looks like it was meant to be exposed in bug 1242871 comment #18, and I > don't see reasons against. If someone has actual need for it, I'm fine with the addition. Otherwise we shouldn't add something to a public api that nobody needs. Giorgio, is having the loading principle url still something that would be useful?
Flags: needinfo?(g.maone)
(In reply to Shane Caraveo (:mixedpuppy) from comment #16) > (In reply to Tomislav Jovanovic :zombie from comment #15) > > Giorgio, is having the loading principle url still something that would be > useful? Yes, exactly for the reasons stated in bug 1242871 comment #18: originUrl and documentUrl are often the same, but the may differ (for example for subresources specified in an external stylesheet, whose load is triggered by the CSS document but whose content is displayed in the HTML document), and both the data points are useful for different security/privacy assessments. Notice also that Chrome does not expose originUrl either (that is a WebExtension addition of mine, and skipping documentUrl an oversight of mine), but this is just one of the reasons why NoScript has never been ported on Chrome and why security and privacy oriented extensions on Chrome are very limited, and sometime just provide a false sense of security, if compared with their Firefox counterparts.
Flags: needinfo?(g.maone)
Whiteboard: triaged → [webRequest]triaged
We're not prompting for webRequest or webRequestBlocking permissions based on the assumption that they will always be accompanied by host permissions. Which means ideally we would get this in before turning on permissions prompts, is this on track to land in 54?
Blocks: webext-permissions
(In reply to Andrew Swan [:aswan] from comment #18) > We're not prompting for webRequest or webRequestBlocking permissions based > on the assumption that they will always be accompanied by host permissions. They already are. Thus bug is about another level of permissions. > Which means ideally we would get this in before turning on permissions > prompts, is this on track to land in 54? I don't think this should block, permissions are already required.
(In reply to Shane Caraveo (:mixedpuppy) from comment #19) > (In reply to Andrew Swan [:aswan] from comment #18) > > We're not prompting for webRequest or webRequestBlocking permissions based > > on the assumption that they will always be accompanied by host permissions. > > They already are. Thus bug is about another level of permissions. or not. Sorry. I'm focused on the requesting part of this, which I feel shouldn't be done. The requested url should be done.
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners https://reviewboard.mozilla.org/r/108326/#review112240 ::: toolkit/components/extensions/ext-webRequest.js:36 (Diff revision 2) > + // and the origin that is loading the resource/initiated the request. > + const origin = data.documentUrl || data.originUrl; > + const own = origin && origin.startsWith(context.extension.getURL()); > + if (origin && !own && !hosts.matchesIgnoringPath(NetUtil.newURI(origin))) { > + return; > + } > + The part specifically (initiating check) I have reservations around. The rest of the patch is good, including the addition of documentUrl.
(In reply to Shane Caraveo (:mixedpuppy) from comment #9) > I'm really wondering what we are gaining with this restriction. As aswan > mentioned, it's just an another reason to request all_urls. If I want to > monitor requests to a specific set of sites (or even just one) that may come > from many origins, I'm basically required to expand my permission request. > This doesn't feel like it's improving security or privacy. It is another reason to request <all_urls>, yes, but in the cases where that matters, those add-ons probably should be requesting access to <all_urls>, since they're essentially getting it anyway. At least in the case of script resources, where scripts from one common domain (e.g., google.com or facebook.com) are injected into thousands of other origins. I'd be OK with easing the restriction for non-script resources.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #22) > (In reply to Shane Caraveo (:mixedpuppy) from comment #9) > > I'm really wondering what we are gaining with this restriction. As aswan > > mentioned, it's just an another reason to request all_urls. If I want to > > monitor requests to a specific set of sites (or even just one) that may come > > from many origins, I'm basically required to expand my permission request. > > This doesn't feel like it's improving security or privacy. > > It is another reason to request <all_urls>, yes, but in the cases where that > matters, those add-ons probably should be requesting access to <all_urls>, > since they're essentially getting it anyway. Then what is the point of any host permission at all? Having the webrequest permission is basically just equating to all_urls. What are the cases that matter? > At least in the case of script > resources, where scripts from one common domain (e.g., google.com or > facebook.com) are injected into thousands of other origins. I'd be OK with > easing the restriction for non-script resources. What is the threat model here, and how are we improving on that if authors just end up requesting all_urls?
(In reply to Shane Caraveo (:mixedpuppy) from comment #23) > (In reply to Kris Maglione [:kmag] from comment #22) > > (In reply to Shane Caraveo (:mixedpuppy) from comment #9) > What are the cases that matter? What I'm actually asking here is, what are the cases that an author would ever only specify specific domains for both loading and request. It seems like a very tiny amount that would ever make sense for.
(In reply to Kris Maglione [:kmag] from comment #22) > I'd be OK with easing the restriction for non-script resources. I don't think this is worth doing. There could be other ways to affect the loading document than just manipulating the loading of scripts. XHR requests, even images/videos with CORS. This seems like a messy thing to try to get right and fullproof. (In reply to Shane Caraveo (:mixedpuppy) from comment #24) > What I'm actually asking here is, what are the cases that an author would > ever only specify specific domains for both loading and request. It seems > like a very tiny amount that would ever make sense for. I don't think that is the issue here. The threat model is that an addon might only ask for permissions for something innocuous, like youtube.com, and then have access to bank.com that happens to load resources from youtube.com (it's a bit contrived, but you get the point).
(In reply to Tomislav Jovanovic :zombie from comment #25) > (In reply to Kris Maglione [:kmag] from comment #22) > (In reply to Shane Caraveo (:mixedpuppy) from comment #24) > > What I'm actually asking here is, what are the cases that an author would > > ever only specify specific domains for both loading and request. It seems > > like a very tiny amount that would ever make sense for. > > I don't think that is the issue here. The threat model is that an addon > might only ask for permissions for something innocuous, like youtube.com, > and then have access to bank.com that happens to load resources from > youtube.com (it's a bit contrived, but you get the point). So given that scenario, what is the actual threat model? And how is requiring a loading principal permission (*with* the assumption that this pushes authors to all_urls) actually help improve or prevent that threat? If I were an attacker, why would I ever request anything less than all_urls? I'm harping on this because this feels like a security feature that doesn't produce any real security. By giving authors yet another reason to request all_urls, it has potential to eliminate any possible benefit. If it produces real security, then I'd like to understand what that is and how that is possible.
(In reply to Shane Caraveo (:mixedpuppy) from comment #26) > So given that scenario, what is the actual threat model? bank.com includes a script from youtube.com, you redirect that to your code, which when executed with bank.com privileges steals user's bank login, account details... (I said a bit contrived, but not impossible) > If I were an attacker, why would I ever request anything less than all_urls? If you advertise a "Youtube downloader", and ask only for youtube.com permissions, I might not think twice about installing the extension. If you ask for <all_urls>, I might.
Attachment #8831787 - Flags: review?(kmaglione+bmo)
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners https://reviewboard.mozilla.org/r/108326/#review113168 ::: toolkit/components/extensions/ext-webRequest.js:58 (Diff revision 4) > > let data2 = { > requestId: data.requestId, > url: data.url, > originUrl: data.originUrl, > + documentUrl: data.documentUrl, Hm. Exposing this to the extension is really separate from this bug. But it's fine, I guess... Please add it to the schema, though.
Attachment #8831787 - Flags: review?(kmaglione+bmo) → review+
Keywords: addon-compat
Comment on attachment 8831787 [details] Bug 1311815 - Check host permissions for webRequest listeners https://reviewboard.mozilla.org/r/108326/#review113168 > Hm. Exposing this to the extension is really separate from this bug. But it's fine, I guess... > > Please add it to the schema, though. We had a few issues like this recently. Would it be worth checking the callback return values against the schema in DEBUG mode (introduced recently)?
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/51541cde2e06 Check host permissions for webRequest listeners r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/51541cde2e06
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Keywords: dev-doc-needed
As my previous comment was ignored, I encourage you again to look at https://developer.chrome.com/extensions/webRequest#examples near "the same goal in a more efficient way". First example can't be recreated in Firefox - and that's like 50% of sample use cases. Is there any chance that you will revert checking origin URL against host permissions? Chrome and Opera only considers target URL. BTW this isn't documented in https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest#Browser_compatibility.
> Is there any chance that you will revert checking origin URL against host > permissions? Not unless you give some valid arguments. > Chrome and Opera only considers target URL. Chrome has planned to do so for some time.
> Not unless you give some valid arguments. I consider the second example at at https://developer.chrome.com/extensions/webRequest#examples as being perfectly valid argument. Also, I don't see YouTube downloader example as valid argument _for_ validating origin: it's bank.com administrator's responsibility to make sure he doesn't include external scripts. It's dangerous to force people to ask for more permissions than necessary (it increases attack surface). By asking for <all_urls>, extension blocking access to evil.com gains permissions to become evil itself. YouYube downloader will have access to bank.com too (even worse than initially). > Chrome has planned to do so for some time. I doubt they will break backward compatibility in the way you did. --- Before 54 you didn't check host permissions at all, now you are more restrictive than anyone. Couldn't you add some new manifest entry or introduce additional pattern to configure origin permissions?
I just noticed that documentUrl is not documented on mdn
Flags: needinfo?(wbamberg)
Keywords: dev-doc-complete → dev-doc-needed
Like for bug 1386707?
Flags: needinfo?(mixedpuppy)
yes. since there's a bug for that, resetting this one.
Flags: needinfo?(wbamberg)
Flags: needinfo?(mixedpuppy)
Keywords: dev-doc-needed → dev-doc-complete
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: