Check .exe's within an archive against manual-blacklist-flag. Before

we were just checking the hash of the main file.

BUG=553595

Review URL: https://codereview.chromium.org/1477523003

Cr-Commit-Position: refs/heads/master@{#362449}

2 files changed, 84 insertions, 22 deletions

diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc
index 93f81e3..caf06c2 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -447,6 +447,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
                         base::TimeTicks::Now() - start_time_);
     UMA_HISTOGRAM_TIMES("SBClientDownload.DownloadRequestNetworkDuration",
                         base::TimeTicks::Now() - request_start_time_);
+
     FinishRequest(result, reason);
   }
 
@@ -710,15 +711,6 @@ class DownloadProtectionService::CheckClientDownloadRequest
 // the server if we're not on one of those platforms.
 // TODO(noelutz): change this code once the UI is done for Linux.
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
-    // User can manually blacklist a sha256 via flag, for testing.
-    // In this case we don't actually send the request.
-    if (item_ && service_ &&
-        service_->IsHashManuallyBlacklisted(item_->GetHash())) {
-      DVLOG(1) << "Download verdict overridden to DANGEROUS by flag.";
-      PostFinishTask(DANGEROUS, REASON_MANUAL_BLACKLIST);
-      return;
-    }
-
     // The URLFetcher is owned by the UI thread, so post a message to
     // start the pingback.
     BrowserThread::PostTask(
@@ -771,6 +763,19 @@ class DownloadProtectionService::CheckClientDownloadRequest
     SendRequest();
   }
 
+  // If the hash of either the original file or any executables within an
+  // archive matches the blacklist flag, return true.
+  bool IsDownloadManuallyBlacklisted(const ClientDownloadRequest& request) {
+    if (service_->IsHashManuallyBlacklisted(request.digests().sha256()))
+      return true;
+
+    for (auto bin_itr : request.archived_binary()) {
+      if (service_->IsHashManuallyBlacklisted(bin_itr.digests().sha256()))
+        return true;
+    }
+    return false;
+  }
+
   void SendRequest() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -850,8 +855,18 @@ class DownloadProtectionService::CheckClientDownloadRequest
       FinishRequest(UNKNOWN, REASON_INVALID_REQUEST_PROTO);
       return;
     }
-    service_->client_download_request_callbacks_.Notify(item_, &request);
 
+    // User can manually blacklist a sha256 via flag, for testing.
+    // This is checked just before the request is sent, to verify the request
+    // would have been sent.  This emmulates the server returning a DANGEROUS
+    // verdict as closely as possible.
+    if (IsDownloadManuallyBlacklisted(request)) {
+      DVLOG(1) << "Download verdict overridden to DANGEROUS by flag.";
+      PostFinishTask(DANGEROUS, REASON_MANUAL_BLACKLIST);
+      return;
+    }
+
+    service_->client_download_request_callbacks_.Notify(item_, &request);
     DVLOG(2) << "Sending a request for URL: "
              << item_->GetUrlChain().back();
     DVLOG(2) << "Detected " << request.archived_binary().size() << " archived "
diff --git a/chrome/browser/safe_browsing/download_protection_service_unittest.cc b/chrome/browser/safe_browsing/download_protection_service_unittest.cc
index f54a270..3ec5c7b 100644
--- a/chrome/browser/safe_browsing/download_protection_service_unittest.cc
+++ b/chrome/browser/safe_browsing/download_protection_service_unittest.cc
@@ -2035,29 +2035,29 @@ TEST_F(DownloadProtectionServiceTest, ShowDetailsForDownloadHasContext) {
 class DownloadProtectionServiceFlagTest : public DownloadProtectionServiceTest {
  protected:
   DownloadProtectionServiceFlagTest()
-      : blacklisted_hash_("abcdefghijklmnopqrstuvwxyz012345") {}
+      // Matches unsigned.exe within zipfile_one_unsigned_binary.zip
+      : blacklisted_hash_hex_("1e954d9ce0389e2ba7447216f21761f98d1e6540c2abecdbecff570e36c493db") {}
 
   void SetUp() override {
+    std::vector<uint8> bytes;
+    ASSERT_TRUE(base::HexStringToBytes(blacklisted_hash_hex_, &bytes) &&
+                bytes.size() == 32);
+    blacklisted_hash_ = std::string(bytes.begin(), bytes.end());
+
     base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
         switches::kSbManualDownloadBlacklist,
-        base::HexEncode(blacklisted_hash_.c_str(), blacklisted_hash_.size()));
+        blacklisted_hash_hex_);
+
     DownloadProtectionServiceTest::SetUp();
   }
 
+  // Hex 64 chars
+  const std::string blacklisted_hash_hex_;
   // Binary 32 bytes
-  const std::string blacklisted_hash_;
+  std::string blacklisted_hash_;
 };
 
 TEST_F(DownloadProtectionServiceFlagTest, CheckClientDownloadOverridenByFlag) {
-  ClientDownloadResponse response;
-  response.set_verdict(ClientDownloadResponse::SAFE);
-  net::FakeURLFetcherFactory factory(NULL);
-  // Empty response means SAFE.
-  factory.SetFakeResponse(
-      DownloadProtectionService::GetDownloadRequestUrl(),
-      response.SerializeAsString(),
-      net::HTTP_OK, net::URLRequestStatus::SUCCESS);
-
   base::FilePath a_tmp(FILE_PATH_LITERAL("a.tmp"));
   base::FilePath a_exe(FILE_PATH_LITERAL("a.exe"));
   std::vector<GURL> url_chain;
@@ -2103,4 +2103,51 @@ TEST_F(DownloadProtectionServiceFlagTest, CheckClientDownloadOverridenByFlag) {
 #endif
 }
 
+// Test a real .zip with a real .exe in it, where the .exe is manually
+// blacklisted by hash.
+TEST_F(DownloadProtectionServiceFlagTest,
+       CheckClientDownloadZipOverridenByFlag) {
+  const base::FilePath zip_path =
+      testdata_path_.AppendASCII("zipfile_one_unsigned_binary.zip");
+
+  base::FilePath final_path(FILE_PATH_LITERAL("a.zip"));
+  std::vector<GURL> url_chain;
+  url_chain.push_back(GURL("http://www.evil.com/a.zip"));
+  GURL referrer("http://www.google.com/");
+  std::string zip_hash("hash");  // not blacklisted
+
+  content::MockDownloadItem item;
+  EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(zip_path));
+  EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(final_path));
+  EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
+  EXPECT_CALL(item, GetReferrerUrl()).WillRepeatedly(ReturnRef(referrer));
+  EXPECT_CALL(item, GetTabUrl()).WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+  EXPECT_CALL(item, GetTabReferrerUrl())
+      .WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+  EXPECT_CALL(item, GetHash()).WillRepeatedly(ReturnRef(zip_hash));
+  EXPECT_CALL(item, GetReceivedBytes()).WillRepeatedly(Return(100));
+  EXPECT_CALL(item, HasUserGesture()).WillRepeatedly(Return(true));
+  EXPECT_CALL(item, GetRemoteAddress()).WillRepeatedly(Return(""));
+
+  EXPECT_CALL(*sb_service_->mock_database_manager(),
+              MatchDownloadWhitelistUrl(_))
+      .WillRepeatedly(Return(false));
+
+  download_service_->CheckClientDownload(
+      &item, base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                        base::Unretained(this)));
+  MessageLoop::current()->Run();
+
+  EXPECT_FALSE(HasClientDownloadRequest());
+#if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
+  // Overriden by flag:
+  EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
+#else
+  // On !(OS_WIN || OS_MACOSX || OS_CHROMEOS),
+  // no file types are currently supported. Hence all
+  // requests to CheckClientDownload() result in a verdict of UNKNOWN.
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#endif
+}
+
 }  // namespace safe_browsing