The default bug view has changed. See this FAQ.

webRequest doesn't check host permissions

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
WebExtensions: Request Handling
P1
normal
RESOLVED FIXED
5 months ago
10 days ago

People

(Reporter: billm, Assigned: zombie)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-needed})

unspecified
mozilla54
addon-compat, dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [webRequest]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
It looks like it's supposed to.

Comment 1

5 months ago
We should, incidentally, check permissions for both the URL being requested and the principal that's requesting it.

Comment 2

5 months ago
(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)
(Reporter)

Updated

5 months ago
Summary: webRequest doesn't request host permissions → webRequest doesn't check host permissions

Comment 3

5 months ago
(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.

Updated

5 months ago
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Whiteboard: triaged

Updated

2 months ago
webextensions: --- → +
(Assignee)

Comment 4

2 months ago
(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)

Updated

2 months ago
Assignee: kmaglione+bmo → tomica
Status: NEW → ASSIGNED

Comment 5

2 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 7

2 months ago
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 8

2 months ago
mozreview-review
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.

Comment 11

2 months ago
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 hidden (mozreview-request)

Comment 13

2 months ago
mozreview-review
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.
(Assignee)

Comment 14

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 15

2 months ago
mozreview-review-reply
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)

Comment 17

2 months ago
(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)

Updated

2 months ago
Whiteboard: triaged → [webRequest]triaged

Comment 18

2 months ago
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: 1308292
(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 21

2 months ago
mozreview-review
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.
(Assignee)

Comment 25

2 months ago
(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.
(Assignee)

Comment 27

2 months ago
(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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8831787 - Flags: review?(kmaglione+bmo)

Comment 30

2 months ago
mozreview-review
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+

Updated

2 months ago
Keywords: addon-compat
Comment hidden (mozreview-request)
(Assignee)

Comment 32

a month ago
mozreview-review-reply
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)?
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 33

a month ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51541cde2e06
Check host permissions for webRequest listeners r=kmag
Keywords: checkin-needed

Comment 34

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/51541cde2e06
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.