Bug 933432: Enable remote lookups for application on Windows (r=gcp)

toolkit/components/downloads/ApplicationReputation.cpp

@@ -49,9 +49,7 @@ using safe_browsing::ClientDownloadRequest;
 using safe_browsing::ClientDownloadRequest_SignatureInfo;
 using safe_browsing::ClientDownloadRequest_CertificateChain;
 
-// Preferences that we need to initialize the query. We may need another
-// preference than browser.safebrowsing.malware.enabled, or simply use
-// browser.safebrowsing.appRepURL. See bug 887041.
+// Preferences that we need to initialize the query.
 #define PREF_SB_APP_REP_URL "browser.safebrowsing.appRepURL"
 #define PREF_SB_MALWARE_ENABLED "browser.safebrowsing.malware.enabled"
 #define PREF_GENERAL_LOCALE "general.useragent.locale"
@@ -128,6 +126,9 @@ private:
   // NULLs.
   nsCString mResponse;
 
+  // Returns true if the file is likely to be binary on Windows.
+  bool IsBinaryFile();
+
   // Clean up and call the callback. PendingLookup must not be used after this
   // function is called.
   nsresult OnComplete(bool shouldBlock, nsresult rv);
@@ -333,6 +334,34 @@ PendingLookup::~PendingLookup()
   LOG(("Destroying pending lookup [this = %p]", this));
 }
 
+bool
+PendingLookup::IsBinaryFile()
+{
+  nsString fileName;
+  nsresult rv = mQuery->GetSuggestedFileName(fileName);
+  if (NS_FAILED(rv)) {
+    return false;
+  }
+  return
+    // Executable extensions for MS Windows, from
+    // https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/safe_browsing/download_protection_util.cc&l=14
+    StringEndsWith(fileName, NS_LITERAL_STRING(".apk")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".bas")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".bat")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".cab")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".cmd")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".com")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".exe")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".hta")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".msi")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".pif")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".reg")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".scr")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".vb")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".vbs")) ||
+    StringEndsWith(fileName, NS_LITERAL_STRING(".zip"));
+}
+
 nsresult
 PendingLookup::LookupNext()
 {
@@ -359,10 +388,14 @@ PendingLookup::LookupNext()
     nsRefPtr<PendingDBLookup> lookup(new PendingDBLookup(this));
     return lookup->LookupSpec(spec, allowlistOnly);
   }
-  // There are no more URIs to check against local list, so send the remote
-  // query if we can.
-  // Revert to just ifdef XP_WIN when remote lookups are enabled (bug 933432)
-#if 0
+#ifdef XP_WIN
+  // There are no more URIs to check against local list. If the file is not
+  // eligible for remote lookup, bail.
+  if (!IsBinaryFile()) {
+    LOG(("Not eligible for remote lookups [this=%x]", this));
+    return OnComplete(false, NS_OK);
+  }
+  // Send the remote query if we are on Windows.
   nsresult rv = SendRemoteQuery();
   if (NS_FAILED(rv)) {
     return OnComplete(false, rv);

toolkit/components/downloads/test/unit/test_app_rep_windows.js

@@ -205,6 +205,7 @@ add_task(function test_setup()
   });
 
   gHttpServer.registerPathHandler("/download", function(request, response) {
+    do_print("Querying remote server for verdict");
     response.setHeader("Content-Type", "application/octet-stream", false);
     let buf = NetUtil.readInputStreamToString(
       request.bodyInputStream,
@@ -215,10 +216,10 @@ add_task(function test_setup()
     let blob = "this is not a serialized protocol buffer";
     // We can't actually parse the protocol buffer here, so just switch on the
     // length instead of inspecting the contents.
-    if (buf.length == 35) {
+    if (buf.length == 45) {
       // evil.com
       blob = createVerdict(true);
-    } else if (buf.length == 38) {
+    } else if (buf.length == 48) {
       // mozilla.com
       blob = createVerdict(false);
     }
@@ -294,13 +295,17 @@ function promiseQueryReputation(query, expectedShouldBlock) {
   return deferred.promise;
 }
 
+add_task(function()
+{
+  // Wait for Safebrowsing local list updates to complete.
+  yield waitForUpdates();
+});
+
 add_task(function test_signature_whitelists()
 {
   // We should never get to the remote server.
   Services.prefs.setCharPref("browser.safebrowsing.appRepURL",
                              "http://localhost:4444/throw");
-  // Wait for Safebrowsing local list updates to complete.
-  yield waitForUpdates();
 
   // Use BackgroundFileSaver to extract the signature on Windows.
   let destFile = getTempFile(TEST_FILE_NAME_1);
@@ -325,6 +330,38 @@ add_task(function test_signature_whitelists()
                                 fileSize: 12}, false);
 });
 
+add_task(function test_blocked_binary()
+{
+  // We should reach the remote server for a verdict.
+  Services.prefs.setCharPref("browser.safebrowsing.appRepURL",
+                             "http://localhost:4444/download");
+  // evil.com should return a malware verdict from the remote server.
+  yield promiseQueryReputation({sourceURI: createURI("http://evil.com"),
+                                suggestedFileName: "noop.bat",
+                                fileSize: 12}, true);
+});
+
+add_task(function test_non_binary()
+{
+  // We should not reach the remote server for a verdict for non-binary files.
+  Services.prefs.setCharPref("browser.safebrowsing.appRepURL",
+                             "http://localhost:4444/throw");
+  yield promiseQueryReputation({sourceURI: createURI("http://evil.com"),
+                                suggestedFileName: "noop.txt",
+                                fileSize: 12}, false);
+});
+
+add_task(function test_good_binary()
+{
+  // We should reach the remote server for a verdict.
+  Services.prefs.setCharPref("browser.safebrowsing.appRepURL",
+                             "http://localhost:4444/download");
+  // mozilla.com should return a not-guilty verdict from the remote server.
+  yield promiseQueryReputation({sourceURI: createURI("http://mozilla.com"),
+                                suggestedFileName: "noop.bat",
+                                fileSize: 12}, false);
+});
+
 add_task(function test_teardown()
 {
   gStillRunning = false;