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 "content/browser/download/download_item.h"
+#include "content/public/common/url_fetcher_delegate.h"
 #include "content/common/net/url_fetcher.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() {
-  STLDeleteContainerPairFirstPointers(download_requests_.begin(),
-                                      download_requests_.end());
-  download_requests_.clear();
+// static
+DownloadProtectionService::DownloadInfo
+DownloadProtectionService::DownloadInfo::FromDownloadItem(
+    const DownloadItem& item) {
+  DownloadInfo download_info;
+  download_info.local_file = item.full_path();
+  download_info.download_url_chain = item.url_chain();
+  download_info.referrer_url = item.referrer_url();
+  // TODO(bryner): Fill in the hash (we shouldn't compute it again)
+  download_info.total_bytes = item.total_bytes();
+  // TODO(bryner): Populate user_initiated
+  return download_info;
 }
 
-void DownloadProtectionService::SetEnabled(bool enabled) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&DownloadProtectionService::SetEnabledOnIOThread,
-                 this, enabled));
-}
+class DownloadProtectionService::CheckClientDownloadRequest
+    : public base::RefCountedThreadSafe<
+          DownloadProtectionService::CheckClientDownloadRequest,
+          BrowserThread::DeleteOnUIThread>,
+      public content::URLFetcherDelegate {
+ public:
+  CheckClientDownloadRequest(const DownloadInfo& info,
+                             const CheckDownloadCallback& callback,
+                             DownloadProtectionService* service,
+                             SafeBrowsingService* sb_service)
+      : info_(info),
+        callback_(callback),
+        service_(service),
+        sb_service_(sb_service),
+        pingback_enabled_(service_->enabled()),
+        canceled_(false) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  }
 
-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 =
-             download_requests_.begin();
-         it != download_requests_.end(); ++it) {
-      it->second.Run(SAFE);
-    }
-    STLDeleteContainerPairFirstPointers(download_requests_.begin(),
-                                        download_requests_.end());
-    download_requests_.clear();
-  }
-}
-
-void DownloadProtectionService::OnURLFetchComplete(const URLFetcher* source) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  scoped_ptr<const URLFetcher> s(source);  // will delete the URLFetcher object.
-  if (download_requests_.find(source) != download_requests_.end()) {
-    CheckDownloadCallback callback = download_requests_[source];
-    download_requests_.erase(source);
-    if (!enabled_) {
-      // SafeBrowsing got disabled.  We can't do anything.  Note: the request
-      // object will be deleted.
-      RecordStats(REASON_SB_DISABLED);
+  void Start() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    // 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);
+      PostFinishTask(SAFE);
       return;
     }
+    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);
+      PostFinishTask(SAFE);
+      return;  // For now we only support HTTP download URLs.
+    }
+
+    if (!IsBinaryFile(info_.local_file)) {
+      RecordStats(REASON_NOT_BINARY_FILE);
+      PostFinishTask(SAFE);
+      return;
+    }
+
+    // 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(&CheckClientDownloadRequest::ExtractFileFeatures, this));
+  }
+
+  // Canceling a request will cause us to always report the result as SAFE.
+  void Cancel() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    canceled_ = true;
+  }
+
+  // From the content::URLFetcher interface.
+  virtual void OnURLFetchComplete(const URLFetcher* source) OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    DCHECK_EQ(source, fetcher_.get());
+    VLOG(2) << "Received a response for URL: "
+            << info_.download_url_chain.back() << ": success="
+            << source->status().is_success() << " response_code="
+            << source->response_code();
     DownloadCheckResultReason reason = REASON_MAX;
     reason = REASON_SERVER_PING_FAILED;
     if (source->status().is_success() &&
         RC_REQUEST_OK == source->response_code()) {
       std::string data;
       source->GetResponseAsString(&data);
       if (data.size() > 0) {
         // 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;
       }
     }
+
+    if (reason != REASON_MAX) {
+      RecordStats(reason);
+    }
+    FinishRequest(SAFE);
+  }
+
+ private:
+  friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
+  friend class DeleteTask<CheckClientDownloadRequest>;
+
+  virtual ~CheckClientDownloadRequest() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  }
+
+  void ExtractFileFeatures() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    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(&CheckClientDownloadRequest::CheckWhitelists, this));
+  }
+
+  void CheckWhitelists() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    DownloadCheckResultReason reason = REASON_MAX;
+    if (!pingback_enabled_ || !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];
+        if (url.is_valid() && sb_service_->MatchDownloadWhitelistUrl(url)) {
+          reason = REASON_WHITELISTED_URL;
+          break;
+        }
+      }
+      if (info_.referrer_url.is_valid() &&
+          sb_service_->MatchDownloadWhitelistUrl(info_.referrer_url)) {
+        reason = REASON_WHITELISTED_REFERRER;
+      }
+    }
+    if (reason != REASON_MAX) {
+      RecordStats(reason);
+      PostFinishTask(SAFE);
+      return;
+    }
+
+    // TODO(noelutz): check signature and CA against whitelist.
+
+    // The URLFetcher is owned by the UI thread, so post a message to
+    // start the pigback.
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
-        base::Bind(&DownloadProtectionService::EndCheckClientDownload,
-                   this, SAFE, reason, callback));
-  } else {
-    NOTREACHED();
+        base::Bind(&CheckClientDownloadRequest::SendRequest, this));
+  }
+
+  void SendRequest() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    ClientDownloadRequest request;
+    request.set_url(info_.download_url_chain.back().spec());
+    request.mutable_digests()->set_sha256(info_.sha256_hash);
+    request.set_length(info_.total_bytes);
+    for (size_t i = 0; i < info_.download_url_chain.size(); ++i) {
+      ClientDownloadRequest::Resource* resource = request.add_resources();
+      resource->set_url(info_.download_url_chain[i].spec());
+      if (i == info_.download_url_chain.size() - 1) {
+        // The last URL in the chain is the download URL.
+        resource->set_type(ClientDownloadRequest::DOWNLOAD_URL);
+        resource->set_referrer(info_.referrer_url.spec());
+      } else {
+        resource->set_type(ClientDownloadRequest::DOWNLOAD_REDIRECT);
+      }
+      // TODO(noelutz): fill out the remote IP addresses.
+    }
+    request.set_user_initiated(info_.user_initiated);
+    std::string request_data;
+    if (!request.SerializeToString(&request_data)) {
+      RecordStats(REASON_INVALID_REQUEST_PROTO);
+      FinishRequest(SAFE);
+      return;
+    }
+
+    VLOG(2) << "Sending a request for URL: "
+            << info_.download_url_chain.back();
+    fetcher_.reset(URLFetcher::Create(0 /* ID used for testing */,
+                                      GURL(kDownloadRequestUrl),
+                                      URLFetcher::POST,
+                                      this));
+    fetcher_->set_load_flags(net::LOAD_DISABLE_CACHE);
+    fetcher_->set_request_context(service_->request_context_getter_.get());

[noelutz, 2011/10/25 21:43:50]

Check if the request is cancelled before accessing service_?

[Brian Ryner, 2011/10/25 22:03:07]

Done.

+    fetcher_->set_upload_data("application/octet-stream", request_data);
+    fetcher_->Start();
+  }
+
+  void PostFinishTask(DownloadCheckResult result) {
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&CheckClientDownloadRequest::FinishRequest, this, result));
+  }
+
+  void FinishRequest(DownloadCheckResult result) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    callback_.Run(canceled_ ? SAFE : result);
+    service_->RequestFinished(this);

[noelutz, 2011/10/25 21:43:50]

Same here.  I'm pretty sure it's possible for service_ to be deleted before
sb_service_ is going away.  See comment below.  I think you can simply cancel
all requests when DPS goes away and check canceled here.

[Brian Ryner, 2011/10/25 22:03:07]

Ah, I guess you mean in the ShutDown() case?  Done.

[noelutz, 2011/10/25 22:13:42]

Yeah the ShutDown case.

+  }
+
+  void RecordStats(DownloadCheckResultReason reason) {
+    UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
+                              reason,
+                              REASON_MAX);
+  }
+
+  DownloadInfo info_;
+  CheckDownloadCallback callback_;
+  DownloadProtectionService* service_;
+  scoped_refptr<SafeBrowsingService> sb_service_;
+  bool pingback_enabled_;
+  bool canceled_;
+  scoped_ptr<URLFetcher> fetcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(CheckClientDownloadRequest);
+};
+
+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::UI));
+  // The SafeBrowsingService owns this class, and any pending request will
+  // have an outstanding reference to the SafeBrowsingService.  Therefore,
+  // at this point there must not be any active requests.
+  DCHECK(download_requests_.empty());

[noelutz, 2011/10/25 21:43:50]

I think you need to cancel requests here.

[Brian Ryner, 2011/10/25 22:03:07]

So normally if we cancel a request, we still rely on the request calling
RequestFinished for the DPS to release its reference.  Are you suggesting that
we change this so that DPS always releases its reference when it Cancels a
request?

[noelutz, 2011/10/25 22:13:42]

Not necessarily but I believe there is a bug here.  In the ShutDown() case this
destructor will be called without SetEnabled being called first (I believe). 
However, some request objects might still live on.  Once they access their
service_ pointer everything will blow up ;).  If you cancel all requests first
you'll be safe assuming you add the checks mentioned above.

+}
+
+void DownloadProtectionService::SetEnabled(bool enabled) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (enabled == enabled_) {
+    return;
+  }
+  enabled_ = enabled;
+  if (!enabled_) {
+    for (std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
+             download_requests_.begin();
+         it != download_requests_.end(); ++it) {
+      (*it)->Cancel();
+    }
   }
 }
 
-bool DownloadProtectionService::CheckClientDownload(
-    const DownloadInfo& info,
+void DownloadProtectionService::CheckClientDownload(
+    const DownloadProtectionService::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.
-  }
-  // 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;
+  scoped_refptr<CheckClientDownloadRequest> request(
+      new CheckClientDownloadRequest(info, callback, this, sb_service_));
+  download_requests_.insert(request);
+  request->Start();
 }
 
-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);
-    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.
-
-  ClientDownloadRequest request;
-  request.set_url(info.download_url_chain.back().spec());
-  request.mutable_digests()->set_sha256(info.sha256_hash);
-  request.set_length(info.total_bytes);
-  for (size_t i = 0; i < info.download_url_chain.size(); ++i) {
-    ClientDownloadRequest::Resource* resource = request.add_resources();
-    resource->set_url(info.download_url_chain[i].spec());
-    if (i == info.download_url_chain.size() - 1) {
-      // The last URL in the chain is the download URL.
-      resource->set_type(ClientDownloadRequest::DOWNLOAD_URL);
-      resource->set_referrer(info.referrer_url.spec());
-    } else {
-      resource->set_type(ClientDownloadRequest::DOWNLOAD_REDIRECT);
-    }
-    // TODO(noelutz): fill out the remote IP addresses.
-  }
-  request.set_user_initiated(info.user_initiated);
-  std::string request_data;
-  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));
-    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::RequestFinished(
+    CheckClientDownloadRequest* request) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
+      download_requests_.find(request);
+  DCHECK(it != download_requests_.end());
+  download_requests_.erase(*it);
 }
 
-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