1057848 - restore remote lookup logic to application reputation

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

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

The behavior should be that if the remote url pref is empty, downloads still get checked locally and don't hang.

We can do this by removing all references to appRepUrl except in SendRemoteQueryInternal, which was already designed to trigger the appropriate callbacks in case the query doesn't get sent for whatever reason. There is no need to check for blank prefs elsewhere.

Comment 1

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

The same patch should also ensure that we never query remote reputation unless the pref that holds goog-badbinurl-shavar is non-empty, to ensure that we never send remote lookups if local checks can't be done.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

I'd like it if there was a way to turn off the Download protection but keep the malware part of SafeBrowsing enabled. They're currently behind the same pref - not having separate enable prefs for the features was a mistake, in retrospect.

Comment 3

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> I'd like it if there was a way to turn off the Download protection but keep
> the malware part of SafeBrowsing enabled. They're currently behind the same
> pref - not having separate enable prefs for the features was a mistake, in
> retrospect.

That was the case before changing (or trying to change appRepUrl) to block remote lookups only instead of the whole feature. Did you want to revert that change?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

No. We have right now:

browser.safebrowsing.enabled (confusingly doesn't actually disable the entire feature)
browser.safebrowsing.malware.enabled (enables at least 2 separate features)
browser.safebrowsing.appRepURL (double-functioning as a disable)

Let's clean this up:

browser.safebrowsing.phishing.enabled
browser.safebrowsing.malware.enabled
browser.safebrowsing.downloads.enabled
browser.safebrowsing.downloads.remote.enabled

Maybe it's offtopic for this bug and should go in a separate one. But the current settings are footgun and we already blew one leg off.

Comment 5

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)
> No. We have right now:
> 
> browser.safebrowsing.enabled (confusingly doesn't actually disable the
> entire feature)
> browser.safebrowsing.malware.enabled (enables at least 2 separate features)
> browser.safebrowsing.appRepURL (double-functioning as a disable)
> 
> Let's clean this up:
> 
> browser.safebrowsing.phishing.enabled
> browser.safebrowsing.malware.enabled

The fact that these are separate prefs is already a footgun:

https://bugzilla.mozilla.org/show_bug.cgi?id=1025358
https://bugzilla.mozilla.org/show_bug.cgi?id=1008706

Why do people turn them off? I'm guessing it's privacy. Why do people turn off one but not the other? I'm guessing that's a mistake.

If it's privacy, then we should fix https://bugzilla.mozilla.org/show_bug.cgi?id=1044308.

> browser.safebrowsing.downloads.enabled
> browser.safebrowsing.downloads.remote.enabled
> 
> Maybe it's offtopic for this bug and should go in a separate one. But the
> current settings are footgun and we already blew one leg off.

If you have feedback on this please comment on https://bugzilla.mozilla.org/show_bug.cgi?id=1053890. For hotfixing off, I agree we need a pref to disable remote lookups, which was what I was originally trying to do with the first beta patch. For user override, I think the UX proposal is good enough.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>The fact that these are separate prefs is already a footgun:
...
>I'm guessing that's a mistake.

There's a strong implication in the *current* naming of "browser.safebrowsing.enabled" that it disables the entire feature, but it doesn't. My previous comment already points this out, and you can see it in the bugs you mentioned. The confusion would not happen if the prefs are named as suggested. 

That said, I'm not opposed to combining malware+phishing into one pref, on the contrary: https://bugzilla.mozilla.org/show_bug.cgi?id=874408 But that needs actual UX work because the separate checkboxes already exist on the UI side.

>If it's privacy, then we should fix  https://bugzilla.mozilla.org/show_bug.cgi?id=1044308.

Fixing that bug on the UX side is orthogonal to cleaning up the prefs (...but having sane prefs will make the UX implementation surely easier!)

>For hotfixing off, I agree we need a pref to disable remote lookups, 
>which was what I was originally trying to do with the first beta patch. 
>For user override, I think the UX proposal is good enough.

My comments aren't related to UX, it's for our own sanity. Note you can't disable local lookups right now without disabling the SafeBrowsing part. What if people hit a false positive on the local list? (The very thing you warned me about) What if there's a bug in expiring the local blacklist? (This has happened before...)

Comment 7

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

https://tbpl.mozilla.org/?tree=Try&rev=37230ea0d538

Comment 8

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

Attachment: Restore remote lookup logic, add 2 flags for hotfixing, always set all prefs (

Comment 9

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

Comment on attachment 8480750 [details] [diff] [review]
Restore remote lookup logic, add 2 flags for hotfixing, always set all prefs (

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

I don't think we'll ever agree on whether it's reasonable to prioritize people who turn on stuff through about:config or preferences UI, but hopefully this approach won't lead to too many more footguns. Let's try to get this in 34, because strings for https://bugzilla.mozilla.org/show_bug.cgi?id=1053890 are going in 34.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8480750 [details] [diff] [review]
Restore remote lookup logic, add 2 flags for hotfixing, always set all prefs (

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

Thanks!

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +1090,5 @@
>    nsIApplicationReputationQuery* aQuery,
>    nsIApplicationReputationCallback* aCallback) {
>    nsresult rv;
>    // If malware checks aren't enabled, don't query application reputation.
>    if (!Preferences::GetBool(PREF_SB_MALWARE_ENABLED, false)) {

I'd really love to remove this so the SafeBrowsing pref isn't overloaded with AppRep, but I guess that needs UI work first. Let's keep this and wait for Bug 1044308.

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/452f87c15124

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/452f87c15124