1385484 - Add the login reputation whitelist behind prefs

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P2)

Description

[François Marier [:francois]]

The login reputation whitelist is the CSD_WHITELIST = 8 threat type:

https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/toolkit/components/url-classifier/chromium/safebrowsing.proto#278
https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_protocol_manager_util.cc?l=123&rcl=e65ab0a11b73c8eea0139c0757005432d6dbb0b6

We'll need to create new prefs:

- browser.safebrowsing.passwords.enabled = false
- urlclassifier.passwordAllowTable = "goog-passwordwhite-proto"

One thing to note is that it violates the protocol in that if we find a fullhash on that list, there’s no need to confirm the with server with a fullhash request. We can block things directly as if we didn't have a gethash URL configured (e.g. tracking protection list).

Comment 1

[François Marier [:francois]]

Attachment: Bug 1385484 - Cleanup Safe Browsing prefs and sync the download protection setting.

Review commit: https://reviewboard.mozilla.org/r/166290/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166290/

Comment 2

[François Marier [:francois]]

Attachment: Bug 1385484 - Remove obsolete prefs and add missing blockedURIs.

This should have been done when blockedURIs was added (bug 1237198),
when safebrowsing.enabled was renamed (bug 1025965), and when the
forbidden list format was removed (bug 1274893).

Review commit: https://reviewboard.mozilla.org/r/166292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166292/

Comment 3

[François Marier [:francois]]

Attachment: Bug 1385484 - Add the login reputation whitelist behind a pref.

Review commit: https://reviewboard.mozilla.org/r/166294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166294/

Comment 4

[François Marier [:francois]]

Attachment: Bug 1385484 - Ensure that login reputation checks are disabled in tests.

The login reputation checks depend on a server lookup and therefore would
render non-deterministic the performance and correctness of tests.

Review commit: https://reviewboard.mozilla.org/r/166296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166296/

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8895155 [details]
Bug 1385484 - Cleanup Safe Browsing prefs and sync the download protection setting.

https://reviewboard.mozilla.org/r/166290/#review173262

The prefs move around looks good but I am uncertain about the expected change by turning on 'services.sync.prefs.sync.browser.safebrowsing.downloads.enabled'.

::: browser/app/profile/firefox.js:1168
(Diff revision 1)
>  pref("services.sync.prefs.sync.browser.newtabpage.enhanced", true);
>  pref("services.sync.prefs.sync.browser.newtabpage.pinned", true);
>  pref("services.sync.prefs.sync.browser.offline-apps.notify", true);
>  pref("services.sync.prefs.sync.browser.safebrowsing.phishing.enabled", true);
>  pref("services.sync.prefs.sync.browser.safebrowsing.malware.enabled", true);
> +pref("services.sync.prefs.sync.browser.safebrowsing.downloads.enabled", true);

Even though this pref isn't appeared and explicitly referenced, given that

http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/services/sync/modules/engines/prefs.js#92

will enumerate prefs prefixed by 'services.sync.prefs.sync.', this new pref might still enable something, shall it?

Comment 6

[Henry Chang [:hchang]]

Comment on attachment 8895156 [details]
Bug 1385484 - Remove obsolete prefs and add missing blockedURIs.

https://reviewboard.mozilla.org/r/166292/#review173270

Should the use of 'browser.safebrowsing.forbiddenURIs.enabled' in

http://searchfox.org/mozilla-central/rev/6482c8a5fa5c7446e82ef187d1a1faff49e3379e/mobile/android/chrome/content/browser.js#494

also need to be removed?

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8895157 [details]
Bug 1385484 - Add the login reputation whitelist behind a pref.

https://reviewboard.mozilla.org/r/166294/#review173272

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 8895158 [details]
Bug 1385484 - Ensure that login reputation checks are disabled in tests.

https://reviewboard.mozilla.org/r/166296/#review173342

I compared with where 'browser.safebrowsing.phishing.enabled' should be set to false

http://searchfox.org/mozilla-central/search?q=browser.safebrowsing.phishing.enabled&case=false&regexp=false&path=

and found 'browserElement_SecurityChange.js' MAY potentially require the same overriding as well.
However, from what this test case would like to test, the value of 'browser.safebrowsing.passwords.enabled'
doesn't seem to matter so we don't need to touch this test case at all.
What do you think?

Comment 9

[François Marier [:francois]]

(In reply to Henry Chang [:hchang] from comment #5)
> Comment on attachment 8895155 [details]
> Bug 1385484 - Cleanup Safe Browsing prefs and sync the download protection
> setting.
> 
> https://reviewboard.mozilla.org/r/166290/#review173262
> 
> The prefs move around looks good but I am uncertain about the expected
> change by turning on
> 'services.sync.prefs.sync.browser.safebrowsing.downloads.enabled'.

That means that the value of browser.safebrowsing.downloads.enabled will be synced if Firefox Sync is enabled.

(In reply to Henry Chang [:hchang] from comment #8)
> and found 'browserElement_SecurityChange.js' MAY potentially require the
> same overriding as well.
> However, from what this test case would like to test, the value of
> 'browser.safebrowsing.passwords.enabled'
> doesn't seem to matter so we don't need to touch this test case at all.
> What do you think?

That test is looking for specific security indicators. I don't think we need to disable download protection or password phishing protection.

Comment 10

[François Marier [:francois]]

Comment on attachment 8895155 [details]
Bug 1385484 - Cleanup Safe Browsing prefs and sync the download protection setting.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166290/diff/1-2/

Comment 11

[François Marier [:francois]]

Comment on attachment 8895156 [details]
Bug 1385484 - Remove obsolete prefs and add missing blockedURIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166292/diff/1-2/

Comment 12

[François Marier [:francois]]

Comment on attachment 8895157 [details]
Bug 1385484 - Add the login reputation whitelist behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166294/diff/1-2/

Comment 13

[François Marier [:francois]]

Comment on attachment 8895158 [details]
Bug 1385484 - Ensure that login reputation checks are disabled in tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166296/diff/1-2/

Comment 14

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: replication log not available; all writes disabled
remote: pretxnopen.vcsreplicator hook failed
abort: push failed on remote

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87ec064a3681
Cleanup Safe Browsing prefs and sync the download protection setting. r=hchang
https://hg.mozilla.org/integration/autoland/rev/1a1c476a9aaa
Remove obsolete prefs and add missing blockedURIs. r=hchang
https://hg.mozilla.org/integration/autoland/rev/2ff443a0c744
Add the login reputation whitelist behind a pref. r=hchang
https://hg.mozilla.org/integration/autoland/rev/401c2744f16b
Ensure that login reputation checks are disabled in tests. r=hchang

Comment 16

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

Backed out in https://hg.mozilla.org/integration/autoland/rev/035a63fad43bf5da717b49341cb6e3a0bdeee54b for build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=125018527&repo=autoland

> 17:21:17 <francois> KWierso: ah crap, looks like I needed to update this number to 79: https://treeherder.mozilla.org/logviewer.html#?job_id=125018527&repo=autoland&lineNumber=26892

Comment 17

[François Marier [:francois]]

Comment on attachment 8895156 [details]
Bug 1385484 - Remove obsolete prefs and add missing blockedURIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166292/diff/2-3/

Comment 18

[François Marier [:francois]]

Comment on attachment 8895157 [details]
Bug 1385484 - Add the login reputation whitelist behind a pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166294/diff/2-3/

Comment 19

[François Marier [:francois]]

Comment on attachment 8895158 [details]
Bug 1385484 - Ensure that login reputation checks are disabled in tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166296/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43594120d3e2
Cleanup Safe Browsing prefs and sync the download protection setting. r=hchang
https://hg.mozilla.org/integration/autoland/rev/02fd2c335027
Remove obsolete prefs and add missing blockedURIs. r=hchang
https://hg.mozilla.org/integration/autoland/rev/c3b2fd555480
Add the login reputation whitelist behind a pref. r=hchang
https://hg.mozilla.org/integration/autoland/rev/bf49a4faa77d
Ensure that login reputation checks are disabled in tests. r=hchang

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/43594120d3e2
https://hg.mozilla.org/mozilla-central/rev/02fd2c335027
https://hg.mozilla.org/mozilla-central/rev/c3b2fd555480
https://hg.mozilla.org/mozilla-central/rev/bf49a4faa77d

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8895156 [details]
Bug 1385484 - Remove obsolete prefs and add missing blockedURIs.

https://reviewboard.mozilla.org/r/166292/#review185050

::: testing/geckodriver/src/prefs.rs
(Diff revision 3)
>          ("browser.reader.detectedFirstArticle", Pref::new(true)),
>  
>          // Disable safebrowsing components
>          ("browser.safebrowsing.blockedURIs.enabled", Pref::new(false)),
>          ("browser.safebrowsing.downloads.enabled", Pref::new(false)),
> -        ("browser.safebrowsing.enabled", Pref::new(false)),

Francois, with which version of Firefox those preferences are no longer in use? I'm worried that those were removed here, because the next version of geckodriver needs if possible backward compatibility back to version 55.

Comment 23

[François Marier [:francois]]

Comment on attachment 8895156 [details]
Bug 1385484 - Remove obsolete prefs and add missing blockedURIs.

https://reviewboard.mozilla.org/r/166292/#review185050

> Francois, with which version of Firefox those preferences are no longer in use? I'm worried that those were removed here, because the next version of geckodriver needs if possible backward compatibility back to version 55.

That one was removed in Firefox 50 (see bug 1025965).

Comment 24

[François Marier [:francois]]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> I'm worried that those were removed here, because the next version of
> geckodriver needs if possible backward compatibility back to version 55.

They were all removed in Fx 50 or earlier. See this commit message for the bug links: https://hg.mozilla.org/mozilla-central/rev/02fd2c335027

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, that's fine then, and we do not cause a regression for our users. In the future maybe also ask for review from testing peers. Thanks.

Comment 26

[François Marier [:francois]]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> Ok, that's fine then, and we do not cause a regression for our users. In the
> future maybe also ask for review from testing peers. Thanks.

Will do. Sorry about that!