1308640 - Tracking protection overrides webextension permission

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[Shane Caraveo (:mixedpuppy)]

A webextension with permissions set for a domain [eg. to graph.facebook.com] in its manifest is unable to make XHR calls if tracking protection is enabled.

It may be arguable what the behavior here should be (does addon permissions override tp, or visa-verse), but the larger problem is a total lack of any error reporting for addon authors, and any visual indication why an addon may break for the user.

Without any indicators to help users and authors, I believe the addon manifest should override TP.

Comment 1

[Kris Maglione [:kmag]]

I'm not sure whether or not WebExtensions should be immune to tracking protection (though I'm leaning towards not). But we should definitely find some way to indicate when a request is blocked by it. I would at least expect it to be in some way obvious if network logging was turned on in the browser console.

Comment 2

[Andy McKay]

I'd lean towards the order of importance being the browser, add-ons and then the content. If a user has set tracking protection on, I think they would expect that to happen everywhere in their browser, not just in content.

I don't think anyone would complain about some notification of this somewhere, otherwise others will get caught with things silently failing too.

Comment 3

[Shane Caraveo (:mixedpuppy)]

There is no immunity from or bypassing TP in this situation.  The addon is making the explicit request for that permission in the manifest.  The user is agreeing to that [in a world where permission UX exists] when they install the addon.  Blocking the url after that breaks the basic contract the API makes to the author, and breaks user expectation.

Comment 4

[Kris Maglione [:kmag]]

Let's not get into what users are agreeing to when they accept extension permissions. Down that road madness lies.

Leaving that aside, giving an extension host permissions for a site essentially allows it to act as if it were that site. It can inject scripts into it, access its cookies, and access the contents of requests to and documents from that domain. The browser, or other extensions, though, can still block or alter those requests. Which is what tracking protection is doing.

So the questions I have are, a) why is tracking protection blocking those requests?, b) is the extension doing something that we would expect tracking protection to block?, and c) if not, is there some reason this isn't an issue with tracking protection in general, rather than extensions in particular?

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #4)
> Let's not get into what users are agreeing to when they accept extension
> permissions. Down that road madness lies.

We may disagree here, not that it's madness (it is), but that it's key to any potential exceptions we make.  However, that can wait for a rainy day as you hit the key issue below...

> So the questions I have are, a) why is tracking protection blocking those
> requests?, b) is the extension doing something that we would expect tracking
> protection to block?, and c) if not, is there some reason this isn't an
> issue with tracking protection in general, rather than extensions in
> particular?

A direct XHR request on a permissioned URL is what is being blocked. I would assume (but should verify) that the tests you pointed to me earlier would break with TP turned on.  TP is probably not accounting for webextensions (and the principal they run under) thus assuming that the XHR is 3rd party access and rightly blocking it.  This wouldn't affect a legacy addon doing the same thing as they use the system principal.

So we could fix TP for XHR, but that would still leave any other functionality we promise via the permission susceptible to blocking, potentially break things for the addon, and thus the user, in a plethora of ways.  Death by a thousand cuts.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Francois, could we borrow you're knowledge here?  Was there any decisions around this kind of behavior related to tp, or is this just an oversight?  Is there a way to flag this kind of xhr if we want it to bypass tp?

Comment 7

[François Marier [:francois]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> Francois, could we borrow you're knowledge here?  Was there any decisions
> around this kind of behavior related to tp, or is this just an oversight? 

I don't believe we considered Web Extensions when TP was implemented. The underlying goal of TP is to block all network requests from websites to blacklisted domains.

While it would be nice to prevent Web Extensions from tracking users or injecting ads, it seems to me like we should protect against unwanted software/behavior elsewhere in the add-on pipeline.

> Is there a way to flag this kind of xhr if we want it to bypass tp?

For internal loads, you can do like bug 1296802 to bypass TP and Safe Browsing. There is no flag for just TP.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Chatted with Francois more on IRC.  The right approach (rather than the approach in bug 1296802) would be to have TP check that the extension has permission to make the request to the domain.  This check would occur in ShouldEnableTrackingProtection().

So I think the question comes down to this.  Given an addon that has a domain permission so it can make use of 3rd party APIs (e.g. graph.facebook.com), do we bypass TP?  I think we should.

straw man: the domain must be explicitly listed in permissions, all_urls permission does not allow the extension to bypass TP.

Comment 9

[Andy McKay]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> straw man: the domain must be explicitly listed in permissions, all_urls
> permission does not allow the extension to bypass TP.

Sounds good to me.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: allow permissioned background xhr through tp

This adds a check for addon permission in TP.  My C++ is probably not clean, feedback there appreciated.  f?francois for the low level TP and use of UrlClassifierTestUtils in the test, f?kmag for pretty much everything.

Tests do fail without changes and pass with changes.

If the direction here if fine lets just move it to review.

Comment 11

[François Marier [:francois]]

Comment on attachment 8808435 [details] [diff] [review]
allow permissioned background xhr through tp

Review of attachment 8808435 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsChannelClassifier.cpp
@@ +114,5 @@
>        }
>        return NS_OK;
>      }
>  
> +    if (AddonMayLoad(aChannel, chanURI)) {

I would add a comment above this line along the lines of:

    // This is an add-on load and the add-on is allowed to load from that URI.

Or maybe it's enough to just have the LOG line mention both of these things.

@@ +115,5 @@
>        return NS_OK;
>      }
>  
> +    if (AddonMayLoad(aChannel, chanURI)) {
> +        return NS_OK;

We should add a LOG line here to help with future debugging.

@@ +206,5 @@
>  
> +bool
> +nsChannelClassifier::AddonMayLoad(nsIChannel *aChannel, nsIURI *aUri) {
> +    nsCOMPtr<nsILoadInfo> channelLoadInfo = aChannel->GetLoadInfo();
> +    if (!channelLoadInfo)

nit: I prefer to see braces around one-line if statements.

Comment 12

[Shane Caraveo (:mixedpuppy)]

@kmag ping

Comment 13

[Shane Caraveo (:mixedpuppy)]

This bug is slowly becoming a smelly rotting corpse.

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8808435 [details] [diff] [review]
allow permissioned background xhr through tp

Review of attachment 8808435 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsIAddonPolicyService.idl
@@ +46,5 @@
>    boolean addonHasPermission(in AString aAddonId, in AString aPerm);
>  
>    /**
>     * Returns true if unprivileged code associated with the given addon may load
> +   * data from |aURI|.  Addons with <all_urls> permission will pass here.

The wording of "will pass here" is unclear.

@@ +54,5 @@
>    /**
> +   * Returns true if unprivileged code associated with the given addon may load
> +   * data from |aURI|.  Addons with <all_urls> permission will not pass if exact is true.
> +   */
> +  boolean addonHasURIPermission(in AString aAddonId, in nsIURI aURI, in boolean exact);

I don't see why we need an extra function for this. Please just add an extra optional parameter to addonMayLoadURI. And I think `explicit` is more appropriate than `exact`. (But please either consistently use aArgs, or consistently don't use them)

::: netwerk/base/nsChannelClassifier.cpp
@@ +210,5 @@
> +    if (!channelLoadInfo)
> +        return false;
> +
> +    nsCOMPtr<nsIPrincipal> triggeringPrincipal;
> +    nsresult rv = channelLoadInfo->GetTriggeringPrincipal(getter_AddRefs(triggeringPrincipal));

I think we should be using the loading principal here.

@@ +221,5 @@
> +        return false;
> +
> +    bool equals = false;
> +    if (NS_FAILED(triggeringURI->SchemeIs("moz-extension", &equals)) || !equals)
> +        return false;

Please use principal->GetAddonId.

@@ +227,5 @@
> +    nsCOMPtr<nsIAddonPolicyService> aps = do_GetService("@mozilla.org/addons/policy-service;1");
> +    NS_ENSURE_TRUE(aps, false);
> +
> +    nsString addonId;
> +    rv = aps->ExtensionURIToAddonId(triggeringURI, addonId);

Please use BasePrincipal->AddonAllowsLoad.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8808435 [details] [diff] [review]
allow permissioned background xhr through tp

Review of attachment 8808435 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/addons/MatchPattern.jsm
@@ +116,5 @@
>    },
>  
> +  matchesIgnoringPath(uri, exact = false) {
> +    if (exact) {
> +      return this.matchers.filter(matcher => matcher.pat != "<all_urls>")

We should also exclude patterns with host globs here. If, if we allow them at all, we should require them to include at least the full base domain as determined by the generic TLD service.

Also, any extension with the <all_urls> permission will not show any of its explicit host permissions in its permissions dialog, so we may need to actually exclude add-ons with the <all_urls> permission from bypass entirely.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1308640 bypass TP when addon has explicit permission to url,

Review commit: https://reviewboard.mozilla.org/r/109862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109862/

Comment 17

[Shane Caraveo (:mixedpuppy)]

I've moved this to reviewboard.  Comments in comment 14 addressed in that patch, except the following.


> @@ +227,5 @@
> > +    nsCOMPtr<nsIAddonPolicyService> aps = do_GetService("@mozilla.org/addons/policy-service;1");
> > +    NS_ENSURE_TRUE(aps, false);
> > +
> > +    nsString addonId;
> > +    rv = aps->ExtensionURIToAddonId(triggeringURI, addonId);
> 
> Please use BasePrincipal->AddonAllowsLoad.


This comment confused me...are you suggesting that should allow the explicit flag and we dont use aps for this?

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #15)
> Comment on attachment 8808435 [details] [diff] [review]
> allow permissioned background xhr through tp

> Also, any extension with the <all_urls> permission will not show any of its
> explicit host permissions in its permissions dialog, so we may need to
> actually exclude add-ons with the <all_urls> permission from bypass entirely.

Other than the permission dialog, I cant come up with a good reason for this.  all_urls still specifies to the user that the addon gets to do lots of things to any page.  The explicit permission in addition to all_urls can be a review indication that something requiring explicit permission is being done.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8808435 [details] [diff] [review]
allow permissioned background xhr through tp

obsoleting in preference to reviewboard

Comment 20

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> This comment confused me...are you suggesting that should allow the explicit
> flag and we dont use aps for this?

No, I'm saying that the BasePrincipal AddonAllowsLoad method is the correct way to consult the add-on policy service for this.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109862/diff/1-2/

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

https://reviewboard.mozilla.org/r/109862/#review110912

r=me for the WebExtenion and policy service parts. The BasePrincipal and ChannelClassifier parts need reviews from CAPS and necko peers.

::: caps/BasePrincipal.h:265
(Diff revision 2)
>  
>    already_AddRefed<BasePrincipal> CloneStrippingUserContextIdAndFirstPartyDomain();
>  
> +  // Helper to check whether this principal is associated with an addon that
> +  // allows unprivileged code to load aURI.
> +  bool AddonAllowsLoad(nsIURI* aURI, bool aExplicit=false);

Nit: s/=/ = /

::: caps/BasePrincipal.cpp:690
(Diff revision 2)
>  
>    return BasePrincipal::CreateCodebasePrincipal(uri, attrs);
>  }
>  
>  bool
> -BasePrincipal::AddonAllowsLoad(nsIURI* aURI)
> +BasePrincipal::AddonAllowsLoad(nsIURI* aURI, bool aExplicit)

Nit: `aExplicit /* = false */`

::: caps/nsIAddonPolicyService.idl:50
(Diff revision 2)
> -   * data from |aURI|.
> +   * data from |aURI|.  Addons with <all_urls> permission is ignored if explicit
> +   * is true, requiring a specific url match in the extension permissions.

"If |aExplicit| is true, the <all_urls> permission and permissive host globs are ignored when checking for a match."

::: toolkit/components/extensions/ExtensionManagement.jsm:210
(Diff revision 2)
>    },
>  
>    // Checks whether a given extension can load this URI (typically via
>    // an XML HTTP request). The manifest.json |permissions| directive
>    // determines this.
> -  checkAddonMayLoad(extension, uri) {
> +  checkAddonMayLoad(extension, uri, exact = false) {

s/exact/explicit/

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_trackingprotection.html:74
(Diff revision 2)
> +      permissions: ["*://tracking.example.com/*"],
> +      content_scripts: [
> +        {
> +          "matches": ["http://mochi.test/*/file_sample.html"],
> +          "js": ["content_script.js"],
> +          "run_at": "document_idle",

"document_idle" is the default, so there's no reason to include it. But this may as well run at "document_start".

::: toolkit/modules/addons/MatchPattern.jsm:124
(Diff revision 2)
> +      return this.matchers.filter(matcher => matcher.pat != "<all_urls>" &&
> +                                             matcher.host &&
> +                                             !matcher.host.includes("*"))

Please cache this in a lazy `isExplicit` property.

::: toolkit/modules/addons/MatchPattern.jsm:126
(Diff revision 2)
>  
> -  matchesIgnoringPath(uri) {
> +  matchesIgnoringPath(uri, exact = false) {
> +    if (exact) {
> +      return this.matchers.filter(matcher => matcher.pat != "<all_urls>" &&
> +                                             matcher.host &&
> +                                             !matcher.host.includes("*"))

s/includes/startsWith/

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

Looking for additional review on BasePrincipal and ChannelClassifier changes.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109862/diff/2-3/

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

updated patch.  Looking for additional review on BasePrincipal and ChannelClassifier changes.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

https://reviewboard.mozilla.org/r/109862/#review111822

::: netwerk/base/nsChannelClassifier.cpp:242
(Diff revision 3)
>      // an event to the securityUI anyway.
>      return NotifyTrackingProtectionDisabled(aChannel);
>  }
>  
> +bool
> +nsChannelClassifier::AddonMayLoad(nsIChannel *aChannel, nsIURI *aUri) {

Nit: the opening { should be on its own line.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

https://reviewboard.mozilla.org/r/109862/#review112664

r=me with the nits below.

::: caps/BasePrincipal.h:269
(Diff revision 3)
>  
>    already_AddRefed<BasePrincipal> CloneStrippingUserContextIdAndFirstPartyDomain();
>  
> +  // Helper to check whether this principal is associated with an addon that
> +  // allows unprivileged code to load aURI.
> +  bool AddonAllowsLoad(nsIURI* aURI, bool aExplicit = false);

The second argument needs documentation.  It's entirely unclear how the behavior of this function should change based on that second arg.

Just referring to the nsIAddonPolicyService doc is probably fine.

::: netwerk/base/nsChannelClassifier.cpp:247
(Diff revision 3)
> +nsChannelClassifier::AddonMayLoad(nsIChannel *aChannel, nsIURI *aUri) {
> +    nsCOMPtr<nsILoadInfo> channelLoadInfo = aChannel->GetLoadInfo();
> +    if (!channelLoadInfo)
> +        return false;
> +
> +    nsCOMPtr<nsIPrincipal> loadingPrincipal;

It's worth documenting why this uses loadingPrincipal, not triggeringPrincipal.  In general, loadingPrincipal means "where is this loading?" and triggeringPrincipal means "who started the load?", and I would have expected the latter to be the relevant thing here....

::: netwerk/base/nsChannelClassifier.cpp:248
(Diff revision 3)
> +    nsCOMPtr<nsILoadInfo> channelLoadInfo = aChannel->GetLoadInfo();
> +    if (!channelLoadInfo)
> +        return false;
> +
> +    nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +    nsresult rv = channelLoadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));

How about:

    nsIPrincipal* loadingPrincipal = channelLoadInfo->LoadingPrincipal();
    
(or equivalent for triggering principal).  No need for the nsresult, in any case; a loadinfo can't fail to hand out a principal.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8833688 [details]
Bug 1308640 bypass TP when addon has explicit permission to url,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109862/diff/3-4/

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27d0ab29f05b
bypass TP when addon has explicit permission to url, r=bz,kmag,mrbkap

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27d0ab29f05b

Comment 31

[Shane Caraveo (:mixedpuppy)]

Just ran into an issue with TP that is at least partly solved by this bug (and adding the domain to permissions in the manifest).  Probably useful to document that somewhere on MDN.

If privacy.trackingprotection.enabled is true and it blocks an xhr or page that the extension is loading, the extension can add the explicit domain to the permissions in the manifest and TP will allow the request.

Comment 32

[Will Bamberg [:wbamberg]]

This is already documented (by freaktechnik): https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions#Host_permissions.

Is there anything else you need?

Comment 33

[Shane Caraveo (:mixedpuppy)]

nope.