1254310 - Create a whitelist pref to temporarily disable Safe Browsing on given domains

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[François Marier [:francois]]

In order to be able to buy us more time to fix future Safe Browsing bugs, we should have a pref (empty by default) which would let us specify domains to temporarily exempt from all Safe Browsing checks.

Bug 1164518 is an example of where we could have used such a mechanism.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Such problems are rather transient so the delay before they they reach Bugzilla and we can diagnose them as SafeBrowsing issues plus the time to prepare a hotfix is often going to be longer than the time the site is actually affected.

Bug 820283 is another example FWIW.

(I'm not saying this might not come in handy, but I am saying that this is going to have limited effectiveness and we should fix the known bugs first and foremost)

Comment 2

[François Marier [:francois]]

Attachment: bug1254310.patch

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8728699 [details] [diff] [review]
bug1254310.patch

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

::: browser/components/safebrowsing/content/test/browser_whitelisted.js
@@ +2,2 @@
>  
> +const PREF_WHITELISTED_HOSTNAMES = "urlclassifier.whitelistedHostnames";

Random thought: maybe "skipping" rather than "whitelisting" is more appropriate, depending on what UrlClassifier actually gets used for.

I know I'm bikeshedding now.

@@ +29,5 @@
>    tabbrowser = gBrowser;
>    let tab = tabbrowser.selectedTab = tabbrowser.addTab();
>  
> +  info("Load a test page that's whitelisted");
> +  Services.prefs.setCharPref(PREF_WHITELISTED_HOSTNAMES, "example.com,www.ItIsaTrap.org,example.net");

Maybe whitelist ItIsATrap.org (without subdomain) for better test coverage?

::: netwerk/base/nsChannelClassifier.cpp
@@ +324,5 @@
> +      rv = uri->GetHost(host);
> +      if (NS_SUCCEEDED(rv) && !host.IsEmpty()) {
> +        ToLowerCase(host);
> +        ToLowerCase(whitelisted);
> +        nsCCharSeparatedTokenizer tokenizer(whitelisted, ',');

Maybe factor this out into a separate function? Do we not have any existing code to match hosts against a pref? This doesn't look like the kind of thing where we want to roll our own.

@@ +330,5 @@
> +          const nsCSubstring& token = tokenizer.nextToken();
> +          if (token.Equals(host)) {
> +            LOG(("nsChannelClassifier[%p]:StartInternal %s is whitelisted",
> +                 this, host.get()));
> +            return NS_ERROR_UNEXPECTED;

This error code sucks. (I know it's used throughout the existing function)

Comment 4

[Gian-Carlo Pascutto [:gcp]]

>Maybe whitelist ItIsATrap.org (without subdomain) for better test coverage?

So I realize now that the way the code is written it wants an exact match. I guess due to the way SafeBrowsing canonicalization works that is OK? We would list the exact host that is used in the fragment that is generating a false positive.

Comment 5

[François Marier [:francois]]

Attachment: bug1254310.patch

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> > +const PREF_WHITELISTED_HOSTNAMES = "urlclassifier.whitelistedHostnames";
> 
> Random thought: maybe "skipping" rather than "whitelisting" is more
> appropriate, depending on what UrlClassifier actually gets used for.

It's kind of a whitelist even though it's only meant to be used temporarily. I renamed the pref to "urlclassifier.skipHostnames" to make it clearer though.

> > +  info("Load a test page that's whitelisted");
> > +  Services.prefs.setCharPref(PREF_WHITELISTED_HOSTNAMES, "example.com,www.ItIsaTrap.org,example.net");
> 
> Maybe whitelist ItIsATrap.org (without subdomain) for better test coverage?

As you pointed out in comment 4, it's an exact match. I think it's fine because it's meant as a stop gap until we fix the underlying problem.

> Maybe factor this out into a separate function? Do we not have any existing
> code to match hosts against a pref? This doesn't look like the kind of thing
> where we want to roll our own.

I moved it to a separate function, but I couldn't find existing code that does the same thing. The closest thing I could find was this pref which has a comma-separated list of hostname regexps:

  media.getusermedia.screensharing.allowed_domains

but it already does something worst then my patch (which, at least, uses an existing tokenizer):

  https://dxr.mozilla.org/mozilla-central/rev/dd1abe874252e507b825a0a4e1063b0e13578288/dom/media/MediaManager.cpp#216-242

> > +            return NS_ERROR_UNEXPECTED;
> 
> This error code sucks. (I know it's used throughout the existing function)

Yes, it does. I didn't feel like changing that function any further (or adding a new error code) because I wanted to keep the change as small as possible so that we can hopefully get it uplifted.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8729247 [details] [diff] [review]
bug1254310.patch

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

>It's kind of a whitelist even though it's only meant to be used temporarily. I renamed the pref to 
>"urlclassifier.skipHostnames" to make it clearer though.

My reasoning was that we have whitelists in AppReputation, where we classify an entry but skip most of that work if they are whitelisted. Here we don't bother with classifying at all. Though one could as well argue that's really the same thing :-P

Anyway, now you have the pref named "skip" and everything else in the code calling it "whitelist", so I'm not sure that's an improvement. I'm fine either way, do whatever you think makes most sense.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abcea78b37db

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/abcea78b37db

Comment 9

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed fixed in Nightly 48, 2016-03-16. 

I used http://testsafebrowsing.appspot.com/ as my testing domain. I blocked and unblocked this domain via that pref to make sure Safe Browsing was enabled/disabled. 

Also, upon unblocking, I noticed that Safe Browsing wasn't immediately re-enabled for this domain. So, I allowed the browser to sit for 12 hours to make sure it got all possible Safe Browsing updates before verifying again. All worked thereafter.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Francois, is this something that we should consider uplifting to Aurora 47 and Beta 46? Would you be able to comment on the risk vs. benefit of uplifting? To me if this eliminates the need to do a dot release (even if it is a small likelihood), it makes sense to uplift to Aurora and Beta.

Comment 11

[François Marier [:francois]]

(In reply to Ritu Kothari (:ritu) from comment #10)
> Francois, is this something that we should consider uplifting to Aurora 47
> and Beta 46? Would you be able to comment on the risk vs. benefit of
> uplifting? To me if this eliminates the need to do a dot release (even if it
> is a small likelihood), it makes sense to uplift to Aurora and Beta.

I think it makes sense to uplift to Aurora because there's a small chance it could be useful in 47.

As for Beta, I think the patch itself is not very risky, but it's in the critical path of every single network load. So it might be good to give it some extra testing time on aurora before throwing it in front of the Beta population.

Comment 12

[François Marier [:francois]]

Comment on attachment 8729247 [details] [diff] [review]
bug1254310.patch

Approval Request Comment
[Feature/regressing bug #]: Ability to temporarily disable Safe Browsing for a given hostname.
[User impact if declined]: None, except there's a small chance it could help avoid a point release.
[Describe test coverage new/current, TreeHerder]: Adds a new test for it.
[Risks and why]: In the critical path of every network load.
[String/UUID change made/needed]: None

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8729247 [details] [diff] [review]
bug1254310.patch

While this is risky, I am taking this uplift in Aurora 47 with the hopes that this will not make things worse and help avoid some dot-release situations.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f8275d6c822