1107372 - Set different update and gethash urls for same type tables in SafeBrowsing.jsm

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Hector Zhao [:hectorz]]

In SafeBrowsing.init, tracking protection table and other safe browsing tables are respectively configured with SafeBrowsing.trackingUpdateURL and SafeBrowsing.updateURL. This means every table in the same pref/type will have the same update and gethash urls. For addition of new tables from extension, we should be able to configure these urls per-table.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

This is how it looks now:

urlclassifier.malwareTable=goog-malware-shavar,test-malware-simple
urlclassifier.trackingTable=mozpub-track-digest256
urlclassifier.phishTable=googpub-phish-shavar,test-phish-simple
browser.safebrowsing.updateURL: https://safebrowsing.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2&key=%GOOGLE_API_KEY%
browser.safebrowsing.gethashURL: https://safebrowsing.google.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2
browser.trackingprotection.gethashURL: https://tracking.services.mozilla.com/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2
browser.trackingprotection.updateURL: https://tracking.services.mozilla.com/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2

I think we want something like:

browser.safebrowsing.malwareTable.0.name = goog-malware-shavar
browser.safebrowsing.malwareTable.0.gethashURL = https://safebrowsing.google.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2
browser.safebrowsing.malwareTable.0.updateURL = https://tracking.services.mozilla.com/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2
browser.safebrowsing.malwareTable.1.name = otherprovider-malware-shavar
browser.safebrowsing.malwareTable.1.gethashURL = https://otherprovider.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2
browser.safebrowsing.malwareTable.1.updateURL = https://otherprovider.com/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2

and so on

Comment 2

[Gian-Carlo Pascutto [:gcp]]

We may have had this capability at some point already, I'm leaving the link here in someone wants to peek at the old code: https://bugzilla.mozilla.org/show_bug.cgi?id=769960

Comment 3

[François Marier [:francois]]

Noting that when we get a match on one of the lists, we need to keep track of which list it matched on. The Google Safe Browsing policy forbids us from attributing the match to Google if the entry came from a different list.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

The only attribution we do right now is via browser.safebrowsing.malware.reportURL which for (malware only) goes to a site report page from Google.

I would be tempted to make malware match the phishing case and just redirect to our generic phising and malware support page, where we can link to the diagnostic pages of all our providers.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)
> The only attribution we do right now is via
> browser.safebrowsing.malware.reportURL which for (malware only) goes to a
> site report page from Google.

Phishing, too, no? browser.safebrowsing.reportURL goes to https://safebrowsing.google.com/safebrowsing/report?

> I would be tempted to make malware match the phishing case and just redirect
> to our generic phising and malware support page, where we can link to the
> diagnostic pages of all our providers.

How is the user supposed to know which provider blocked the URL?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>Phishing, too, no? 

Nope, it sends you to a generic page:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2840

>How is the user supposed to know which provider blocked the URL?

Just try the diagnostic pages of all of them. (I'm serious! If we have >>2 providers we can still do something else.)

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> >Phishing, too, no? 
> 
> Nope, it sends you to a generic page:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js#2840

Firefox -> Help -> Report Web Forgery goes to Google.

> >How is the user supposed to know which provider blocked the URL?
> 
> Just try the diagnostic pages of all of them. (I'm serious! If we have >>2
> providers we can still do something else.)

I think this will be pretty much unusable. For things like SSL error reporting we report back to us (with the eventual goal of reporting to legitimate CA authorities). But that's out of scope of this bug.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a7413b6f562

Comment 9

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03be6ea66d54

Comment 10

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e5e4c22eb11

Comment 11

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd6ffc38c4e6

Comment 12

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a2aa6641cc

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Attachment: Set different update and gethash urls for same type tables in SafeBrowsing.jsm

Bug 1107372 - Restructure SafeBrowsing prefs to allow multiple providers.

Comment 14

[Hector Zhao [:hectorz]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> Created attachment 8652341 [details] [diff] [review]
> Set different update and gethash urls for same type tables in
> SafeBrowsing.jsm
> 
> Bug 1107372 - Restructure SafeBrowsing prefs to allow multiple providers.

Hi Gian-Carlo,

Is it possible to also remove the urlclassifier.*Table prefs, and generate the equivalents from browser.safebrowsing.provider.*.lists? This will make adding a new safebrowsing provider as easy as just defining a new set of browser.safebrowsing.provider.*.* prefs. Overriding urlclassifier.*Table in an extension will be really hacky when the extension is compatible with multiple Fx versions, also "-malware-", "-phish-" and "-track-" etc. in list names can be used to group them when necessary.

Thanks.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

It's not possible unless we make the -malware- (etc) part of the list name significant, and that seems like a bad idea if we support multiple providers, as it's the provider that chooses the list name, not us.

A followup to make urlclassifier.*Table a pref with subprefs instead of a comma seperated list seems like a better idea.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Alternatively, following bug 1177085, maybe

browser.safebrowsing.provider.google.lists.goog-phish-shavar.type = phishing
browser.safebrowsing.provider.google.lists.goog-phish-shavar.name = <index to l10n string>

Comment 17

[Hector Zhao [:hectorz]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #15)
> It's not possible unless we make the -malware- (etc) part of the list name
> significant, and that seems like a bad idea if we support multiple
> providers, as it's the provider that chooses the list name, not us.

I was under the impression that it is already significant, see [1].

> 
> A followup to make urlclassifier.*Table a pref with subprefs instead of a
> comma seperated list seems like a better idea.

Yes, and |TablesToResponse| in [1] should be updated accordingly.

[1]: https://dxr.mozilla.org/mozilla-central/rev/c46370eea81a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#172

Comment 18

[Gian-Carlo Pascutto [:gcp]]

(In reply to Hector Zhao [:hectorz] from comment #17)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #15)
> > It's not possible unless we make the -malware- (etc) part of the list name
> > significant, and that seems like a bad idea if we support multiple
> > providers, as it's the provider that chooses the list name, not us.
> 
> I was under the impression that it is already significant, see [1].

It is. I'm just saying this is probably not a good idea in the long run. It's OK for now because it's only us and Google.

Comment 19

[Hector Zhao [:hectorz]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #18)
> (In reply to Hector Zhao [:hectorz] from comment #17)
> > (In reply to Gian-Carlo Pascutto [:gcp] from comment #15)
> > > It's not possible unless we make the -malware- (etc) part of the list name
> > > significant, and that seems like a bad idea if we support multiple
> > > providers, as it's the provider that chooses the list name, not us.
> > 
> > I was under the impression that it is already significant, see [1].
> 
> It is. I'm just saying this is probably not a good idea in the long run.
> It's OK for now because it's only us and Google.

Sorry I missed the "if we support multiple providers" part.

As a side note, I just thought I should check what's the current safe browsing setup in the Yandex repack. It turns out it is still using http endpoint which I think is not supported any more[1], and yandex server is serving tables with both "goog-" and "ydx-" prefixs[2].

[1]: http://hg.mozilla.org/build/partner-repacks/file/tip/partners/yandex-ru/distribution/distribution.ini
[2]: https://sba.yandex.net/list?client=navclient-auto-ffox&appver=42.0a2&pver=2.2

Comment 20

[Gian-Carlo Pascutto [:gcp]]

(In reply to Hector Zhao [:hectorz] from comment #19)

> As a side note, I just thought I should check what's the current safe
> browsing setup in the Yandex repack. It turns out it is still using http
> endpoint which I think is not supported any more[1], and yandex server is
> serving tables with both "goog-" and "ydx-" prefixs[2].

Ugh, HTTPS is indeed not supported. It might still "work" but there is no more MAC so it's totally insecure.

Comment 21

[Gian-Carlo Pascutto [:gcp]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #20)

> Ugh, HTTPS is indeed not supported. It might still "work" but there is no
> more MAC so it's totally insecure.

That should've said "HTTP is not supported".

Yanxed told me they have HTTPS so I guess the links just need updating.

Comment 22

[François Marier [:francois]]

Comment on attachment 8652341 [details] [diff] [review]
Set different update and gethash urls for same type tables in SafeBrowsing.jsm

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

::: b2g/app/b2g.js
@@ -352,5 @@
>  pref("dom.w3c_touch_events.safetyX", 0); // escape borders in units of 1/240"
>  pref("dom.w3c_touch_events.safetyY", 120); // escape borders in units of 1/240"
>  
>  #ifdef MOZ_SAFE_BROWSING
> -// Safe browsing does nothing unless this pref is set

Thanks for removing this lie!

@@ +356,3 @@
>  pref("browser.safebrowsing.enabled", false);
> +// Prevent loading of pages identified as malware
> +pref("browser.safebrowsing.malware.enabled", true);

That should remain "false" by default until there's UI to enable/disable it on B2G.

::: browser/app/profile/firefox.js
@@ +961,5 @@
>  pref("browser.safebrowsing.malware.enabled", true);
>  pref("browser.safebrowsing.downloads.enabled", true);
>  pref("browser.safebrowsing.downloads.remote.enabled", true);
>  pref("browser.safebrowsing.downloads.remote.timeout_ms", 10000);
> +pref("browser.safebrowsing.debug", true);

That should remain "false" by default because it's pretty spammy.

::: modules/libpref/init/all.js
@@ +4793,5 @@
>  pref("urlclassifier.trackingTable", "test-track-simple,mozpub-track-digest256");
>  pref("urlclassifier.trackingWhitelistTable", "test-trackwhite-simple,mozpub-trackwhite-digest256");
> +
> +pref("browser.safebrowsing.provider.mozilla.lists", "mozpub-track-digest256,mozpub-trackwhite-digest256");
> +pref("browser.safebrowsing.provider.mozilla.updateURL", "https://tracking.services.mozilla.com/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");

BTW, this will soon change to shavar.services.mozilla.com (see bug 1198030).

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +36,5 @@
>  const downloadAllowLists = getLists("urlclassifier.downloadAllowTable");
>  const trackingProtectionLists = getLists("urlclassifier.trackingTable");
>  const trackingProtectionWhitelists = getLists("urlclassifier.trackingWhitelistTable");
>  
> +var debug = true;

Should this be "false"?

@@ +201,5 @@
> +      updateURL = updateURL.replace("SAFEBROWSING_ID", clientID);
> +      gethashURL = gethashURL.replace("SAFEBROWSING_ID", clientID);
> +
> +      log("Provider: " + provider + " updateURL=" + updateURL);
> +      log("Provider: " + provider + " gethashURL=" + updateURL);

This should be "+ gethashURL".

Comment 23

[Gian-Carlo Pascutto [:gcp]]

Attachment: Update preferences for new SafeBrowsing prefs structure

Addressed review comments.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2734a3110b4a

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Failing on Talos now:
FATAL ERROR: Non-local network connections are disabled and a connection attempt to tracking.services.mozilla.com (52.26.18.160) was made. 

Will need a a backout.

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

See bug 1192416 for how the same issue has been solved in the past.

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #25)
> Failing on Talos now:
> FATAL ERROR: Non-local network connections are disabled and a connection
> attempt to tracking.services.mozilla.com (52.26.18.160) was made. 
> 
> Will need a a backout.

its backed out now from mozilla-inbound for causing this issues https://treeherder.mozilla.org/logviewer.html#?job_id=13497503&repo=mozilla-inbound

Comment 28

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6782a855c243

Comment 29

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch talos for new SB prefs.

Comment 30

[Gian-Carlo Pascutto [:gcp]]

Attachment: Update preferences for new SafeBrowsing prefs structure

Carrying forward r+ but fix a truncated line error.

Comment 31

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03bfe3ea56c2

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8655465 [details] [diff] [review]
Patch talos for new SB prefs.

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

we can land this on talos and then bump testing/talos/talos.json.  We are close to landing talos in tree, but for now that is the process.

Comment 33

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/build/talos/rev/3b94adbd66f146cbd821d0ba465c1f2b090893ec
Bug 1107372 - Rework SafeBrowsing prefs for multiple providers. r=jmaher

Comment 34

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b317ee483a6415c07aaa8337c2cbd4d588524576
Bug 1107372 - Update preferences for new SafeBrowsing prefs structure. r=francois

Comment 35

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13649742&repo=mozilla-inbound

Comment 36

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/174ee19829ec

Comment 37

[Gian-Carlo Pascutto [:gcp]]

Ugh, a second line was truncated in the Androids prefs...

Comment 38

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0fe4cfdf0b

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Blew up B2G reftests too.

Comment 40

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3131a0a53427

Comment 41

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=963812aad90a

Comment 42

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb0c5b4dee1b

Comment 43

[Gian-Carlo Pascutto [:gcp]]

I really don't get the B2G failures.

07:11:15     INFO -  09-04 14:11:07.248 I/GeckoDump(  730): Error getting tracking ID [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://b2g/content/settings.js :: setUpdateTrackingId :: line 279"  data: no]

let dntValue =  Services.prefs.getIntPref('privacy.donottrackheader.value');

The patch doesn't change anything there. It looks like moving around the other "browser.trackingprotection" prefs now causes a different path to be taken in this code?

Comment 44

[Gian-Carlo Pascutto [:gcp]]

I'm still baffled here. Blame points to bug 1161299 but that's a patch that was backed out with nobody investigating or understanding what was up...

Fabrice, Francois, does any of you have any idea why changing the safebrowsing prefs suddenly causes this to fail:
https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#279

Comment 45

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5b071d6e2ce

Comment 46

[Gian-Carlo Pascutto [:gcp]]

Last try push was green (after deleting a random unused pref). Let's hope it stays that way.

Comment 47

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b54a9c9f9b645a86099c17d3c14e6f5a9f78ba
Bug 1107372 - Update preferences for new SafeBrowsing prefs structure. r=francois

Comment 48

[Carsten Book [:Tomcat]]

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

Comment 49

[Gian-Carlo Pascutto [:gcp]]

This is causing Talos alerts, but see Bug 1188543.

Comment 50

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

Verified fixed in latest Nightly Fx43. 

I used both old and new profiles, and verified that safe browsing is still functional using tests located here: https://wiki.mozilla.org/Phishing_Protection#QA.