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) {

[mattm, 2011/10/19 18:54:12]

"Binary" doesn't seem like quite the right term.. not coming up with any
brilliant alternatives though

[Brian Ryner, 2011/10/19 19:20:11]

I could use "executable" if you think that's better.

+  FilePath::StringType extension = file.Extension();

[noelutz, 2011/10/19 16:12:49]

Maybe wrap this in StringToLowerASCII to make sure the comparison below does the
right thing?

[Brian Ryner, 2011/10/19 18:06:59]

I ended up replacing this with calls to FilePath::MatchesExtension(), which is
case insensitive.

+  return (EndsWith(extension, FILE_PATH_LITERAL("exe"), false) ||
+          EndsWith(extension, FILE_PATH_LITERAL("cab"), false) ||
+          EndsWith(extension, FILE_PATH_LITERAL("msi"), false));
+}
+}  // namespace
+
 DownloadProtectionService::DownloadInfo::DownloadInfo()
     : total_bytes(0), user_initiated(false) {}
 
 DownloadProtectionService::DownloadInfo::~DownloadInfo() {}
 
 DownloadProtectionService::DownloadProtectionService(
-    SafeBrowsingService* sb_service,
+    const base::WeakPtr<SafeBrowsingService>& sb_service,

[mattm, 2011/10/19 18:54:12]

Hmm, still not sure about these WeakPtr's but I'm not sure this is safe. 
weak_ptr.h says a WeakPtr should only be derefed on the thread that created the
WeakPtr, which I think is UI for this, but it's derefed on IO thread later on.

[Brian Ryner, 2011/10/19 19:20:11]

Hm, good point.  Maybe we need to AddRef the SafeBrowsingService during the time
we're executing on the IO thread?  See my comment below about the rationale for
the weak pointer.

     net::URLRequestContextGetter* request_context_getter)
     : sb_service_(sb_service),
       request_context_getter_(request_context_getter),
       enabled_(false) {}
 
 DownloadProtectionService::~DownloadProtectionService() {
   STLDeleteContainerPairFirstPointers(download_requests_.begin(),
                                       download_requests_.end());
   download_requests_.clear();
 }
@@ skipping 72 matching lines @@
   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,
+                 this, info, callback));
+  return false;
+}
+
+void DownloadProtectionService::ExtractFileFeatures(
+    const DownloadInfo& info,
+    const CheckDownloadCallback& callback) {
+  if (safe_browsing::signature_util::IsSigned(info.local_file)) {
+    VLOG(2) << "Downloaded a signed binary: " << info.local_file.value();
+    UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedBinaryDownload", 1);

[mattm, 2011/10/19 18:54:12]

Could use UMA_HISTOGRAM_BOOLEAN to get them both in the same histogram.

[Brian Ryner, 2011/10/19 19:20:11]

Done.

+  } else {
+    VLOG(2) << "Downloaded an unsigned binary: " << info.local_file.value();
+    UMA_HISTOGRAM_COUNTS("SBClientDownload.UnsignedBinaryDownload", 1);
+  }
+
   // 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;
 }
 
 void DownloadProtectionService::StartCheckClientDownload(
     const DownloadInfo& info,
     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);

[noelutz, 2011/10/19 16:12:49]

Mhh.  We might want to treat those two cases separately.  If enabled_ is false
we might still want to call the callback because the download manager might be
waiting for a response?

[Brian Ryner, 2011/10/19 18:06:59]

Is theree a reason we specifically want to avoid calling the callback is
enabled_ is true but sb_service_ is null?  If not, it's easiest to just change
the behavior for both cases.

[noelutz, 2011/10/19 18:41:14]

Good point.  We can always call the callback with SAFE here.

     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)) {
@@ skipping 52 matching lines @@
   RecordStats(reason);
   callback.Run(result);
 }
 
 void DownloadProtectionService::RecordStats(DownloadCheckResultReason reason) {
   UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
                             reason,
                             REASON_MAX);
 }
 }  // namespace safe_browsing