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

chrome/browser/safe_browsing/download_protection_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop_helpers.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/time.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/signature_util.h"
 #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"
 #include "content/public/common/url_fetcher.h"
 #include "content/public/common/url_fetcher_delegate.h"
 #include "net/base/load_flags.h"
 #include "net/base/x509_cert_types.h"
 #include "net/base/x509_certificate.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 
 using content::BrowserThread;
 
 namespace {
 static const int64 kDownloadRequestTimeoutMs = 3000;
 }  // namespace
 
 namespace safe_browsing {
 
 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) {
   DCHECK(IsBinaryFile(file));
   if (file.MatchesExtension(FILE_PATH_LITERAL(".apk")))
     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;
 }
 
 // List of extensions for which we track some UMA stats.
 enum MaliciousExtensionType {
   EXTENSION_EXE,
   EXTENSION_MSI,
   EXTENSION_CAB,
   EXTENSION_SYS,
   EXTENSION_SCR,
@@ skipping 57 matching lines @@
   DOWNLOAD_HASH_CHECKS_TOTAL,
   DOWNLOAD_HASH_CHECKS_MALWARE,
 
   // Memory space for histograms is determined by the max.
   // ALWAYS ADD NEW VALUES BEFORE THIS ONE.
   DOWNLOAD_CHECKS_MAX
 };
 }  // namespace
 
 DownloadProtectionService::DownloadInfo::DownloadInfo()
-    : total_bytes(0), user_initiated(false) {}
+    : total_bytes(0), user_initiated(false), zipped_executable(false) {}
 
 DownloadProtectionService::DownloadInfo::~DownloadInfo() {}
 
 std::string DownloadProtectionService::DownloadInfo::DebugString() const {
   std::string chain;
   for (size_t i = 0; i < download_url_chain.size(); ++i) {
     chain += download_url_chain[i].spec();
     if (i < download_url_chain.size() - 1) {
       chain += " -> ";
     }
@@ skipping 317 matching lines @@
 
  private:
   friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
   friend class base::DeleteHelper<CheckClientDownloadRequest>;
 
   virtual ~CheckClientDownloadRequest() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   }
 
   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) {
       VLOG(2) << "Downloaded a signed binary: " << info_.local_file.value();
     } else {
       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() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
     DownloadCheckResultReason reason = REASON_MAX;
     if (!sb_service_.get()) {
       reason = REASON_SB_DISABLED;
     } else {
       for (size_t i = 0; i < info_.download_url_chain.size(); ++i) {
         const GURL& url = info_.download_url_chain[i];
@@ skipping 239 matching lines @@
   // Currently, the UI only works on Windows.  On Linux and Mac we still
   // want to show the dangerous file type warning if the file is possibly
   // dangerous which means we have to always return false here.
 #if defined(OS_WIN)
   DownloadCheckResultReason reason = REASON_MAX;
   ClientDownloadRequest::DownloadType type =
       ClientDownloadRequest::WIN_EXECUTABLE;
   return (CheckClientDownloadRequest::IsSupportedDownload(info,
                                                           &reason,
                                                           &type) &&
-          ClientDownloadRequest::WIN_EXECUTABLE == type);
+          ClientDownloadRequest::WIN_EXECUTABLE == type ||
+          ClientDownloadRequest::ZIPPED_WIN_EXECUTABLE == type);
 #else
   return false;
 #endif
 }
 
 void DownloadProtectionService::CancelPendingRequests() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   for (std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
            download_requests_.begin();
        it != download_requests_.end();) {
@@ skipping 93 matching lines @@
 
   std::string issuer_fp = base::HexEncode(issuer.fingerprint().data,
                                           sizeof(issuer.fingerprint().data));
   for (std::set<std::string>::iterator it = paths_to_check.begin();
        it != paths_to_check.end(); ++it) {
     whitelist_strings->push_back("cert/" + issuer_fp + *it);
   }
 }
 
 }  // namespace safe_browsing