1429178 - Policies: Network interceptor whitelist/blacklist

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

A common policy request is to either blacklist a list of sites that can't be visited, or whitelist the list of ones that can be.

There's already an API for that (the network interceptor), that is used by adblockers. CCK2 already supports this too.

We need to make sure that the whitelist doesn't break any browser functionality..

Comment 1

[Mike Kaply [:mkaply]]

FYI, It appears chrome allows you to put entire schemes on the blocklist:

https://bugs.chromium.org/p/chromium/issues/detail?id=471010

to prevent local file access.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1429178 - Policy: Implement website blocklist/allowlist.

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

https://reviewboard.mozilla.org/r/232410/#review237936

Looking reasonable, but I'm thinking using nsIContentPolicy may be better here.

::: browser/components/enterprisepolicies/Policies.jsm:476
(Diff revision 1)
> +  "WebsiteFilter": {
> +    onBeforeUIStartup(manager, param) {
> +      let blocklist = param.Block || [];
> +      let exceptionslist = param.Exceptions || [];
> +
> +      if (blocklist.length > 50) {

What's the magic number here?  Can this be moved to a const, or even better to a pref or ... another policy?

::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:33
(Diff revision 1)
> +
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +ChromeUtils.defineModuleGetter(this, "PlacesUtils",
> +  "resource://gre/modules/PlacesUtils.jsm");

not used

::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:41
(Diff revision 1)
> +    // tip: set maxLogLevel to "debug" and use log.debug() to create detailed
> +    // messages during development. See LOG_LEVELS in Console.jsm for details.

You say this, but the logging below has no details.  This log getter seems like a lot of effort that could be replaced with Cu.reportError.

::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:67
(Diff revision 1)
> +      log.error("Wrong pattern in WebsiteFilter.Exceptions. This policy won't be activated.");
> +      return;
> +    }
> +  }
> +
> +  Services.obs.addObserver(this, "http-on-modify-request", true);

This is better than using webrequest here.  Two issues, and I think the second would probably override the first.

1. See if this can be done earlier, during http-on-opening-request.  That should block any blacklist site before any other webextension would ever see a request for it.  Otherwise you might leak those out to some extensions (though they wouldn't be able to do anything really, just see the request start).

2. This is also ONLY going to catch requests that use httpChannel.  What about other protocols such as ftp?  I wonder if you wouldn't be better off using nsIContentPolicy so you can see the other protocol requests (though I'm not sure it gets them all either).

See toolkit/modules/addons/WebRequestContent.js as an example.

::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:84
(Diff revision 1)
> +    } catch (e) {
> +      return;
> +    }
> +
> +    // Only filter top-level accesses
> +    if (loadInfo.externalContentPolicyType != Ci.nsIContentPolicy.TYPE_DOCUMENT) {

What about XHR, etc.?  Why not support a type param for the filtering?

::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:88
(Diff revision 1)
> +    // Only filter top-level accesses
> +    if (loadInfo.externalContentPolicyType != Ci.nsIContentPolicy.TYPE_DOCUMENT) {
> +      return;
> +    }
> +
> +    if (this._blockPatterns.matches(channel.URI)) {

Are we intending all sites be blocked, including those sites that some firefox features may rely on?  If so we need to make sure that tests exists for that situation.

::: browser/components/enterprisepolicies/schemas/policies-schema.json:449
(Diff revision 1)
>          }
>        }
> +    },
> +
> +    "WebsiteFilter": {
> +      "description": "Blocks websites from being visited. The parameters take an array of Match Patterns, as documented in https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Match_patterns.",

If the lists will be limited, that should be documented here, and if possible, a maxLength should be set on the arrays below.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_websitefilter.js:17
(Diff revision 1)
> +    let blocked = await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() {
> +      return content.document.documentURI.startsWith("about:neterror");
> +    });

feels like an intermittent being born...but I don't have any bright idea.

Comment 4

[Mike Kaply [:mkaply]]

FYI, here's how Chrome's filter works:

https://www.chromium.org/administrators/url-blacklist-filter-format

It can even be used to block local file access.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> Comment on attachment 8963468 [details]
> Bug 1429178 - Policy: Implement website blocklist/allowlist.
> 
> https://reviewboard.mozilla.org/r/232410/#review237936
> 
> Looking reasonable, but I'm thinking using nsIContentPolicy may be better
> here.

I first planned on using it too. The reason I didn't is because with nsIContentPolicy, I couldn't choose the error code to  return  (as I want to return Cr.NS_ERROR_BLOCKED_BY_POLICY to display the right message).

I suppose I can take a look at adding another REJECT type to nsIContentPolicy and see if it works well. If it's not too hard it might be worth if that will give us coverage of other protocols. Otherwise I think starting with http/https is a good start.

> 
> ::: browser/components/enterprisepolicies/Policies.jsm:476
> (Diff revision 1)
> > +  "WebsiteFilter": {
> > +    onBeforeUIStartup(manager, param) {
> > +      let blocklist = param.Block || [];
> > +      let exceptionslist = param.Exceptions || [];
> > +
> > +      if (blocklist.length > 50) {
> 
> What's the magic number here?  Can this be moved to a const, or even better
> to a pref or ... another policy?

Just seemed a good idea to do. What do you all think, should I keep a hard limit or not? If yes I'll document it better.. Chrome has a limit for this policy too, but it's much higher: 1000.

> 
> ::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:67
> (Diff revision 1)
> > +      log.error("Wrong pattern in WebsiteFilter.Exceptions. This policy won't be activated.");
> > +      return;
> > +    }
> > +  }
> > +
> > +  Services.obs.addObserver(this, "http-on-modify-request", true);
> 
> This is better than using webrequest here.  Two issues, and I think the
> second would probably override the first.
> 
> 1. See if this can be done earlier, during http-on-opening-request.  That
> should block any blacklist site before any other webextension would ever see
> a request for it.  Otherwise you might leak those out to some extensions
> (though they wouldn't be able to do anything really, just see the request
> start).

I don't think there's any harm in other extensions seeing the request. Choosing to http-on-opening-request would be nice, but it looks like that's not fired for redirects.

> 
> 2. This is also ONLY going to catch requests that use httpChannel.  What
> about other protocols such as ftp?  I wonder if you wouldn't be better off
> using nsIContentPolicy so you can see the other protocol requests (though
> I'm not sure it gets them all either).

I'll take a look at using it.

> ::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:84
> (Diff revision 1)
> > +    } catch (e) {
> > +      return;
> > +    }
> > +
> > +    // Only filter top-level accesses
> > +    if (loadInfo.externalContentPolicyType != Ci.nsIContentPolicy.TYPE_DOCUMENT) {
> 
> What about XHR, etc.?  Why not support a type param for the filtering?

This is prone to get very complex and admins inadvertently breaking websites, etc. The goal of this policy is only to block the user from visiting certain pages. I'm starting with this simple approach and then can think about a new policy that gives more control in the future.
FWIW this is also the behavior from Chrome's policy.
If someone really wants to block all kinds of network access, they are probably better off doing this at a firewall level.

> 
> ::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:88
> (Diff revision 1)
> > +    // Only filter top-level accesses
> > +    if (loadInfo.externalContentPolicyType != Ci.nsIContentPolicy.TYPE_DOCUMENT) {
> > +      return;
> > +    }
> > +
> > +    if (this._blockPatterns.matches(channel.URI)) {
> 
> Are we intending all sites be blocked, including those sites that some
> firefox features may rely on?  If so we need to make sure that tests exists
> for that situation.

Another reason for only blocking TYPE_DOCUMENT is to prevent any problems with this. This doesn't have the risk of blocking any Firefox internal pings, etc.


> browser/components/enterprisepolicies/tests/browser/
> browser_policy_websitefilter.js:17
> (Diff revision 1)
> > +    let blocked = await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() {
> > +      return content.document.documentURI.startsWith("about:neterror");
> > +    });
> 
> feels like an intermittent being born...but I don't have any bright idea.

No, this is reliable. Other tests blocking about: pages are doing the same.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

(In reply to Mike Kaply [:mkaply] from comment #4)
> FYI, here's how Chrome's filter works:
> 
> https://www.chromium.org/administrators/url-blacklist-filter-format
> 
> It can even be used to block local file access.

The MatchPattern is slightly different but supports most use cases here, including blocking entire protocols. So it can be used to block files by using: "file://*"

(Only problem is that the current implementation only blocks http/https channels)

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to :Felipe Gomes (needinfo me!) from comment #5)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> > Comment on attachment 8963468 [details]
> > Bug 1429178 - Policy: Implement website blocklist/allowlist.
> > 
> > https://reviewboard.mozilla.org/r/232410/#review237936
> > 
> > Looking reasonable, but I'm thinking using nsIContentPolicy may be better
> > here.
> 
> I first planned on using it too. The reason I didn't is because with
> nsIContentPolicy, I couldn't choose the error code to  return  (as I want to
> return Cr.NS_ERROR_BLOCKED_BY_POLICY to display the right message).

Not sure if its possible, but I wonder if REJECT_OTHER with the extra param data can be used somehow.

> > 
> > ::: browser/components/enterprisepolicies/Policies.jsm:476
> > (Diff revision 1)
> > > +  "WebsiteFilter": {
> > > +    onBeforeUIStartup(manager, param) {
> > > +      let blocklist = param.Block || [];
> > > +      let exceptionslist = param.Exceptions || [];
> > > +
> > > +      if (blocklist.length > 50) {
> > 
> > What's the magic number here?  Can this be moved to a const, or even better
> > to a pref or ... another policy?
> 
> Just seemed a good idea to do. What do you all think, should I keep a hard
> limit or not? If yes I'll document it better.. Chrome has a limit for this
> policy too, but it's much higher: 1000.

50 seems small.  Configurable via pref seems reasonable to me, otherwise we could use 1000.


> > ::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:67
> > (Diff revision 1)
> > > +      log.error("Wrong pattern in WebsiteFilter.Exceptions. This policy won't be activated.");
> > > +      return;
> > > +    }
> > > +  }
> > > +
> > > +  Services.obs.addObserver(this, "http-on-modify-request", true);
> > 
> > This is better than using webrequest here.  Two issues, and I think the
> > second would probably override the first.
> > 
> > 1. See if this can be done earlier, during http-on-opening-request.  That
> > should block any blacklist site before any other webextension would ever see
> > a request for it.  Otherwise you might leak those out to some extensions
> > (though they wouldn't be able to do anything really, just see the request
> > start).
> 
> I don't think there's any harm in other extensions seeing the request.
> Choosing to http-on-opening-request would be nice, but it looks like that's
> not fired for redirects.

You should be able to catch redirects using nsIChannelEventSink.  I'd prefer to avoid any extra processing if it's just going to be canceled.

> > ::: browser/components/enterprisepolicies/helpers/WebsiteFilter.jsm:88
> > (Diff revision 1)
> > > +    // Only filter top-level accesses
> > > +    if (loadInfo.externalContentPolicyType != Ci.nsIContentPolicy.TYPE_DOCUMENT) {
> > > +      return;
> > > +    }
> > > +
> > > +    if (this._blockPatterns.matches(channel.URI)) {
> > 
> > Are we intending all sites be blocked, including those sites that some
> > firefox features may rely on?  If so we need to make sure that tests exists
> > for that situation.
> 
> Another reason for only blocking TYPE_DOCUMENT is to prevent any problems
> with this. This doesn't have the risk of blocking any Firefox internal
> pings, etc.

I'm talking about our internal list of sites that we don't allow extensions tampering with.  see extensions.webextensions.restrictedDomains.  I'm not saying we need to, just asking.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

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

Comment 9

[:Felipe Gomes (needinfo for replies!)]

So I did a couple of changes here:

- Moved the limit up to 1000, and documented it

- Made it so that one invalid pattern won't fail the entire list, but only that pattern won't be used.

- Changed the isDocument check from loadInfo.type to channel.isDocument. This means that these URLs when loaded directly iframes will also be blocked (this matches what the Chrome policy does).

The other changes will be left for follow-ups as discussed on IRC

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

:bz for the added error code in xpc.msg

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

https://reviewboard.mozilla.org/r/232410/#review238218

Can you post a bug for the followup to convert to contentpolicy?

Comment 12

[:Felipe Gomes (needinfo for replies!)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Comment on attachment 8963468 [details]
> Bug 1429178 - Policy: Implement website blocklist/allowlist.
> 
> https://reviewboard.mozilla.org/r/232410/#review238218
> 
> Can you post a bug for the followup to convert to contentpolicy?

Filed bug 1450309 for that

Comment 13

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3af7ec240d21db284fca6e89ac2c6f4b00cdd1e9 -d 18a14e2a17c6: rebasing 455202:3af7ec240d21 "Bug 1429178 - Policy: Implement website blocklist/allowlist. r=mixedpuppy" (tip)
merging browser/components/enterprisepolicies/Policies.jsm
merging browser/components/enterprisepolicies/schemas/policies-schema.json
merging browser/components/enterprisepolicies/tests/browser/browser.ini
warning: conflicts while merging browser/components/enterprisepolicies/Policies.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

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

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

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

Comment 16

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5eb4fae475b1564347bd7f4cf5f2715f5dc9ef98 -d 0f2d2c86412c: rebasing 455216:5eb4fae475b1 "Bug 1429178 - Policy: Implement website blocklist/allowlist. r=mixedpuppy" (tip)
merging browser/components/enterprisepolicies/Policies.jsm
merging browser/components/enterprisepolicies/schemas/policies-schema.json
merging browser/components/enterprisepolicies/tests/browser/browser.ini
warning: conflicts while merging browser/components/enterprisepolicies/Policies.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/789953ae0349
Policy: Implement website blocklist/allowlist. r=mixedpuppy

Comment 19

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Enterprise Policies

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/789953ae0349

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policies
[User impact if declined]: Policy to blacklist/whitelist certain websites from being visited
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: QA will handle it
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: No
[Why is the change risky/not risky?]: Code is only contained to when this specific policy is in use
[String changes made/needed]: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8963468 [details]
Bug 1429178 - Policy: Implement website blocklist/allowlist.

Policy Engine fix needed for ESR60. Approved for 60.0b9.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cf6c11e50958

Comment 24

[Abe - QA (:Abe_LV)]

We tested this with latest nightly using JSON policy format and it is verified as fixed.

It will be rested with adm format when available.

Test case and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760

Comment 25

[Abe - QA (:Abe_LV)]

We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760

Comment 26

[Brindusa Tot, Desktop QA]

Based on previous comments, this bug was verified on latest Beta and Nightly. Remove the qe-verify+ flag.

Comment 27

[slimmclovin112]

Sorry if not the right/place time for this comment but I've tried using the policy to block URLs, and when one is blocked, if I right-click then view-source, I'm able to see the source. Is it possible to really block the URL from being downloaded?

Comment 28

[:Felipe Gomes (needinfo for replies!)]

(In reply to slimmclovin112 from comment #27)
> Sorry if not the right/place time for this comment but I've tried using the
> policy to block URLs, and when one is blocked, if I right-click then
> view-source, I'm able to see the source. Is it possible to really block the
> URL from being downloaded?

Thanks for the report. I filed bug 1465847 for that.