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

VERIFIED FIXED in Firefox 43

Status

()

Core
DOM: Security
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: hectorz, Assigned: gcp)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

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

Comment 2

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

Comment 4

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

Comment 6

2 years ago
>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.)
(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.
Blocks: 1149867
(Assignee)

Comment 8

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a7413b6f562
(Assignee)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03be6ea66d54
(Assignee)

Comment 10

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e5e4c22eb11
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd6ffc38c4e6
(Assignee)

Comment 12

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a2aa6641cc
(Assignee)

Comment 13

2 years ago
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.
Attachment #8652341 - Flags: review?(francois)
(Reporter)

Comment 14

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

Comment 15

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

Comment 16

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

Comment 17

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

Comment 18

2 years ago
(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.
(Reporter)

Comment 19

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

Comment 20

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

Comment 21

2 years ago
(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 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".
Attachment #8652341 - Flags: review?(francois) → review-
Assignee: nobody → gpascutto
Status: NEW → ASSIGNED
(Assignee)

Comment 23

2 years ago
Created attachment 8653329 [details] [diff] [review]
Update preferences for new SafeBrowsing prefs structure

Addressed review comments.
Attachment #8653329 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8652341 - Attachment is obsolete: true
Attachment #8653329 - Flags: review?(francois) → review+
QA Contact: mwobensmith

Comment 24

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2734a3110b4a
(Assignee)

Comment 25

2 years ago
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.
See bug 1192416 for how the same issue has been solved in the past.
(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
Flags: needinfo?(gpascutto)

Comment 28

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6782a855c243
(Assignee)

Comment 29

2 years ago
Created attachment 8655465 [details] [diff] [review]
Patch talos for new SB prefs.
Flags: needinfo?(gpascutto)
Attachment #8655465 - Flags: review?(jmaher)
(Assignee)

Comment 30

2 years ago
Created attachment 8655466 [details] [diff] [review]
Update preferences for new SafeBrowsing prefs structure

Carrying forward r+ but fix a truncated line error.
Attachment #8655466 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8653329 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03bfe3ea56c2
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.
Attachment #8655465 - Flags: review?(jmaher) → review+
(Assignee)

Comment 33

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

Comment 34

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b317ee483a6415c07aaa8337c2cbd4d588524576
Bug 1107372 - Update preferences for new SafeBrowsing prefs structure. r=francois
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13649742&repo=mozilla-inbound
Flags: needinfo?(gpascutto)

Comment 36

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/174ee19829ec
(Assignee)

Comment 37

2 years ago
Ugh, a second line was truncated in the Androids prefs...
Flags: needinfo?(gpascutto)
(Assignee)

Comment 38

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b0fe4cfdf0b
Blew up B2G reftests too.
(Assignee)

Comment 40

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3131a0a53427
(Assignee)

Comment 41

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=963812aad90a
(Assignee)

Comment 42

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb0c5b4dee1b
(Assignee)

Comment 43

2 years ago
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?
(Assignee)

Comment 44

2 years ago
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
Flags: needinfo?(francois)
Flags: needinfo?(fabrice)
(Assignee)

Comment 45

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5b071d6e2ce
(Assignee)

Comment 46

2 years ago
Last try push was green (after deleting a random unused pref). Let's hope it stays that way.
Flags: needinfo?(francois)
Flags: needinfo?(fabrice)
(Assignee)

Comment 47

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b54a9c9f9b645a86099c17d3c14e6f5a9f78ba
Bug 1107372 - Update preferences for new SafeBrowsing prefs structure. r=francois
https://hg.mozilla.org/mozilla-central/rev/f1b54a9c9f9b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 49

2 years ago
This is causing Talos alerts, but see Bug 1188543.
Depends on: 1204367

Updated

2 years ago
Depends on: 1205389
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.
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified

Updated

2 years ago
Depends on: 1186585
Depends on: 1237103
Depends on: 1237132
(Assignee)

Updated

a year ago
No longer depends on: 1186585
You need to log in before you can comment on or make changes to this bug.