Closed Bug 1033450 Opened 5 years ago Closed 4 years ago

consolidate safebrowsing prefs in all.js

Categories

(Toolkit :: Safe Browsing, defect, P2)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: mmc, Assigned: dimi)

References

Details

Attachments

(1 file, 2 obsolete files)

They are currently in firefox.js, mobile.js, and b2g.js(!). Some are wrapped in ifdef MOZ_SAFE_BROWSING and some are not. Update URLs are only defined in firefox.js and mobile.js, though some safebrowsing prefs only(!) exist in b2g.js.

pref("browser.safebrowsing.enabled", true);
pref("browser.safebrowsing.malware.enabled", true);
pref("browser.safebrowsing.debug", true);

pref("browser.safebrowsing.updateURL", "https://safebrowsing.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2&key=%GOOGLE_API_KEY%");
pref("browser.safebrowsing.gethashURL", "https://safebrowsing.google.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
pref("browser.safebrowsing.reportURL", "https://safebrowsing.google.com/safebrowsing/report?");
pref("browser.safebrowsing.reportGenericURL", "http://%LOCALE%.phish-generic.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportErrorURL", "http://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportPhishURL", "http://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportMalwareURL", "http://%LOCALE%.malware-report.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportMalwareErrorURL", "http://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%");
Francois, not sure how important that is. Can you please apply a priority or mark as wontfix if not needed anymore.
Flags: needinfo?(francois)
Whiteboard: [domsecurity-backlog]
Component: DOM: Security → Safe Browsing
Flags: needinfo?(francois)
Product: Core → Toolkit
Whiteboard: [domsecurity-backlog]
Good first bug since it's easy to fix but requires testing the various ways in which Safe Browsing is used.
Whiteboard: tpe-seceng
I would like to work on this bug.
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Comment on attachment 8731545 [details] [diff] [review]
(WIP) P1. Consolidate safebrowsing prefs in all.js

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

::: mobile/android/app/mobile.js
@@ -653,5 @@
>  
>  // optimize images memory usage
>  pref("image.downscale-during-decode.enabled", true);
>  
>  #ifdef MOZ_SAFE_BROWSING

It would be interesting as part of this bug to verify whether or not we can build Firefox without MOZ_SAFE_BROWSING.

@@ -660,1 @@
>  pref("browser.safebrowsing.downloads.enabled", false);

Note: we'll need to confirm (by building on Fennec and testing manually) that this pref override takes precedence over the default that's set in all.js because Fennec doesn't yet have the UI to handle blocked downloads.
Only run try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ffe3429e15

Not yet verify on various platform
Attachment #8731545 - Attachment description: WIP patch → (WIP) P1. Consolidate safebrowsing prefs in all.js
You could move your second patch to bug 336290.
Duplicate of this bug: 534548
Hi François,
Sorry pending this for a while.

I have some question while testing, i guess i can check with you next week in taipei.
Just leave some note first

A. http://testsafebrowsing.appspot.com/
[1]Desktop Webpage Warnings - 4. Client-side phishing detection,
The page is not blocked, what is the expected behavior ?

[2]Desktop Download Warnings - 4,5,6,7 Windows platform download
Is there any other way to test it on ubuntu or mac ? or it can only be test on windows.

[3]Bad IP Warnings
After clicking link from http://testsafebrowsing.appspot.com/s/badip.html, i didn't see anything special, but i did see warning when use chrome.
What do we expect after navigating to thpse pages. 

B.https://wiki.mozilla.org/Security/Tracking_protection#QA
I turned on Pref privacy.trackingprotection.enabled
And the link from "Strict blacklist" still shows "Your browser is not set up for tracking protection. Please check your setup and try again" , is this correct ?
(In reply to Dimi Lee[:dimi][:dlee] from comment #9)
> A. http://testsafebrowsing.appspot.com/
> [1]Desktop Webpage Warnings - 4. Client-side phishing detection,
> The page is not blocked, what is the expected behavior ?

Yes, we don't implement the client-side stuff in Firefox.

> [2]Desktop Download Warnings - 4,5,6,7 Windows platform download
> Is there any other way to test it on ubuntu or mac ? or it can only be test
> on windows.

You should be able to test them on Linux or Mac in Firefox.

> [3]Bad IP Warnings
> After clicking link from http://testsafebrowsing.appspot.com/s/badip.html, i
> didn't see anything special, but i did see warning when use chrome.
> What do we expect after navigating to thpse pages. 

We don't implement the Bad IP Warnings in Firefox. I've added a note on https://wiki.mozilla.org/Security/Safe_Browsing#QA.

> B.https://wiki.mozilla.org/Security/Tracking_protection#QA
> I turned on Pref privacy.trackingprotection.enabled
> And the link from "Strict blacklist" still shows "Your browser is not set up
> for tracking protection. Please check your setup and try again" , is this
> correct ?

In order for that one to work, you need to switch to the "Strict" tracking protection list:

about:preferences | Privacy | Use Tracking Protection | Change Block List | Disconnect.me strict protection.
Attachment #8731545 - Attachment is obsolete: true
Attachment #8732697 - Attachment is obsolete: true
I have test this patch manually on both desktop and fennec with following tests

[1]https://wiki.mozilla.org/Phishing_Protection
- Malware, phishing, unwanted software and forbidden

[2]http://testsafebrowsing.appspot.com/
- Desktop Webpage Warnings
- Desktop Download Warnings (enabled in desktop and disabled in fennec)

[3]http://itisatrap.org/firefox/forbidden.html

[4]https://wiki.mozilla.org/Security/Tracking_protection#QA
- Blacklist and whitelist using hardcoded values (Show tracker)
- Standard blacklist (default not enabled)
- Strict blacklist (default not enabled)

Also try looks ok:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ea3d5488b3d
Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review42531

As discussed in person.

::: b2g/app/b2g.js:350
(Diff revision 1)
>  
>  #ifdef MOZ_SAFE_BROWSING
> -pref("browser.safebrowsing.enabled", true);
> -// Prevent loading of pages identified as malware
> -pref("browser.safebrowsing.malware.enabled", true);
>  pref("browser.safebrowsing.downloads.enabled", true);

We can move these ones to libpref as well and only override them in Fennec.

::: b2g/app/b2g.js:353
(Diff revision 1)
> -
> -pref("browser.safebrowsing.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportPhishURL", "https://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url=");
>  
>  pref("browser.safebrowsing.id", "Firefox");

This is the same as the Fennec's use of @MOZ_APP_UA_NAME@ so we don't need to hardcode "Firefox" here.

::: browser/app/profile/firefox.js:967
(Diff revision 1)
> -
> -pref("browser.safebrowsing.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportPhishURL", "https://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url=");
>  
>  #ifdef MOZILLA_OFFICIAL

Let's move this block to libpref and add:

    #else
    pref("browser.safebrowsing.id", "@MOZ_APP_UA_NAME@");
    #endif

so that the ID is the same on all platforms for official and non-official builds.

::: browser/app/profile/firefox.js:975
(Diff revision 1)
>  pref("browser.safebrowsing.id", "navclient-auto-ffox");
>  #endif
>  
>  // Name of the about: page contributed by safebrowsing to handle display of error
>  // pages on phishing/malware hits.  (bug 399233)
>  pref("urlclassifier.alternate_error_page", "blocked");

This can also move to libpref.

::: mobile/android/app/mobile.js:648
(Diff revision 1)
>  pref("image.downscale-during-decode.enabled", true);
>  
>  #ifdef MOZ_SAFE_BROWSING
> -pref("browser.safebrowsing.enabled", true);
> -pref("browser.safebrowsing.malware.enabled", true);
>  pref("browser.safebrowsing.downloads.enabled", false);

We need to make sure that this override still works (i.e. malware downloads are not blocked on Fennec).

::: mobile/android/app/mobile.js:649
(Diff revision 1)
>  
>  #ifdef MOZ_SAFE_BROWSING
> -pref("browser.safebrowsing.enabled", true);
> -pref("browser.safebrowsing.malware.enabled", true);
>  pref("browser.safebrowsing.downloads.enabled", false);
>  pref("browser.safebrowsing.downloads.remote.enabled", false);

This second pref is unnecessary given that `browser.safebrowsing.downloads.enabled` is already set to `false`.

::: modules/libpref/init/all.js:4950
(Diff revision 1)
>  pref("dom.inter-app-communication-api.enabled", false);
>  
>  // Disable mapped array buffer by default.
>  pref("dom.mapped_arraybuffer.enabled", false);
>  
> +#ifdef MOZ_SAFE_BROWSING

I think we should leave this out since it's being removed anyways in a different bug.

::: modules/libpref/init/all.js:4969
(Diff revision 1)
> +pref("urlclassifier.downloadBlockTable", "goog-badbinurl-shavar");
> +
> +// The number of random entries to send with a gethash request.
> +pref("urlclassifier.gethashnoise", 4);
> +
> +// Gethash timeout for Safebrowsing.	

nit: trailing whitespace

::: modules/libpref/init/all.js:5020
(Diff revision 1)
>  pref("browser.safebrowsing.provider.mozilla.lists.mozfull.name", "mozfullName");
>  pref("browser.safebrowsing.provider.mozilla.lists.mozfull.description", "mozfullDesc");
>  
>  // Allow users to ignore Safe Browsing warnings.
>  pref("browser.safebrowsing.allowOverride", true);
> +#endif

We also need to remove this since we'll leave the `ifdef` out.
Attachment #8740738 - Flags: review?(francois)
Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45977/diff/1-2/
Attachment #8740738 - Flags: review?(francois)
Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review44003

That looks great Dimi!

If you haven't tested that Download Protection is disabled on Fennec (i.e. you can download all of the test malware binaries from <http://testsafebrowsing.appspot.com>), then please do that before landing.
Attachment #8740738 - Flags: review?(francois) → review+
QA Contact: mwobensmith
(In reply to François Marier [:francois] from comment #15)
> If you haven't tested that Download Protection is disabled on Fennec (i.e.
> you can download all of the test malware binaries from
> <http://testsafebrowsing.appspot.com>), then please do that before landing.

Yes, I have test download protection on fennec, it is disabled.
The try result looks strange now, so i pull latest code and push to try again to see if anything different.
Blocks: 1265359
Blocks: 1265358
https://hg.mozilla.org/mozilla-central/rev/f71acdbd0d45
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Good job! Thanks, Dimi and François!
The patch that landed on central (https://hg.mozilla.org/mozilla-central/rev/f71acdbd0d45) is not actually the right one, this needs to be backed out and re-done.

The first revision (https://reviewboard.mozilla.org/r/45977/diff/1/) is what landed whereas the second revision (https://reviewboard.mozilla.org/r/45977/diff/2/) is what should have landed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backout and re-landed in  https://hg.mozilla.org/integration/fx-team/rev/2271b0c11307 

François: could you take a look at this and verify i landed the correct patch ? Thanks!
Flags: needinfo?(francois)
(In reply to Carsten Book [:Tomcat] from comment #23)
> backout and re-landed in 
> https://hg.mozilla.org/integration/fx-team/rev/2271b0c11307 
> 
> François: could you take a look at this and verify i landed the correct
> patch ? Thanks!

Looks great, thanks!
Flags: needinfo?(francois)
sorry had to back this out again for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8905525&repo=fx-team
Flags: needinfo?(dlee)
Since we would like to land bug 1265359 and bug 1265358 later today, if the two bugs land first I simply suggest rebasing this patch taking into consideration the simple pref changes made there.

As François pointed out, these changes can be moved to "all.js". As far as I can tell by looking at the patches, this might mean that these checks would be enabled on B2G as well, but not on Android where the entire feature is disabled. If this needs to be disabled for B2G, this could be handled later in a separate bug.
The pref changes landed in fx-team.
No longer blocks: 1265358, 1265359
Depends on: 1265358, 1265359
(In reply to :Paolo Amadini from comment #27)
> Since we would like to land bug 1265359 and bug 1265358 later today, if the
> two bugs land first I simply suggest rebasing this patch taking into
> consideration the simple pref changes made there.

Makes perfect sense. It looks like we have a bit more work to do looking into the test failures.
Dimi, one more thing we should add to the patch is to move this block from https://hg.mozilla.org/integration/fx-team/rev/2271b0c11307#l2.56:

  #ifdef XP_WIN
  // Only download the whitelist on Windows, since the whitelist is
  // only useful for suppressing remote lookups for signed binaries which we can
  // only verify on Windows (Bug 974579). Other platforms always do remote lookups.
  pref("urlclassifier.downloadAllowTable", "goog-downloadwhite-digest256");
  #endif

into modules/libpref/init/all.js too.

That way, all of the prefs are in the same place except for the overrides.
(In reply to François Marier [:francois] from comment #30)
> 
> That way, all of the prefs are in the same place except for the overrides.

Ok, i will add it to next patch
(In reply to Carsten Book [:Tomcat] from comment #25)
> sorry had to back this out again for failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=8905525&repo=fx-team

I think the root cause is that the annotation @PREF_NAME@ can only be used in fennec not in desktop.
This line : pref("browser.safebrowsing.id", @MOZ_APP_UA_NAME@);  in all.js cause this try error.

I am sorry that somehow i miss this change when push to try (Comment 17) so i thought everything is fine :(

Push to try again to see if everything works well after applying fix
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55e06196947
Flags: needinfo?(dlee)
Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45977/diff/2-3/
Hi Francois
Try looks good(Comment 32), so I update the patch to review again, following is the change in this patch:
1. Address Comment 30
2. Move pref("browser.safebrowsing.id", @MOZ_APP_UA_NAME@);  to mobile.js
3. In all.js, use

#ifdef MOZILLA_OFFICIAL
pref("browser.safebrowsing.id", "navclient-auto-ffox");
#else
pref("browser.safebrowsing.id", "Firefox");
#endif

Originally if MOZILLA_OFFICIAL is not defined, desktop will get id by |Services.appinfo.name|[1], which is "FireFox" for desktop.So i use |pref("browser.safebrowsing.id", "Firefox");| directly. Please let me know if you have any concern about this

[1]https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/SafeBrowsing.jsm#185
Flags: needinfo?(francois)
Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review45915

That looks all good except for the default value of the UNCOMMON and UNWANTED prefs. These were just modified (when the UI landed in Fx48) so we need to set them both to true.

::: modules/libpref/init/all.js:5005
(Diff revision 3)
> +
> +// Name of the about: page contributed by safebrowsing to handle display of error
> +// pages on phishing/malware hits.  (bug 399233)
> +pref("urlclassifier.alternate_error_page", "blocked");
> +
> +pref("browser.safebrowsing.enabled", true);

nit: let's add "// Enable phishing protection" to the end of this line just to remind folks that it's not a global "disable all of safe browsing" switch.

::: modules/libpref/init/all.js:5014
(Diff revision 3)
> +pref("browser.safebrowsing.downloads.remote.enabled", true);
> +pref("browser.safebrowsing.downloads.remote.timeout_ms", 10000);
> +pref("browser.safebrowsing.downloads.remote.url", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> +pref("browser.safebrowsing.downloads.remote.block_dangerous",            true);
> +pref("browser.safebrowsing.downloads.remote.block_dangerous_host",       true);
> +pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", false);

This should now default to `true` (it just changed).

::: modules/libpref/init/all.js:5015
(Diff revision 3)
> +pref("browser.safebrowsing.downloads.remote.timeout_ms", 10000);
> +pref("browser.safebrowsing.downloads.remote.url", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> +pref("browser.safebrowsing.downloads.remote.block_dangerous",            true);
> +pref("browser.safebrowsing.downloads.remote.block_dangerous_host",       true);
> +pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", false);
> +pref("browser.safebrowsing.downloads.remote.block_uncommon",             false);

This should also now default to `true`.
Attachment #8740738 - Flags: review+
Flags: needinfo?(francois)
(In reply to François Marier [:francois] from comment #35)
> > +pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", false);
> > +pref("browser.safebrowsing.downloads.remote.block_uncommon",             false);
> 
> This should also now default to `true`.

I found currently these two preferences still set to false for b2g & fennec
Is this intended ?
(In reply to Dimi Lee[:dimi][:dlee] from comment #36)
> > This should also now default to `true`.
> 
> I found currently these two preferences still set to false for b2g & fennec
> Is this intended ?

Fennec disables all of download protection by setting browser.safebrowsing.downloads.enabled to false (because it doesn't have UI for this), so we can flip these prefs to true and it won't be affected.

B2G doesn't disable download protection, so enabling more of it will presumably be fine.
Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45977/diff/3-4/
Attachment #8740738 - Flags: review?(francois)
(In reply to François Marier [:francois] from comment #37)
> Fennec disables all of download protection by setting
> browser.safebrowsing.downloads.enabled to false (because it doesn't have UI
> for this), so we can flip these prefs to true and it won't be affected.
> 
> B2G doesn't disable download protection, so enabling more of it will
> presumably be fine.

Ok, then i will just set those two preferences to true in all.js.
Thanks!
Attachment #8740738 - Flags: review?(francois) → review+
Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review46119
Priority: -- → P2
Whiteboard: tpe-seceng
https://hg.mozilla.org/mozilla-central/rev/84bc3fe034f2
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Blocks: 1269773
I verified by doing a test pass for Safe Browsing on latest Nightly builds, Mac/Win, 2016-05-10.
Status: RESOLVED → VERIFIED
(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #44)
> I verified by doing a test pass for Safe Browsing on latest Nightly builds,
> Mac/Win, 2016-05-10.

Matt, thanks for verifying this!
No longer blocks: safebrowsingv4
You need to log in before you can comment on or make changes to this bug.