Collect some histograms about signed binary downloads.

This hooks up the DownloadProtectionService to run after a download finishes.  For now, this does not send a download pingback since the flag defaults to off.


TEST=DownloadProtectionServiceTest
BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107528

Review URL: http://codereview.chromium.org/8345033

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107674 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -156,8 +156,27 @@ bool ChromeDownloadManagerDelegate::ShouldOpenDownload(DownloadItem* item) {
 }
 
 bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
-  if (!IsExtensionDownload(item))
+  if (!IsExtensionDownload(item)) {
+#if defined(ENABLE_SAFE_BROWSING)
+    // Begin the safe browsing download protection check.
+    SafeBrowsingService* sb_service =
+        g_browser_process->safe_browsing_service();
+    if (sb_service && sb_service->download_protection_service() &&
+        profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+      using safe_browsing::DownloadProtectionService;
+      sb_service->download_protection_service()->CheckClientDownload(
+          DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
+          base::Bind(
+              &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
+              this, item->id()));
+      // For now, we won't delay the download for this.
+    }
+#else
+    // Assume safe.
+#endif
+
     return true;
+  }
 
   scoped_refptr<CrxInstaller> crx_installer =
       download_crx_util::OpenChromeExtension(profile_, *item);
@@ -227,6 +246,12 @@ void ChromeDownloadManagerDelegate::UpdatePathForItemInPersistentStore(
   download_history_->UpdateDownloadPath(item, new_path);
 }
 
+void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
+    int32 download_id,
+    safe_browsing::DownloadProtectionService::DownloadCheckResult result) {
+  // TODO(bryner): notify the user based on this result
+}
+
 void ChromeDownloadManagerDelegate::RemoveItemFromPersistentStore(
     DownloadItem* item) {
   download_history_->RemoveEntry(item);

chrome/browser/download/chrome_download_manager_delegate.h

@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/task.h"
+#include "chrome/browser/safe_browsing/download_protection_service.h"
 #include "content/public/browser/download_manager_delegate.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -134,6 +135,11 @@ class ChromeDownloadManagerDelegate
   // service.
   void CheckDownloadHashDone(int32 download_id, bool is_dangerous_hash);
 
+  // Callback function after the DownloadProtectionService completes.
+  void CheckClientDownloadDone(
+      int32 download_id,
+      safe_browsing::DownloadProtectionService::DownloadCheckResult result);
+
   Profile* profile_;
   scoped_ptr<DownloadPrefs> download_prefs_;
   scoped_ptr<DownloadHistory> download_history_;

chrome/browser/safe_browsing/client_side_detection_host_unittest.cc

@@ -170,11 +170,13 @@ class ClientSideDetectionHostTest : public TabContentsWrapperTestHarness {
   }
 
   virtual void TearDown() {
-    // Delete the host object on the UI thread.
+    // Delete the host object on the UI thread and release the
+    // SafeBrowsingService.
     BrowserThread::PostTask(
         BrowserThread::UI,
         FROM_HERE,
         new DeleteTask<ClientSideDetectionHost>(csd_host_.release()));
+    sb_service_ = NULL;
     message_loop_.RunAllPending();
     TabContentsWrapperTestHarness::TearDown();
     io_thread_.reset();

chrome/browser/safe_browsing/download_protection_service.h

@@ -9,36 +9,34 @@
 #define CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_SERVICE_H_
 #pragma once
 
-#include <map>
+#include <set>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/file_path.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/time.h"
-#include "content/public/common/url_fetcher_delegate.h"
 #include "googleurl/src/gurl.h"
 
 namespace net {
 class URLRequestContextGetter;
 class URLRequestStatus;
 }  // namespace net
+class DownloadItem;
 class SafeBrowsingService;
 
 namespace safe_browsing {
 
 // This class provides an asynchronous API to check whether a particular
 // client download is malicious or not.
-class DownloadProtectionService
-    : public base::RefCountedThreadSafe<DownloadProtectionService>,
-      public content::URLFetcherDelegate {
+class DownloadProtectionService {
  public:
-  // TODO(noelutz): we're missing some fields here: filename to get
-  // the signature, server IPs, tab URL redirect chain, ...
+  // TODO(noelutz): we're missing some fields here: server IPs,
+  // tab URL redirect chain, ...
   struct DownloadInfo {
+    FilePath local_file;
     std::vector<GURL> download_url_chain;
     GURL referrer_url;
     std::string sha256_hash;
@@ -46,6 +44,9 @@ class DownloadProtectionService
     bool user_initiated;
     DownloadInfo();
     ~DownloadInfo();
+
+    // Creates a DownloadInfo from a DownloadItem object.
+    static DownloadInfo FromDownloadItem(const DownloadItem& item);
   };
 
   enum DownloadCheckResult {
@@ -59,32 +60,25 @@ class DownloadProtectionService
   typedef base::Callback<void(DownloadCheckResult)> CheckDownloadCallback;
 
   // Creates a download service.  The service is initially disabled.  You need
-  // to call SetEnabled() to start it.  We keep scoped references to both of
-  // these objects.
+  // to call SetEnabled() to start it.  |sb_service| owns this object; we
+  // keep a reference to |request_context_getter|.
   DownloadProtectionService(
       SafeBrowsingService* sb_service,
       net::URLRequestContextGetter* request_context_getter);
 
-  // From the content::URLFetcherDelegate interface.
-  virtual void OnURLFetchComplete(const content::URLFetcher* source) OVERRIDE;
-
-  // Checks whether the given client download is likely to be
-  // malicious or not.  If this method returns true it means the
-  // download is safe or we're unable to tell whether it is safe or
-  // not.  In this case the callback will never be called.  If this
-  // method returns false we will asynchronously check whether the
-  // download is safe and call the callback when we have the response.
-  // This method should be called on the UI thread.  The callback will
-  // be called on the UI thread and will always be called asynchronously.
-  virtual bool CheckClientDownload(const DownloadInfo& info,
+  virtual ~DownloadProtectionService();
+
+  // Checks whether the given client download is likely to be malicious or not.
+  // The result is delivered asynchronously via the given callback.  This
+  // method must be called on the UI thread, and the callback will also be
+  // invoked on the UI thread.
+  virtual void CheckClientDownload(const DownloadInfo& info,
                                    const CheckDownloadCallback& callback);
 
   // Enables or disables the service.  This is usually called by the
   // SafeBrowsingService, which tracks whether any profile uses these services
-  // at all.  Disabling cancels any pending requests; existing requests will
-  // have their callbacks called with "SAFE" results.  Note: SetEnabled() is
-  // asynchronous because this method is called on the UI thread but most
-  // everything else happens on the IO thread.
+  // at all.  Disabling causes any pending and future requests to have their
+  // callbacks called with "SAFE" results.
   void SetEnabled(bool enabled);
 
   bool enabled() const {
@@ -102,13 +96,12 @@ class DownloadProtectionService
     REASON_INVALID_REQUEST_PROTO,
     REASON_SERVER_PING_FAILED,
     REASON_INVALID_RESPONSE_PROTO,
+    REASON_NOT_BINARY_FILE,
     REASON_MAX  // Always add new values before this one.
   };
 
-  virtual ~DownloadProtectionService();
-
  private:
-  friend class base::RefCountedThreadSafe<DownloadProtectionService>;
+  class CheckClientDownloadRequest;  // Per-request state
   friend class DownloadProtectionServiceTest;
   FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
                            CheckClientDownloadValidateRequest);
@@ -119,25 +112,20 @@ class DownloadProtectionService
 
   static const char kDownloadRequestUrl[];
 
-  // Same as above but this method is called on the IO thread after we have
-  // done some basic checks to see whether the download is definitely not
-  // safe.
-  void StartCheckClientDownload(const DownloadInfo& info,
-                                const CheckDownloadCallback& callback);
-
-  // This function must run on the UI thread and will invoke the callback
-  // with the given result.
-  void EndCheckClientDownload(DownloadCheckResult result,
-                              DownloadCheckResultReason reason,
-                              const CheckDownloadCallback& callback);
+  // Cancels all requests in |download_requests_|, and empties it, releasing
+  // the references to the requests.
+  void CancelPendingRequests();
 
-  void RecordStats(DownloadCheckResultReason reason);
+  // Called by a CheckClientDownloadRequest instance when it finishes, to
+  // remove it from |download_requests_|.
+  void RequestFinished(CheckClientDownloadRequest* request);
 
-  // SetEnabled(bool) calls this method on the IO thread.
-  void SetEnabledOnIOThread(bool enableed);
+  static void FillDownloadInfo(const DownloadItem& item,
+                               DownloadInfo* download_info);
 
-  // This pointer may be NULL if SafeBrowsing is disabled.
-  scoped_refptr<SafeBrowsingService> sb_service_;
+  // This pointer may be NULL if SafeBrowsing is disabled. The
+  // SafeBrowsingService owns us, so we don't need to hold a reference to it.
+  SafeBrowsingService* sb_service_;
 
   // The context we use to issue network requests.
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
@@ -145,9 +133,7 @@ class DownloadProtectionService
   // Map of client download request to the corresponding callback that
   // has to be invoked when the request is done.  This map contains all
   // pending server requests.
-  typedef std::map<const content::URLFetcher*, CheckDownloadCallback>
-      DownloadRequests;
-  DownloadRequests download_requests_;
+  std::set<scoped_refptr<CheckClientDownloadRequest> > download_requests_;
 
   // Keeps track of the state of the service.
   bool enabled_;

chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc

@@ -90,6 +90,12 @@ class SafeBrowsingBlockingPageTest : public ChromeRenderViewHostTestHarness,
     ResetUserResponse();
   }
 
+  virtual void TearDown() {
+    // Release the SafeBrowsingService before the BrowserThreads are destroyed.
+    service_ = NULL;
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
   // SafeBrowsingService::Client implementation.
   virtual void OnUrlCheckResult(const GURL& url,
                                 SafeBrowsingService::UrlCheckResult result) {

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -30,7 +30,6 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
-#include "content/browser/browser_thread.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
@@ -175,12 +174,9 @@ SafeBrowsingService::SafeBrowsingService()
         safe_browsing::ClientSideDetectionService::Create(
             g_browser_process->system_request_context()));
   }
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableImprovedDownloadProtection)) {
-    download_service_ = new safe_browsing::DownloadProtectionService(
-        this,
-        g_browser_process->system_request_context());
-  }
+  download_service_.reset(new safe_browsing::DownloadProtectionService(
+      this,
+      g_browser_process->system_request_context()));
 #endif
 }
 
@@ -216,20 +212,12 @@ void SafeBrowsingService::Initialize() {
 }
 
 void SafeBrowsingService::ShutDown() {
-  if (download_service_.get()) {
-    // Disabling the download service first will ensure that it is
-    // disabled before the SafeBrowsingService object becomes invalid.  The
-    // download service might stay around for a bit since it's
-    // ref-counted but it won't do any harm because it will be
-    // disabled.
-    download_service_->SetEnabled(false);
-    download_service_ = NULL;
-  }
   Stop();
   // The IO thread is going away, so make sure the ClientSideDetectionService
   // dtor executes now since it may call the dtor of URLFetcher which relies
   // on it.
   csd_service_.reset();
+  download_service_.reset();
 }
 
 bool SafeBrowsingService::CanCheckUrl(const GURL& url) const {
@@ -1368,6 +1356,9 @@ void SafeBrowsingService::RefreshState() {
 
   if (csd_service_.get())
     csd_service_->SetEnabledAndRefreshState(enable);
-  if (download_service_.get())
-    download_service_->SetEnabled(enable);
+  if (download_service_.get()) {
+    download_service_->SetEnabled(
+        enable && CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kEnableImprovedDownloadProtection));
+  }
 }

chrome/browser/safe_browsing/safe_browsing_service.h

@@ -23,6 +23,7 @@
 #include "base/task.h"
 #include "base/time.h"
 #include "chrome/browser/safe_browsing/safe_browsing_util.h"
+#include "content/browser/browser_thread.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "googleurl/src/gurl.h"
@@ -49,7 +50,8 @@ class DownloadProtectionService;
 
 // Construction needs to happen on the main thread.
 class SafeBrowsingService
-    : public base::RefCountedThreadSafe<SafeBrowsingService>,
+    : public base::RefCountedThreadSafe<SafeBrowsingService,
+                                        BrowserThread::DeleteOnUIThread>,
       public content::NotificationObserver {
  public:
   class Client;
@@ -267,6 +269,8 @@ class SafeBrowsingService
     return csd_service_.get();
   }
 
+  // The DownloadProtectionService is not valid after the SafeBrowsingService
+  // is destroyed.
   safe_browsing::DownloadProtectionService*
       download_protection_service() const {
     return download_service_.get();
@@ -325,7 +329,8 @@ class SafeBrowsingService
     base::TimeTicks start;  // When check was queued.
   };
 
-  friend class base::RefCountedThreadSafe<SafeBrowsingService>;
+  friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
+  friend class DeleteTask<SafeBrowsingService>;
   friend class SafeBrowsingServiceTest;
 
   // Called to initialize objects that are used on the io_thread.
@@ -556,7 +561,7 @@ class SafeBrowsingService
 
   // The DownloadProtectionService is managed by the SafeBrowsingService,
   // since its running state and lifecycle depends on SafeBrowsingService's.
-  scoped_refptr<safe_browsing::DownloadProtectionService> download_service_;
+  scoped_ptr<safe_browsing::DownloadProtectionService> download_service_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingService);
 };

chrome/browser/safe_browsing/signature_util.h

+// 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.
+//
+// Utility functions to check executable signatures for malicious binary
+// detection.  Each platform has its own implementation of this class.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_SIGNATURE_UTIL_H_
+#define CHROME_BROWSER_SAFE_BROWSING_SIGNATURE_UTIL_H_
+#pragma once
+
+class FilePath;
+
+namespace safe_browsing {
+namespace signature_util {
+
+// Returns true if the given file path contains a signature.  No checks are
+// performed as to the validity of the signature.
+bool IsSigned(const FilePath& file_path);
+
+}  // namespace signature_util
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_SIGNATURE_UTIL_H_

chrome/browser/safe_browsing/signature_util_posix.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.
+//
+// This is a stub for the code signing utilities on Mac and Linux.
+// It should eventually be replaced with a real implementation.
+
+#include "chrome/browser/safe_browsing/signature_util.h"
+
+namespace safe_browsing {
+namespace signature_util {
+
+bool IsSigned(const FilePath& file_path) {
+  return false;
+}
+
+}  // namespace signature_util
+}  // namespace safe_browsing

chrome/browser/safe_browsing/signature_util_win.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/signature_util.h"
+
+#include <windows.h>
+#include <wincrypt.h>
+#include "base/file_path.h"
+
+namespace safe_browsing {
+namespace signature_util {
+
+bool IsSigned(const FilePath& file_path) {
+  // Get message handle and store handle from the signed file.
+  BOOL result = CryptQueryObject(CERT_QUERY_OBJECT_FILE,
+                                 file_path.value().c_str(),
+                                 CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED_EMBED,
+                                 CERT_QUERY_FORMAT_FLAG_BINARY,
+                                 0,      // flags
+                                 NULL,   // encoding
+                                 NULL,   // content type
+                                 NULL,   // format type
+                                 NULL,   // HCERTSTORE
+                                 NULL,   // HCRYPTMSG
+                                 NULL);  // context
+  return result == TRUE;
+}
+
+}  // namespace signature_util
+}  // namespace safe_browsing

chrome/browser/safe_browsing/signature_util_win_unittest.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/signature_util.h"
+
+#include "base/base_paths.h"
+#include "base/file_path.h"
+#include "base/path_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace safe_browsing {
+
+TEST(SignatureUtilWinTest, IsSigned) {
+  FilePath source_path;
+  ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &source_path));
+
+  FilePath testdata_path = source_path
+      .AppendASCII("chrome")
+      .AppendASCII("test")
+      .AppendASCII("data")
+      .AppendASCII("safe_browsing")
+      .AppendASCII("download_protection");
+
+  EXPECT_TRUE(signature_util::IsSigned(testdata_path.Append(L"signed.exe")));
+  EXPECT_FALSE(signature_util::IsSigned(
+      testdata_path.Append(L"unsigned.exe")));
+  EXPECT_FALSE(signature_util::IsSigned(
+      testdata_path.Append(L"doesnotexist.exe")));
+}
+
+}  // namespace safe_browsing

chrome/chrome_browser.gypi

@@ -2055,6 +2055,9 @@
         'browser/safe_browsing/safe_browsing_store_file.h',
         'browser/safe_browsing/safe_browsing_util.cc',
         'browser/safe_browsing/safe_browsing_util.h',
+        'browser/safe_browsing/signature_util_posix.cc',
+        'browser/safe_browsing/signature_util_win.cc',
+        'browser/safe_browsing/signature_util.h',
         'browser/screensaver_window_finder_gtk.cc',
         'browser/screensaver_window_finder_gtk.h',
         'browser/search_engines/search_engine_type.h',

chrome/chrome_tests.gypi

@@ -1508,6 +1508,7 @@
         'browser/safe_browsing/safe_browsing_store_unittest.cc',
         'browser/safe_browsing/safe_browsing_store_unittest_helper.cc',
         'browser/safe_browsing/safe_browsing_util_unittest.cc',
+        'browser/safe_browsing/signature_util_win_unittest.cc',
         'browser/search_engines/search_host_to_urls_map_unittest.cc',
         'browser/search_engines/search_provider_install_data_unittest.cc',
         'browser/search_engines/template_url_fetcher_unittest.cc',
@@ -1989,6 +1990,7 @@
           'sources/': [
             ['exclude', '^browser/password_manager/native_backend_gnome_x_unittest.cc'],
             ['exclude', '^browser/password_manager/native_backend_kwallet_x_unittest.cc'],
+            ['exclude', '^browser/safe_browsing/download_protection_service_unittest.cc' ],
             ['exclude', '^../content/browser/geolocation/wifi_data_provider_linux_unittest.cc'],
             # TODO(thestig) Enable PrintPreviewUI tests on CrOS when
             # print preview is enabled on CrOS.