Add SafeBrowsing support for checking downloaded zip files that contain executables.

chrome/browser/safe_browsing/download_protection_service.cc

Index: chrome/browser/safe_browsing/download_protection_service.cc
diff --git a/chrome/browser/safe_browsing/download_protection_service.cc b/chrome/browser/safe_browsing/download_protection_service.cc
index 797ee20be4ee3de616db5cbefeab9734f22b588b..ad464cdc306753b57f564e0984ebac7160a0d17b 100644
--- a/chrome/browser/safe_browsing/download_protection_service.cc
+++ b/chrome/browser/safe_browsing/download_protection_service.cc
@@ -22,6 +22,7 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/common/zip_reader.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/page_navigator.h"
@@ -46,12 +47,19 @@ const char DownloadProtectionService::kDownloadRequestUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/download";
 
 namespace {
+bool IsArchiveFile(const FilePath& file) {
+  return file.MatchesExtension(FILE_PATH_LITERAL(".zip"));
+}
+
 bool IsBinaryFile(const FilePath& file) {
   return (file.MatchesExtension(FILE_PATH_LITERAL(".exe")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".cab")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".msi")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".crx")) ||
-          file.MatchesExtension(FILE_PATH_LITERAL(".apk")));
+          file.MatchesExtension(FILE_PATH_LITERAL(".apk")) ||
+          // Archives _may_ contain binaries, we'll check in
+          // ExtractFileFeatures.
+          IsArchiveFile(file));
 }
 
 ClientDownloadRequest::DownloadType GetDownloadType(const FilePath& file) {
@@ -60,6 +68,8 @@ ClientDownloadRequest::DownloadType GetDownloadType(const FilePath& file) {
     return ClientDownloadRequest::ANDROID_APK;
   else if (file.MatchesExtension(FILE_PATH_LITERAL(".crx")))
     return ClientDownloadRequest::CHROME_EXTENSION;
+  else if (file.MatchesExtension(FILE_PATH_LITERAL(".zip")))
+    return ClientDownloadRequest::ZIPPED_WIN_EXECUTABLE;

[mattm, 2012/05/11 22:42:06]

Seems a little weird to say this since we haven't looked in the zip file yet,
right?  Is the idea that we only ever send the ping if that happens to be true?

[Brian Ryner, 2012/05/12 00:06:59]

Yep, that's right.  Added a comment to clarify.

   return ClientDownloadRequest::WIN_EXECUTABLE;
 }
 
@@ -137,7 +147,7 @@ enum SBStatsType {
 }  // namespace
 
 DownloadProtectionService::DownloadInfo::DownloadInfo()
-    : total_bytes(0), user_initiated(false) {}
+    : total_bytes(0), user_initiated(false), zipped_executable(false) {}
 
 DownloadProtectionService::DownloadInfo::~DownloadInfo() {}
 
@@ -475,6 +485,31 @@ class DownloadProtectionService::CheckClientDownloadRequest
   }
 
   void ExtractFileFeatures() {
+    // If we're checking an archive file, look to see if there are any
+    // executables inside.  If not, we will skip the pingback for this
+    // download.
+    if (info_.target_file.MatchesExtension(FILE_PATH_LITERAL(".zip"))) {
+      ExtractZipFeatures();
+      if (!info_.zipped_executable) {
+        RecordImprovedProtectionStats(REASON_NOT_BINARY_FILE);

[noelutz, 2012/05/11 17:54:33]

might be nice to have a separate reason for that.

[Brian Ryner, 2012/05/11 22:25:44]

Done.

+        PostFinishTask(SAFE);
+        return;
+      }
+    } else {
+      DCHECK(!IsArchiveFile(info_.target_file));
+      ExtractSignatureFeatures();
+    }
+
+    // TODO(noelutz): DownloadInfo should also contain the IP address of
+    // every URL in the redirect chain.  We also should check whether the
+    // download URL is hosted on the internal network.
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&CheckClientDownloadRequest::CheckWhitelists, this));
+  }
+
+  void ExtractSignatureFeatures() {
     signature_util_->CheckSignature(info_.local_file, &signature_info_);
     bool is_signed = (signature_info_.certificate_chain_size() > 0);
     if (is_signed) {
@@ -483,14 +518,33 @@ class DownloadProtectionService::CheckClientDownloadRequest
       VLOG(2) << "Downloaded an unsigned binary: " << info_.local_file.value();
     }
     UMA_HISTOGRAM_BOOLEAN("SBClientDownload.SignedBinaryDownload", is_signed);
+  }
 
-    // TODO(noelutz): DownloadInfo should also contain the IP address of every
-    // URL in the redirect chain.  We also should check whether the download
-    // URL is hosted on the internal network.
-    BrowserThread::PostTask(
-        BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&CheckClientDownloadRequest::CheckWhitelists, this));
+  void ExtractZipFeatures() {

[noelutz, 2012/05/11 17:54:33]

Would you mind adding a histogram for both this method and the signature method
to measure how long it takes?  We're seeing quite a few requests being cancelled
on the client (>5%).  It would be nice to see if it's because these checks take
too long.

[Brian Ryner, 2012/05/11 22:25:44]

Done.

+    zip::ZipReader reader;
+    if (reader.Open(info_.local_file)) {
+      for (; reader.HasMore(); reader.AdvanceToNextEntry()) {
+        if (!reader.OpenCurrentEntryInZip()) {
+          VLOG(1) << "Failed to open current entry in zip file: "
+                  << info_.local_file.value();
+          continue;
+        }
+        const FilePath& file = reader.current_entry_info()->file_path();
+        // Don't consider an archived archive to be executable.
+        if (IsBinaryFile(file) && !IsArchiveFile(file)) {
+          VLOG(2) << "Downloaded a zipped executable: "
+                  << info_.local_file.value();
+          info_.zipped_executable = true;
+          break;
+        } else {
+          VLOG(3) << "Ignoring non-binary file: " << file.value();
+        }
+      }
+    } else {
+      VLOG(1) << "Failed to open zip file: " << info_.local_file.value();
+    }
+    UMA_HISTOGRAM_BOOLEAN("SBClientDownload.ZipFileHasExecutable",
+                          info_.zipped_executable);
   }
 
   void CheckWhitelists() {
@@ -750,7 +804,8 @@ bool DownloadProtectionService::IsSupportedDownload(
   return (CheckClientDownloadRequest::IsSupportedDownload(info,
                                                           &reason,
                                                           &type) &&
-          ClientDownloadRequest::WIN_EXECUTABLE == type);
+          ClientDownloadRequest::WIN_EXECUTABLE == type ||
+          ClientDownloadRequest::ZIPPED_WIN_EXECUTABLE == type);
 #else
   return false;
 #endif