1111741 - Enable SafeBrowsing remote lookups for mac and linux

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

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

After talking to Google safebrowsing team, they are willing to accept remote lookups from mac and linux users even without binary signature verification information.

Comment 1

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

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.

Comment 2

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

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

Comment 3

[François Marier [:francois]]

Attachment: 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.

Comment 4

[François Marier [:francois]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d495fc69f4e7

Comment 5

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

(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.

Comment 6

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

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?

Comment 7

[François Marier [:francois]]

Attachment: 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?

Comment 8

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

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.

Comment 9

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

(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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69c263e8d2b9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69c263e8d2b9

Comment 12

[François Marier [:francois]]

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)]:

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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"?

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Or even something closer to the bug title. "SafeBrowsing malware detection lookups enabled for downloads on Mac and Linux"

Comment 16

[François Marier [:francois]]

(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.