Closed Bug 1107372 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- verified

People

(Reporter: hectorz, Assigned: gcp)

References

Details

Attachments

(2 files, 2 obsolete files)

Patch talos for new SB prefs.
1.88 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Update preferences for new SafeBrowsing prefs structure
29.89 KB, patch
gcp
: review+
Details | Diff | Splinter Review 
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.
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
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.
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?
>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.
Bug 1107372 - Restructure SafeBrowsing prefs to allow multiple providers.
Attachment #8652341 - Flags: review?(francois)
(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.
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.
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>
(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
(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.
(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
(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.
(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
Addressed review comments.
Attachment #8653329 - Flags: review?(francois)
Attachment #8652341 - Attachment is obsolete: true
Attachment #8653329 - Flags: review?(francois) → review+
QA Contact: mwobensmith
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)
Flags: needinfo?(gpascutto)

        Attachment #8655465 -
        Flags: review?(jmaher)

    
    

    
  


  
Carrying forward r+ but fix a truncated line error.

        Attachment #8655466 -
        Flags: review+

    
  

        Attachment #8653329 -
        Attachment is obsolete: true

    
    

    
  
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+

    
    

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

    
    

    
  
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)

    
    

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

    
    

    
  
Blew up B2G reftests too.

    
    

    
  
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?

    
    

    
  
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)

    
    

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

    
    

    
  
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
Closed: 8 years ago

          status-firefox43:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

    
    

    
  
This is causing Talos alerts, but see Bug 1188543.
Depends on: 1204367
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
Depends on: CVE-2016-1947
Depends on: 1237132

    
  
No longer depends on: 1186585

          You need to log in
          before you can comment on or make changes to this bug.