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..be675a1372f5a1b20abbb46d7c8322b065d847de 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,10 @@ ClientDownloadRequest::DownloadType GetDownloadType(const FilePath& file) {
     return ClientDownloadRequest::ANDROID_APK;
   else if (file.MatchesExtension(FILE_PATH_LITERAL(".crx")))
     return ClientDownloadRequest::CHROME_EXTENSION;
+  // For zip files, we use the ZIPPED_EXECUTABLE type since we will only send
+  // the pingback if we find an executable inside the zip archive.
+  else if (file.MatchesExtension(FILE_PATH_LITERAL(".zip")))
+    return ClientDownloadRequest::ZIPPED_EXECUTABLE;
   return ClientDownloadRequest::WIN_EXECUTABLE;
 }
 
@@ -137,7 +149,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 +487,32 @@ 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_ARCHIVE_WITHOUT_BINARIES);
+        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() {
+    base::TimeTicks start_time = base::TimeTicks::Now();
     signature_util_->CheckSignature(info_.local_file, &signature_info_);
     bool is_signed = (signature_info_.certificate_chain_size() > 0);
     if (is_signed) {
@@ -483,14 +521,46 @@ class DownloadProtectionService::CheckClientDownloadRequest
       VLOG(2) << "Downloaded an unsigned binary: " << info_.local_file.value();
     }
     UMA_HISTOGRAM_BOOLEAN("SBClientDownload.SignedBinaryDownload", is_signed);
+    UMA_HISTOGRAM_TIMES("SBClientDownload.ExtractSignatureFeaturesTime",
+                        base::TimeTicks::Now() - start_time);
+  }
 
-    // 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() {
+    base::TimeTicks start_time = base::TimeTicks::Now();
+    zip::ZipReader reader;
+    bool zip_file_has_archive = false;
+    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();
+        if (IsBinaryFile(file)) {
+          // Don't consider an archived archive to be executable, but record
+          // a histogram.
+          if (IsArchiveFile(file)) {
+            zip_file_has_archive = true;
+          } else {
+            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);
+    UMA_HISTOGRAM_BOOLEAN("SBClientDownload.ZipFileHasArchiveButNoExecutable",
+                          zip_file_has_archive);

[mattm, 2012/05/15 01:02:35]

should it be: zip_file_has_archive && !info_.zipped_executable ?

[Brian Ryner, 2012/05/15 01:13:39]

Whoops, yes.  Fixed.

+    UMA_HISTOGRAM_TIMES("SBClientDownload.ExtractZipFeaturesTime",
+                        base::TimeTicks::Now() - start_time);
   }
 
   void CheckWhitelists() {
@@ -750,7 +820,8 @@ bool DownloadProtectionService::IsSupportedDownload(
   return (CheckClientDownloadRequest::IsSupportedDownload(info,
                                                           &reason,
                                                           &type) &&
-          ClientDownloadRequest::WIN_EXECUTABLE == type);
+          ClientDownloadRequest::WIN_EXECUTABLE == type ||
+          ClientDownloadRequest::ZIPPED_EXECUTABLE == type);
 #else
   return false;
 #endif