Collect some histograms about signed binary downloads.

chrome/browser/safe_browsing/download_protection_service.cc

 // Copyright (c) 2011 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/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
+#include "base/string_util.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/safe_browsing/signature_util.h"
 #include "chrome/common/net/http_return.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 #include "content/browser/browser_thread.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 
 namespace safe_browsing {
 
 const char DownloadProtectionService::kDownloadRequestUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/download";
 
+namespace {
+bool IsBinaryFile(const FilePath& file) {
+  return (file.MatchesExtension(FILE_PATH_LITERAL(".exe")) ||
+          file.MatchesExtension(FILE_PATH_LITERAL(".cab")) ||
+          file.MatchesExtension(FILE_PATH_LITERAL(".msi")));
+}
+}  // namespace
+
 DownloadProtectionService::DownloadInfo::DownloadInfo()
     : total_bytes(0), user_initiated(false) {}
 
 DownloadProtectionService::DownloadInfo::~DownloadInfo() {}
 
 DownloadProtectionService::DownloadProtectionService(
     SafeBrowsingService* sb_service,
     net::URLRequestContextGetter* request_context_getter)
     : sb_service_(sb_service),
       request_context_getter_(request_context_getter),
       enabled_(false) {}
 
 DownloadProtectionService::~DownloadProtectionService() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   STLDeleteContainerPairFirstPointers(download_requests_.begin(),
                                       download_requests_.end());
   download_requests_.clear();
 }
 
 void DownloadProtectionService::SetEnabled(bool enabled) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&DownloadProtectionService::SetEnabledOnIOThread,
-                 this, enabled));
+                 base::Unretained(this), enabled));

[noelutz, 2011/10/22 00:41:46]

I just want to make sure I follow the code here.  We're assuming that we will
only delete this service object once the IO thread is going away and no-one will
flush the IO message loop?

 }
 
 void DownloadProtectionService::SetEnabledOnIOThread(bool enabled) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (enabled == enabled_) {
     return;
   }
   enabled_ = enabled;
   if (!enabled_) {
     for (std::map<const URLFetcher*, CheckDownloadCallback>::iterator it =
@@ skipping 32 matching lines @@
       // For now no matter what we'll always say the download is safe.
       // TODO(noelutz): Parse the response body to see exactly what's going on.
       reason = REASON_INVALID_RESPONSE_PROTO;
     } else {
       reason = REASON_SERVER_PING_FAILED;
     }
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&DownloadProtectionService::EndCheckClientDownload,
-                   this, SAFE, reason, callback));
+                   base::Unretained(this), SAFE, reason, callback));
   } else {
     NOTREACHED();
   }
 }
 
 bool DownloadProtectionService::CheckClientDownload(
     const DownloadInfo& info,
     const CheckDownloadCallback& callback) {
   // TODO(noelutz): implement some cache to make sure we don't issue the same
   // request over and over again if a user downloads the same binary multiple
   // times.
   if (info.download_url_chain.empty()) {
     RecordStats(REASON_INVALID_URL);
     return true;
   }
   const GURL& final_url = info.download_url_chain.back();
   if (!final_url.is_valid() || final_url.is_empty() ||
       !final_url.SchemeIs("http")) {
     RecordStats(REASON_INVALID_URL);
     return true;  // For now we only support HTTP download URLs.
   }
+
+  if (!IsBinaryFile(info.local_file)) {
+    RecordStats(REASON_NOT_BINARY_FILE);
+    return true;
+  }
+
+  // Compute features from the file contents. Note that we record histograms
+  // based on the result, so this runs regardless of whether the pingbacks are
+  // enabled.  Since we do blocking I/O, this happens on the file thread.
+  BrowserThread::PostTask(
+      BrowserThread::FILE,
+      FROM_HERE,
+      base::Bind(&DownloadProtectionService::ExtractFileFeatures,
+                 base::Unretained(this), info, enabled_, callback));

[noelutz, 2011/10/22 00:41:46]

I think there is a race condition here.  You access enabled_ on the UI thread
but it's written and set on the IO thread.  Also, you're not really using
pingback_enabled anywhere as far as I can tell.  Could you just read enabled_ in
StartCheckClientDownload which runs on the IO thread?

+  return false;
+}
+
+void DownloadProtectionService::ExtractFileFeatures(
+    const DownloadInfo& info,
+    bool pingback_enabled,
+    const CheckDownloadCallback& callback) {
+  bool is_signed;
+  if (safe_browsing::signature_util::IsSigned(info.local_file)) {
+    VLOG(2) << "Downloaded a signed binary: " << info.local_file.value();
+    is_signed = true;
+  } else {
+    VLOG(2) << "Downloaded an unsigned binary: " << info.local_file.value();
+    is_signed = false;
+  }
+  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(&DownloadProtectionService::StartCheckClientDownload,
-                 this, info, callback));
-  return false;
+                 base::Unretained(this), info, pingback_enabled, callback));
 }
 
 void DownloadProtectionService::StartCheckClientDownload(
     const DownloadInfo& info,
+    bool pingback_enabled,
     const CheckDownloadCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (!enabled_ || !sb_service_.get()) {
-    // This is a hard fail.  We won't even call the callback in this case.
-    RecordStats(REASON_SB_DISABLED);
+  DownloadCheckResultReason reason = REASON_MAX;
+  if (!enabled_ || !sb_service_) {
+    reason = REASON_SB_DISABLED;
+    RecordStats(reason);
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&DownloadProtectionService::EndCheckClientDownload,
+                   base::Unretained(this), SAFE, reason, callback));

[noelutz, 2011/10/22 00:41:46]

Will this always work?  What if the destructor is called before
EndCheckClientDownload?

     return;
   }
-  DownloadCheckResultReason reason = REASON_MAX;
   for (size_t i = 0; i < info.download_url_chain.size(); ++i) {
     if (sb_service_->MatchDownloadWhitelistUrl(info.download_url_chain[i])) {
       reason = REASON_WHITELISTED_URL;
       break;
     }
   }
   if (sb_service_->MatchDownloadWhitelistUrl(info.referrer_url)) {
     reason = REASON_WHITELISTED_REFERRER;
   }
   // TODO(noelutz): check signature and CA against whitelist.
@@ skipping 19 matching lines @@
   if (!request.SerializeToString(&request_data)) {
     reason = REASON_INVALID_REQUEST_PROTO;
   }
 
   if (reason != REASON_MAX) {
     // We stop here because the download is considered safe.
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         base::Bind(&DownloadProtectionService::EndCheckClientDownload,
-                   this, SAFE, reason, callback));
+                   base::Unretained(this), SAFE, reason, callback));

[noelutz, 2011/10/22 00:41:46]

same question as above.

     return;
   }
 
   URLFetcher* fetcher = URLFetcher::Create(0 /* ID used for testing */,
                                            GURL(kDownloadRequestUrl),
                                            URLFetcher::POST,
                                            this);
   download_requests_[fetcher] = callback;
   fetcher->set_load_flags(net::LOAD_DISABLE_CACHE);
   fetcher->set_request_context(request_context_getter_.get());
   fetcher->set_upload_data("application/octet-stream", request_data);
   fetcher->Start();
 }
 
 void DownloadProtectionService::EndCheckClientDownload(
     DownloadCheckResult result,
     DownloadCheckResultReason reason,
     const CheckDownloadCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   RecordStats(reason);
   callback.Run(result);
 }
 
 void DownloadProtectionService::RecordStats(DownloadCheckResultReason reason) {
   UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
                             reason,
                             REASON_MAX);
 }
 }  // namespace safe_browsing