Enable SafeBrowsing remote lookups for mac and linux

VERIFIED FIXED in Firefox 39

Status

()

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

People

(Reporter: mmc, Assigned: francois)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 verified, relnote-firefox 39+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

After talking to Google safebrowsing team, they are willing to accept remote lookups from mac and linux users even without binary signature verification information.
Blocks: 662819
This change should be pretty easy.

1) Remove #ifdef XP_WIN from the remote lookup check: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/ApplicationReputation.cpp#430

2) Clone https://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_app_rep_windows.js for mac and linux and add to https://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/xpcshell.ini#14 with skip-if = os == "win"

3) Modify test_signature_whitelists() to expect to hit the remote server because we won't hit the signature verification path on mac and linux. All other test cases should work as is. If you can figure out a good way within xpcshell to do #ifdef win then you could do that instead of cloning the test file. I found https://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_udp_multicast.js#19 but haven't tried it myself.

Francois, let me know if you have any questions.
Flags: needinfo?(francois)
Depends on: 1137909
You probably also want to update this comment from https://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1016, since we'll be sending remote lookups for non windows binaries.

1013 // Only download the whitelist on Windows, since the whitelist is
1014 // only useful for suppressing remote lookups for signed binaries which we can
1015 // only verify on Windows (Bug 974579).
1016 pref("urlclassifier.downloadAllowTable", "goog-downloadwhite-digest256");
1017 #endif
(Assignee)

Updated

3 years ago
Flags: needinfo?(francois)
(Assignee)

Updated

3 years ago
Assignee: nobody → francois
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created attachment 8581426 [details] [diff] [review]
Patch and test

The xpcshell tests pass, but does Google offer bad binaries for manual testing purposes?

I tried eicar.com and it wasn't flagged as malware.
Flags: needinfo?(mmc)
Attachment #8581426 - Flags: review?(mmc)
(Assignee)

Updated

3 years ago
Summary: Enable remote lookups for mac and linux → Enable SafeBrowsing remote lookups for mac and linux
(Assignee)

Comment 4

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d495fc69f4e7
(In reply to François Marier [:francois] from comment #3)
> Created attachment 8581426 [details] [diff] [review]
> Patch and test
> 
> The xpcshell tests pass, but does Google offer bad binaries for manual
> testing purposes?
> 
> I tried eicar.com and it wasn't flagged as malware.

testsafebrowsing.appspot.com sometimes works, for both Firefox and Chrome.
Flags: needinfo?(mmc)
Comment on attachment 8581426 [details] [diff] [review]
Patch and test

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

Looks great! One minor nit: I think if you produce the unittest with hg copy rather than manually copying and adding, the version information is better and a true diff shows up in hg logs and also in splinter. Can you try it?

One more thing, it looks like you are using email as part of your workflow. You could try bzexport or bzpost, or reviewboard to save some steps (https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Extensions or ./mach rbt).

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +885,5 @@
>    if (!mRequest.SerializeToString(&serialized)) {
>      return NS_ERROR_UNEXPECTED;
>    }
> +  LOG(("Serialized protocol buffer [this = %p]: %s", this,
> +       serialized.c_str()));

This will have non-printables.

@@ +1015,5 @@
>  
>    std::string buf(mResponse.Data(), mResponse.Length());
>    safe_browsing::ClientDownloadResponse response;
>    if (!response.ParseFromString(buf)) {
>      NS_WARNING("Could not parse protocol buffer");

Get rid of this NS_WARNING, the LOG below is better.

::: toolkit/components/downloads/test/unit/test_app_rep_maclinux.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Did you produce this file with hg copy?
Attachment #8581426 - Flags: review?(mmc) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8582854 [details] [diff] [review]
Patch and test

This addresses most of the feedback from comment 6. Also, it actually enables the remote lookup pref :) Can be tested manually using the download links on http://testsafebrowsing.appspot.com/

Monica, are the non-printable characters a problem? It could be good to see them to detect malformed protobufs. That's the purpose of that LOG line.

If you think they should be filtered out, what's the best way to strip them out?
Attachment #8581426 - Attachment is obsolete: true
Flags: needinfo?(mmc)
Attachment #8582854 - Flags: review?(mmc)
Comment on attachment 8582854 [details] [diff] [review]
Patch and test

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

Thanks, that is a lot easier to review. Looks great! When you push this, you should also send a heads up to Moheeb and Noe and Stephan that we are enabling remote lookups for mac and linux in 39.

::: toolkit/components/downloads/ApplicationReputation.cpp
@@ +811,5 @@
>  PendingLookup::SendRemoteQueryInternal()
>  {
>    // If we aren't supposed to do remote lookups, bail.
>    if (!Preferences::GetBool(PREF_SB_DOWNLOADS_REMOTE_ENABLED, false)) {
> +    LOG(("Remote lookups are disabled [this = %p]", this));

You might want to prefix these log lines with the name of the module, just to make it easier to separate out NSPR output. I realize this behavior is somewhat inconsistent throughout the file.
Attachment #8582854 - Flags: review?(mmc) → review+
(In reply to François Marier [:francois] from comment #7)
> Created attachment 8582854 [details] [diff] [review]
> Patch and test
> 
> This addresses most of the feedback from comment 6. Also, it actually
> enables the remote lookup pref :) Can be tested manually using the download
> links on http://testsafebrowsing.appspot.com/
> 
> Monica, are the non-printable characters a problem? It could be good to see
> them to detect malformed protobufs. That's the purpose of that LOG line.
> 
> If you think they should be filtered out, what's the best way to strip them
> out?

That was really just an FYI :) It's OK, since these logs don't go straight to terminal without some effort.
Flags: needinfo?(mmc)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
QA Contact: mwobensmith
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c263e8d2b9
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/69c263e8d2b9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Updated

3 years ago
Keywords: feature
(Assignee)

Updated

3 years ago
Keywords: relnote
(Assignee)

Comment 12

3 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]: Mac and Linux users now benefit from the Google malware detection service
[Suggested wording]: Downloads on Mac and Linux now use the same malware detection service as Windows
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Keywords: feature, relnote
Verified fixed on 2015-05-07.

I used the test page here: 
http://testsafebrowsing.appspot.com 

There are three links under "Download warnings" to test with. The first link is never blocked; it appears to be a text file and has no file extension, so this is to be expected. The second and third links are to a blocked EXE and do not get downloaded.

FWIW, Fx's behavior looks good across Mac/Linux/Windows. However, Chrome does not block anything on Mac; only Windows (haven't tested Linux). Seems odd.
Status: RESOLVED → VERIFIED
status-firefox39: fixed → verified
François, I think linking from the release note to the support page on malware prevention might be good: https://support.mozilla.org/en-US/kb/how-does-phishing-and-malware-protection-work  

The wording in the note seems a little bit unclear.  How about "Mac and Linux downloads can optionally use SafeBrowsing malware detection"?
Flags: needinfo?(francois)
Or even something closer to the bug title. "SafeBrowsing malware detection lookups enabled for downloads on Mac and Linux"
relnote-firefox: ? → 39+
(Assignee)

Comment 16

3 years ago
(In reply to Liz Henry (:lizzard) from comment #15)
> Or even something closer to the bug title. "SafeBrowsing malware detection
> lookups enabled for downloads on Mac and Linux"

Sounds good to me.
Flags: needinfo?(francois)
You need to log in before you can comment on or make changes to this bug.