[Downloads] Gracefully handle SafeBrowsing check failures.

If the SafeBrowsing service couldn't come up with a definite verdict for
some reason (e.g. couldn't reach the backend or encountered some error)
it would default to a verdict of SAFE. This in turn could cause a
download that would've been considered dangerous due its file type to be
considered safe. This change introduces an UNKNOWN verdict that the
SafeBrowsing service can use to indicate that a verdict couldn't be
determined.

Downloads then uses that signal to revert to the old behavior of
displaying a dangerous downloads warning based on the file type.

BUG=269157

Review URL: https://codereview.chromium.org/565053002

Cr-Commit-Position: refs/heads/master@{#295167}

chrome/browser/download/chrome_download_manager_delegate.cc

@@ -78,26 +78,10 @@ const char kSafeBrowsingUserDataKey[] = "Safe Browsing ID";
 // The state of a safebrowsing check.
 class SafeBrowsingState : public DownloadCompletionBlocker {
  public:
-  SafeBrowsingState()
-    : verdict_(DownloadProtectionService::SAFE) {
-  }
-
+  SafeBrowsingState() {}
   virtual ~SafeBrowsingState();
 
-  // The verdict that we got from calling CheckClientDownload. Only valid to
-  // call if |is_complete()|.
-  DownloadProtectionService::DownloadCheckResult verdict() const {
-    return verdict_;
-  }
-
-  void SetVerdict(DownloadProtectionService::DownloadCheckResult result) {
-    verdict_ = result;
-    CompleteDownload();
-  }
-
  private:
-  DownloadProtectionService::DownloadCheckResult verdict_;
-
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingState);
 };
 
@@ -159,7 +143,8 @@ void CheckDownloadUrlDone(
     bool is_content_check_supported,
     DownloadProtectionService::DownloadCheckResult result) {
   content::DownloadDangerType danger_type;
-  if (result == DownloadProtectionService::SAFE) {
+  if (result == DownloadProtectionService::SAFE ||
+      result == DownloadProtectionService::UNKNOWN) {
     // If this type of files is handled by the enhanced SafeBrowsing download
     // protection, mark it as potentially dangerous content until we are done
     // with scanning it.
@@ -293,7 +278,7 @@ void ChromeDownloadManagerDelegate::DisableSafeBrowsing(DownloadItem* item) {
     state = new SafeBrowsingState();
     item->SetUserData(&kSafeBrowsingUserDataKey, state);
   }
-  state->SetVerdict(DownloadProtectionService::SAFE);
+  state->CompleteDownload();
 #endif
 }
 
@@ -315,10 +300,25 @@ bool ChromeDownloadManagerDelegate::IsDownloadReadyForCompletion(
       item->SetUserData(&kSafeBrowsingUserDataKey, state);
       service->CheckClientDownload(
           item,
-          base::Bind(
-              &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
-              weak_ptr_factory_.GetWeakPtr(),
-              item->GetId()));
+          base::Bind(&ChromeDownloadManagerDelegate::CheckClientDownloadDone,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     item->GetId()));
+      return false;
+    }
+
+    // In case the service was disabled between the download starting and now,
+    // we need to restore the danger state.
+    content::DownloadDangerType danger_type = item->GetDangerType();
+    if (DownloadItemModel(item).IsDangerousFileBasedOnType() &&
+        (danger_type == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS ||
+         danger_type ==
+             content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT)) {
+      DVLOG(2) << __FUNCTION__
+               << "() SB service disabled. Marking download as DANGEROUS FILE";
+      item->OnContentCheckCompleted(
+          content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE);
+      content::BrowserThread::PostTask(
+          content::BrowserThread::UI, FROM_HERE, internal_complete_callback);
       return false;
     }
   } else if (!state->is_complete()) {
@@ -326,6 +326,7 @@ bool ChromeDownloadManagerDelegate::IsDownloadReadyForCompletion(
     state->set_callback(internal_complete_callback);
     return false;
   }
+
 #endif
   return true;
 }
@@ -636,6 +637,11 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
     content::DownloadDangerType danger_type =
         content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS;
     switch (result) {
+      case DownloadProtectionService::UNKNOWN:
+        // The check failed or was inconclusive.
+        if (DownloadItemModel(item).IsDangerousFileBasedOnType())
+          danger_type = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
+        break;
       case DownloadProtectionService::SAFE:
         // Do nothing.
         break;
@@ -659,7 +665,7 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
 
   SafeBrowsingState* state = static_cast<SafeBrowsingState*>(
       item->GetUserData(&kSafeBrowsingUserDataKey));
-  state->SetVerdict(result);
+  state->CompleteDownload();
 }
 #endif  // FULL_SAFE_BROWSING
 
@@ -688,10 +694,15 @@ void ChromeDownloadManagerDelegate::OnDownloadTargetDetermined(
     scoped_ptr<DownloadTargetInfo> target_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* item = download_manager_->GetDownload(download_id);
-  if (!target_info->target_path.empty() && item &&
-      IsOpenInBrowserPreferreredForFile(target_info->target_path) &&
-      target_info->is_filetype_handled_safely)
-    DownloadItemModel(item).SetShouldPreferOpeningInBrowser(true);
+  if (item) {
+    if (!target_info->target_path.empty() &&
+        IsOpenInBrowserPreferreredForFile(target_info->target_path) &&
+        target_info->is_filetype_handled_safely)
+      DownloadItemModel(item).SetShouldPreferOpeningInBrowser(true);
+
+    if (target_info->is_dangerous_file)
+      DownloadItemModel(item).SetIsDangerousFileBasedOnType(true);
+  }
   callback.Run(target_info->target_path,
                target_info->target_disposition,
                target_info->danger_type,

chrome/browser/download/download_browsertest.cc

@@ -47,9 +47,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h"
 #include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
-#include "chrome/browser/safe_browsing/download_feedback_service.h"
-#include "chrome/browser/safe_browsing/download_protection_service.h"
-#include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -95,6 +92,13 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(FULL_SAFE_BROWSING)
+#include "chrome/browser/safe_browsing/download_feedback_service.h"
+#include "chrome/browser/safe_browsing/download_protection_service.h"
+#include "chrome/browser/safe_browsing/safe_browsing_database.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#endif
+
 using content::BrowserContext;
 using content::BrowserThread;
 using content::DownloadItem;
@@ -572,11 +576,10 @@ class DownloadTest : public InProcessBrowserTest {
       Browser* browser,
       int num_downloads,
       content::DownloadTestObserver::DangerousDownloadAction
-      dangerous_download_action) {
+          dangerous_download_action) {
     DownloadManager* download_manager = DownloadManagerForBrowser(browser);
     return new content::DownloadTestObserverTerminal(
-        download_manager, num_downloads,
-        dangerous_download_action);
+        download_manager, num_downloads, dangerous_download_action);
   }
 
   void CheckDownloadStatesForBrowser(Browser* browser,
@@ -1914,6 +1917,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, PRE_DownloadTest_History) {
 }
 
 #if defined(OS_CHROMEOS)
+// Times out on ChromeOS: http://crbug.com/217810
 #define MAYBE_DownloadTest_History DISABLED_DownloadTest_History
 #else
 #define MAYBE_DownloadTest_History DownloadTest_History
@@ -3343,6 +3347,143 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadTest_GZipWithNoContent) {
 }
 
 #if defined(FULL_SAFE_BROWSING)
+
+// The following two tests are only meaningful on OS_WIN since that's the only
+// platform where client download checks are currently performed.
+// TODO(asanka): Relax this restriction as other platforms are added.
+#if defined(OS_WIN)
+namespace {
+
+// This is a custom DownloadTestObserver for
+// DangerousFileWithSBDisabledBeforeCompletion test that disables the
+// SafeBrowsing service when a single download is IN_PROGRESS and has a target
+// path assigned.  DownloadItemImpl is expected to call MaybeCompleteDownload
+// soon afterwards and we want to disable the service before then.
+class DisableSafeBrowsingOnInProgressDownload
+    : public content::DownloadTestObserver {
+ public:
+  explicit DisableSafeBrowsingOnInProgressDownload(Browser* browser)
+      : DownloadTestObserver(DownloadManagerForBrowser(browser),
+                             1,
+                             ON_DANGEROUS_DOWNLOAD_QUIT),
+        browser_(browser),
+        final_state_seen_(false) {
+    Init();
+  }
+  virtual ~DisableSafeBrowsingOnInProgressDownload() {}
+
+  virtual bool IsDownloadInFinalState(DownloadItem* download) OVERRIDE {
+    if (download->GetState() != DownloadItem::IN_PROGRESS ||
+        download->GetTargetFilePath().empty())
+      return false;
+
+    if (final_state_seen_)
+      return true;
+
+    final_state_seen_ = true;
+    browser_->profile()->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled,
+                                                false);
+    EXPECT_EQ(content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT,
+              download->GetDangerType());
+    EXPECT_FALSE(download->IsDangerous());
+    EXPECT_TRUE(DownloadItemModel(download).IsDangerousFileBasedOnType());
+    return true;
+  }
+
+ private:
+  Browser* browser_;
+  bool final_state_seen_;
+};
+
+}  // namespace
+
+IN_PROC_BROWSER_TEST_F(DownloadTest,
+                       DangerousFileWithSBDisabledBeforeCompletion) {
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled,
+                                               true);
+  ASSERT_TRUE(test_server()->Start());
+  GURL download_url(
+      test_server()->GetURL("files/downloads/dangerous/dangerous.exe"));
+  scoped_ptr<content::DownloadTestObserver> dangerous_observer(
+      DangerousDownloadWaiter(
+          browser(),
+          1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
+  scoped_ptr<content::DownloadTestObserver> in_progress_observer(
+      new DisableSafeBrowsingOnInProgressDownload(browser()));
+  ui_test_utils::NavigateToURLWithDisposition(browser(),
+                                              download_url,
+                                              NEW_BACKGROUND_TAB,
+                                              ui_test_utils::BROWSER_TEST_NONE);
+  in_progress_observer->WaitForFinished();
+
+  // SafeBrowsing should have been disabled by our observer.
+  ASSERT_FALSE(browser()->profile()->GetPrefs()->GetBoolean(
+      prefs::kSafeBrowsingEnabled));
+
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+  DownloadItem* download = downloads[0];
+
+  dangerous_observer->WaitForFinished();
+
+  EXPECT_TRUE(download->IsDangerous());
+  EXPECT_EQ(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE,
+            download->GetDangerType());
+  download->Cancel(true);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DangerousFileWithSBDisabledBeforeStart) {
+  // Disable SafeBrowsing
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kSafeBrowsingEnabled,
+                                               false);
+
+  ASSERT_TRUE(test_server()->Start());
+  GURL download_url(
+      test_server()->GetURL("files/downloads/dangerous/dangerous.exe"));
+  scoped_ptr<content::DownloadTestObserver> dangerous_observer(
+      DangerousDownloadWaiter(
+          browser(),
+          1,
+          content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT));
+  ui_test_utils::NavigateToURLWithDisposition(browser(),
+                                              download_url,
+                                              NEW_BACKGROUND_TAB,
+                                              ui_test_utils::BROWSER_TEST_NONE);
+  dangerous_observer->WaitForFinished();
+
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+
+  DownloadItem* download = downloads[0];
+  EXPECT_TRUE(download->IsDangerous());
+  EXPECT_EQ(content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE,
+            download->GetDangerType());
+
+  download->Cancel(true);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, SafeSupportedFile) {
+  ASSERT_TRUE(test_server()->Start());
+  GURL download_url(test_server()->GetURL("files/downloads/a_zip_file.zip"));
+  DownloadAndWait(browser(), download_url);
+
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForBrowser(browser())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+
+  DownloadItem* download = downloads[0];
+  EXPECT_FALSE(download->IsDangerous());
+  EXPECT_EQ(content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT,
+            download->GetDangerType());
+
+  download->Cancel(true);
+}
+
+#endif // OS_WIN
+
 IN_PROC_BROWSER_TEST_F(DownloadTest, FeedbackService) {
   // Make a dangerous file.
   base::FilePath file(FILE_PATH_LITERAL("downloads/dangerous/dangerous.swf"));

chrome/browser/download/download_item_model.cc

@@ -48,29 +48,6 @@ class DownloadItemModelData : public base::SupportsUserData::Data {
   // object if not found. Always returns a non-NULL pointer, unless OOM.
   static DownloadItemModelData* GetOrCreate(DownloadItem* download);
 
-  bool should_show_in_shelf() const { return should_show_in_shelf_; }
-  void set_should_show_in_shelf(bool should_show_in_shelf) {
-    should_show_in_shelf_ = should_show_in_shelf;
-  }
-
-  bool was_ui_notified() const { return was_ui_notified_; }
-  void set_was_ui_notified(bool was_ui_notified) {
-    was_ui_notified_ = was_ui_notified;
-  }
-
-  bool should_prefer_opening_in_browser() const {
-    return should_prefer_opening_in_browser_;
-  }
-  void set_should_prefer_opening_in_browser(bool preference) {
-    should_prefer_opening_in_browser_ = preference;
-  }
-
- private:
-  DownloadItemModelData();
-  virtual ~DownloadItemModelData() {}
-
-  static const char kKey[];
-
   // Whether the download should be displayed in the download shelf. True by
   // default.
   bool should_show_in_shelf_;
@@ -81,6 +58,16 @@ class DownloadItemModelData : public base::SupportsUserData::Data {
   // Whether the download should be opened in the browser vs. the system handler
   // for the file type.
   bool should_prefer_opening_in_browser_;
+
+  // Whether the download should be considered dangerous if SafeBrowsing doesn't
+  // come up with a verdict.
+  bool is_dangerous_file_based_on_type_;
+
+ private:
+  DownloadItemModelData();
+  virtual ~DownloadItemModelData() {}
+
+  static const char kKey[];
 };
 
 // static
@@ -107,7 +94,8 @@ DownloadItemModelData* DownloadItemModelData::GetOrCreate(
 DownloadItemModelData::DownloadItemModelData()
     : should_show_in_shelf_(true),
       was_ui_notified_(false),
-      should_prefer_opening_in_browser_(false) {
+      should_prefer_opening_in_browser_(false),
+      is_dangerous_file_based_on_type_(false) {
 }
 
 base::string16 InterruptReasonStatusMessage(int reason) {
@@ -560,12 +548,12 @@ bool DownloadItemModel::ShouldShowDownloadStartedAnimation() const {
 
 bool DownloadItemModel::ShouldShowInShelf() const {
   const DownloadItemModelData* data = DownloadItemModelData::Get(download_);
-  return !data || data->should_show_in_shelf();
+  return !data || data->should_show_in_shelf_;
 }
 
 void DownloadItemModel::SetShouldShowInShelf(bool should_show) {
   DownloadItemModelData* data = DownloadItemModelData::GetOrCreate(download_);
-  data->set_should_show_in_shelf(should_show);
+  data->should_show_in_shelf_ = should_show;
 }
 
 bool DownloadItemModel::ShouldNotifyUI() const {
@@ -594,22 +582,32 @@ bool DownloadItemModel::ShouldNotifyUI() const {
 
 bool DownloadItemModel::WasUINotified() const {
   const DownloadItemModelData* data = DownloadItemModelData::Get(download_);
-  return data && data->was_ui_notified();
+  return data && data->was_ui_notified_;
 }
 
 void DownloadItemModel::SetWasUINotified(bool was_ui_notified) {
   DownloadItemModelData* data = DownloadItemModelData::GetOrCreate(download_);
-  data->set_was_ui_notified(was_ui_notified);
+  data->was_ui_notified_ = was_ui_notified;
 }
 
 bool DownloadItemModel::ShouldPreferOpeningInBrowser() const {
   const DownloadItemModelData* data = DownloadItemModelData::Get(download_);
-  return data && data->should_prefer_opening_in_browser();
+  return data && data->should_prefer_opening_in_browser_;
 }
 
 void DownloadItemModel::SetShouldPreferOpeningInBrowser(bool preference) {
   DownloadItemModelData* data = DownloadItemModelData::GetOrCreate(download_);
-  data->set_should_prefer_opening_in_browser(preference);
+  data->should_prefer_opening_in_browser_ = preference;
+}
+
+bool DownloadItemModel::IsDangerousFileBasedOnType() const {
+  const DownloadItemModelData* data = DownloadItemModelData::Get(download_);
+  return data && data->is_dangerous_file_based_on_type_;
+}
+
+void DownloadItemModel::SetIsDangerousFileBasedOnType(bool dangerous) {
+  DownloadItemModelData* data = DownloadItemModelData::GetOrCreate(download_);
+  data->is_dangerous_file_based_on_type_ = dangerous;
 }
 
 base::string16 DownloadItemModel::GetProgressSizesString() const {

chrome/browser/download/download_item_model.h

@@ -137,6 +137,16 @@ class DownloadItemModel {
   // Change what's returned by ShouldPreferOpeningInBrowser to |preference|.
   void SetShouldPreferOpeningInBrowser(bool preference);
 
+  // Mark that the download should be considered dangerous based on the file
+  // type. This value may differ from the download's danger type in cases where
+  // the SafeBrowsing service hasn't returned a verdict about the download. If
+  // SafeBrowsing fails to return a decision, then the download should be
+  // considered dangerous based on this flag. Defaults to false.
+  bool IsDangerousFileBasedOnType() const;
+
+  // Change what's returned by IsDangerousFileBasedOnType().
+  void SetIsDangerousFileBasedOnType(bool dangerous);
+
   // Open the download using the platform handler for the download. The behavior
   // of this method will be different from DownloadItem::OpenDownload() if
   // ShouldPreferOpeningInBrowser().

chrome/browser/download/download_target_determiner.cc

@@ -89,6 +89,7 @@ DownloadTargetDeterminer::DownloadTargetDeterminer(
       create_target_directory_(false),
       conflict_action_(DownloadPathReservationTracker::OVERWRITE),
       danger_type_(download->GetDangerType()),
+      is_dangerous_file_(false),
       virtual_path_(initial_virtual_path),
       is_filetype_handled_safely_(false),
       download_(download),
@@ -576,10 +577,9 @@ DownloadTargetDeterminer::Result
   next_state_ = STATE_DETERMINE_INTERMEDIATE_PATH;
 
   // Checking if there are prior visits to the referrer is only necessary if the
-  // danger level of the download depends on the file type. This excludes cases
-  // where the download has already been deemed dangerous, or where the user is
-  // going to be prompted or where this is a programmatic download.
-  if (danger_type_ != content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)
+  // danger level of the download depends on the file type.
+  if (danger_type_ != content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS &&
+      danger_type_ != content::DOWNLOAD_DANGER_TYPE_MAYBE_DANGEROUS_CONTENT)
     return CONTINUE;
 
   // Assume that:
@@ -610,7 +610,9 @@ DownloadTargetDeterminer::Result
     // If the danger level doesn't depend on having visited the refererrer URL
     // or if original profile doesn't have a HistoryService or the referrer url
     // is invalid, then assume the referrer has not been visited before.
-    danger_type_ = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
+    is_dangerous_file_ = true;
+    if (danger_type_ == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)
+      danger_type_ = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
   }
   return CONTINUE;
 }
@@ -619,9 +621,12 @@ void DownloadTargetDeterminer::CheckVisitedReferrerBeforeDone(
     bool visited_referrer_before) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(STATE_DETERMINE_INTERMEDIATE_PATH, next_state_);
-  if (IsDangerousFile(
-          visited_referrer_before ? VISITED_REFERRER : NO_VISITS_TO_REFERRER))
-    danger_type_ = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
+  if (IsDangerousFile(visited_referrer_before ? VISITED_REFERRER
+                                              : NO_VISITS_TO_REFERRER)) {
+    is_dangerous_file_ = true;
+    if (danger_type_ == content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS)
+      danger_type_ = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE;
+  }
   DoLoop();
 }
 
@@ -707,7 +712,8 @@ void DownloadTargetDeterminer::ScheduleCallbackAndDeleteSelf() {
             << " Local:" << local_path_.AsUTF8Unsafe()
             << " Intermediate:" << intermediate_path_.AsUTF8Unsafe()
             << " Should prompt:" << should_prompt_
-            << " Danger type:" << danger_type_;
+            << " Danger type:" << danger_type_
+            << " Is dangerous file:" << is_dangerous_file_;
   scoped_ptr<DownloadTargetInfo> target_info(new DownloadTargetInfo);
 
   target_info->target_path = local_path_;
@@ -716,6 +722,7 @@ void DownloadTargetDeterminer::ScheduleCallbackAndDeleteSelf() {
            ? DownloadItem::TARGET_DISPOSITION_PROMPT
            : DownloadItem::TARGET_DISPOSITION_OVERWRITE);
   target_info->danger_type = danger_type_;
+  target_info->is_dangerous_file = is_dangerous_file_;
   target_info->intermediate_path = intermediate_path_;
   target_info->mime_type = mime_type_;
   target_info->is_filetype_handled_safely = is_filetype_handled_safely_;

chrome/browser/download/download_target_determiner.h

@@ -304,6 +304,7 @@ class DownloadTargetDeterminer
   bool create_target_directory_;
   DownloadPathReservationTracker::FilenameConflictAction conflict_action_;
   content::DownloadDangerType danger_type_;
+  bool is_dangerous_file_;  // See DownloadTargetInfo::is_dangerous_file
   base::FilePath virtual_path_;
   base::FilePath local_path_;
   base::FilePath intermediate_path_;

chrome/browser/download/download_target_info.h

@@ -26,6 +26,17 @@ struct DownloadTargetInfo {
   // Danger type of the download.
   content::DownloadDangerType danger_type;
 
+  // The danger type of the download could be set to MAYBE_DANGEROUS_CONTENT if
+  // the file type is handled by SafeBrowsing. However, if the SafeBrowsing
+  // service is unable to verify whether the file is safe or not, we are on our
+  // own. This flag indicates whether the download should be considered
+  // dangerous if SafeBrowsing returns an unknown verdict.
+  //
+  // Note that some downloads (e.g. "Save link as" on a link to a binary) would
+  // not be considered 'Dangerous' even if SafeBrowsing came back with an
+  // unknown verdict. So we can't always show a warning when SB fails.
+  bool is_dangerous_file;
+
   // Suggested intermediate path. The downloaded bytes should be written to this
   // path until all the bytes are available and the user has accepted a
   // dangerous download. At that point, the download can be renamed to

chrome/browser/safe_browsing/download_protection_service.cc

@@ -311,12 +311,12 @@ class DownloadProtectionService::CheckClientDownloadRequest
       switch (reason) {
         case REASON_EMPTY_URL_CHAIN:
         case REASON_INVALID_URL:
-          PostFinishTask(SAFE, reason);
+          PostFinishTask(UNKNOWN, reason);
           return;
 
         case REASON_NOT_BINARY_FILE:
           RecordFileExtensionType(item_->GetTargetFilePath());
-          PostFinishTask(SAFE, reason);
+          PostFinishTask(UNKNOWN, reason);
           return;
 
         default:
@@ -357,7 +357,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
             service_->download_request_timeout_ms()));
   }
 
-  // Canceling a request will cause us to always report the result as SAFE
+  // Canceling a request will cause us to always report the result as UNKNOWN
   // unless a pending request is about to call FinishRequest.
   void Cancel() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -371,7 +371,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
     // reference to this object.  We'll eventually wind up in some method on
     // the UI thread that will call FinishRequest() again.  If FinishRequest()
     // is called a second time, it will be a no-op.
-    FinishRequest(SAFE, REASON_REQUEST_CANCELED);
+    FinishRequest(UNKNOWN, REASON_REQUEST_CANCELED);
     // Calling FinishRequest might delete this object, we may be deleted by
     // this point.
   }
@@ -399,7 +399,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
         "SBClientDownload.DownloadRequestNetError",
         -source->GetStatus().error());
     DownloadCheckResultReason reason = REASON_SERVER_PING_FAILED;
-    DownloadCheckResult result = SAFE;
+    DownloadCheckResult result = UNKNOWN;
     if (source->GetStatus().is_success() &&
         net::HTTP_OK == source->GetResponseCode()) {
       ClientDownloadResponse response;
@@ -408,16 +408,19 @@ class DownloadProtectionService::CheckClientDownloadRequest
       DCHECK(got_data);
       if (!response.ParseFromString(data)) {
         reason = REASON_INVALID_RESPONSE_PROTO;
+        result = UNKNOWN;
       } else if (response.verdict() == ClientDownloadResponse::SAFE) {
         reason = REASON_DOWNLOAD_SAFE;
+        result = SAFE;
       } else if (service_ && !service_->IsSupportedDownload(
           *item_, item_->GetTargetFilePath())) {
         // The client of the download protection service assumes that we don't
         // support this download so we cannot return any other verdict than
-        // SAFE even if the server says it's dangerous to download this file.
+        // UNKNOWN even if the server says it's dangerous to download this file.
         // Note: if service_ is NULL we already cancelled the request and
-        // returned SAFE.
+        // returned UNKNOWN.
         reason = REASON_DOWNLOAD_NOT_SUPPORTED;
+        result = UNKNOWN;
       } else if (response.verdict() == ClientDownloadResponse::DANGEROUS) {
         reason = REASON_DOWNLOAD_DANGEROUS;
         result = DANGEROUS;
@@ -435,6 +438,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
         LOG(DFATAL) << "Unknown download response verdict: "
                     << response.verdict();
         reason = REASON_INVALID_RESPONSE_VERDICT;
+        result = UNKNOWN;
       }
       DownloadFeedbackService::MaybeStorePingsForDownload(
           result, item_, client_download_request_data_, data);
@@ -566,55 +570,61 @@ class DownloadProtectionService::CheckClientDownloadRequest
                         base::TimeTicks::Now() - zip_analysis_start_time_);
 
     if (!zipped_executable_) {
-      PostFinishTask(SAFE, REASON_ARCHIVE_WITHOUT_BINARIES);
+      PostFinishTask(UNKNOWN, REASON_ARCHIVE_WITHOUT_BINARIES);
       return;
     }
     OnFileFeatureExtractionDone();
   }
 
+  static void RecordCountOfSignedOrWhitelistedDownload() {
+    UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedOrWhitelistedDownload", 1);
+  }
+
   void CheckWhitelists() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    DownloadCheckResultReason reason = REASON_MAX;
+
     if (!database_manager_.get()) {
-      reason = REASON_SB_DISABLED;
-    } else {
-      const GURL& url = url_chain_.back();
-      if (url.is_valid() && database_manager_->MatchDownloadWhitelistUrl(url)) {
-        VLOG(2) << url << " is on the download whitelist.";
-        reason = REASON_WHITELISTED_URL;
-      }
-      if (reason != REASON_MAX || signature_info_.trusted()) {
-        UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedOrWhitelistedDownload", 1);
-      }
+      PostFinishTask(UNKNOWN, REASON_SB_DISABLED);
+      return;
+    }
+
+    const GURL& url = url_chain_.back();
+    if (url.is_valid() && database_manager_->MatchDownloadWhitelistUrl(url)) {
+      VLOG(2) << url << " is on the download whitelist.";
+      RecordCountOfSignedOrWhitelistedDownload();
+      PostFinishTask(SAFE, REASON_WHITELISTED_URL);
+      return;
     }
-    if (reason == REASON_MAX && signature_info_.trusted()) {
+
+    if (signature_info_.trusted()) {
+      RecordCountOfSignedOrWhitelistedDownload();
       for (int i = 0; i < signature_info_.certificate_chain_size(); ++i) {
         if (CertificateChainIsWhitelisted(
                 signature_info_.certificate_chain(i))) {
-          reason = REASON_TRUSTED_EXECUTABLE;
-          break;
+          PostFinishTask(SAFE, REASON_TRUSTED_EXECUTABLE);
+          return;
         }
       }
     }
-    if (reason != REASON_MAX) {
-      PostFinishTask(SAFE, reason);
-    } else if (!pingback_enabled_) {
-      PostFinishTask(SAFE, REASON_PING_DISABLED);
-    } else {
-      // Currently, the UI only works on Windows so we don't even bother
-      // with pinging the server if we're not on Windows.  TODO(noelutz):
-      // change this code once the UI is done for Linux and Mac.
+
+    if (!pingback_enabled_) {
+      PostFinishTask(UNKNOWN, REASON_PING_DISABLED);
+      return;
+    }
+
+    // Currently, the UI only works on Windows so we don't even bother with
+    // pinging the server if we're not on Windows.
+    // TODO(noelutz): change this code once the UI is done for Linux and Mac.
 #if defined(OS_WIN)
-      // The URLFetcher is owned by the UI thread, so post a message to
-      // start the pingback.
-      BrowserThread::PostTask(
-          BrowserThread::UI,
-          FROM_HERE,
-          base::Bind(&CheckClientDownloadRequest::GetTabRedirects, this));
+    // The URLFetcher is owned by the UI thread, so post a message to
+    // start the pingback.
+    BrowserThread::PostTask(
+        BrowserThread::UI,
+        FROM_HERE,
+        base::Bind(&CheckClientDownloadRequest::GetTabRedirects, this));
 #else
-      PostFinishTask(SAFE, REASON_OS_NOT_SUPPORTED);
+    PostFinishTask(UNKNOWN, REASON_OS_NOT_SUPPORTED);
 #endif
-    }
   }
 
   void GetTabRedirects() {
@@ -709,7 +719,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
     request.mutable_signature()->CopyFrom(signature_info_);
     request.mutable_image_headers()->CopyFrom(image_headers_);
     if (!request.SerializeToString(&client_download_request_data_)) {
-      FinishRequest(SAFE, REASON_INVALID_REQUEST_PROTO);
+      FinishRequest(UNKNOWN, REASON_INVALID_REQUEST_PROTO);
       return;
     }
 
@@ -765,7 +775,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
     }
     if (service_) {
       VLOG(2) << "SafeBrowsing download verdict for: "
-              << item_->DebugString(true) << " verdict:" << reason;
+              << item_->DebugString(true) << " verdict:" << reason
+              << " result:" << result;
       UMA_HISTOGRAM_ENUMERATION("SBClientDownload.CheckDownloadStats",
                                 reason,
                                 REASON_MAX);
@@ -778,7 +789,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
       // DownloadProtectionService::RequestFinished will decrement our refcount,
       // so we may be deleted now.
     } else {
-      callback_.Run(SAFE);
+      callback_.Run(UNKNOWN);
     }
   }
 

chrome/browser/safe_browsing/download_protection_service.h

@@ -41,6 +41,7 @@ class BinaryFeatureExtractor;
 class DownloadProtectionService {
  public:
   enum DownloadCheckResult {
+    UNKNOWN,
     SAFE,
     DANGEROUS,
     UNCOMMON,
@@ -90,7 +91,7 @@ class DownloadProtectionService {
   // Enables or disables the service.  This is usually called by the
   // SafeBrowsingService, which tracks whether any profile uses these services
   // at all.  Disabling causes any pending and future requests to have their
-  // callbacks called with "SAFE" results.
+  // callbacks called with "UNKNOWN" results.
   void SetEnabled(bool enabled);
 
   bool enabled() const {

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -379,7 +379,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
   Mock::VerifyAndClearExpectations(&item);
 
   url_chain.push_back(GURL("file://www.google.com/"));
@@ -395,7 +395,30 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+}
+
+TEST_F(DownloadProtectionServiceTest, CheckClientDownloadNotABinary) {
+  base::FilePath a_tmp(FILE_PATH_LITERAL("a.tmp"));
+  base::FilePath a_txt(FILE_PATH_LITERAL("a.txt"));
+  std::vector<GURL> url_chain;
+  GURL referrer("http://www.google.com/");
+
+  content::MockDownloadItem item;
+  url_chain.push_back(GURL("http://www.example.com/foo"));
+  EXPECT_CALL(item, GetFullPath()).WillRepeatedly(ReturnRef(a_tmp));
+  EXPECT_CALL(item, GetTargetFilePath()).WillRepeatedly(ReturnRef(a_txt));
+  EXPECT_CALL(item, GetUrlChain()).WillRepeatedly(ReturnRef(url_chain));
+  EXPECT_CALL(item, GetReferrerUrl()).WillRepeatedly(ReturnRef(referrer));
+  EXPECT_CALL(item, GetTabUrl()).WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+  EXPECT_CALL(item, GetTabReferrerUrl())
+      .WillRepeatedly(ReturnRef(GURL::EmptyGURL()));
+  download_service_->CheckClientDownload(
+      &item,
+      base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
+                 base::Unretained(this)));
+  MessageLoop::current()->Run();
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
@@ -451,7 +474,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 
   // Check that the referrer is not matched against the whitelist.
@@ -464,7 +487,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 
   // Redirect from a site shouldn't be checked either.
@@ -477,7 +500,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadWhitelistedUrl) {
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 
   // Only if the final url is whitelisted should it be SAFE.
@@ -528,7 +551,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadFetchFailed) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
@@ -574,9 +597,15 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
+#if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+#else
+  // On !OS_WIN, no file types are currently supported. Hence all erquests to
+  // CheckClientDownload() result in a verdict of UNKNOWN.
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#endif
 
-  // Invalid response should be safe too.
+  // Invalid response should result in UNKNOWN.
   response.Clear();
   factory.SetFakeResponse(
       DownloadProtectionService::GetDownloadRequestUrl(),
@@ -588,7 +617,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
   std::string feedback_ping;
   std::string feedback_response;
   EXPECT_FALSE(DownloadFeedbackService::GetPingsForDownloadForTesting(
@@ -611,7 +640,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 
   // If the response is uncommon the result should also be marked as uncommon.
@@ -635,7 +664,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
   EXPECT_EQ(url_chain.back().spec(), decoded_request.url());
   EXPECT_EQ(response.SerializeAsString(), feedback_response);
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 
   // If the response is dangerous_host the result should also be marked as
@@ -657,7 +686,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
       item, &feedback_ping, &feedback_response));
   EXPECT_EQ(response.SerializeAsString(), feedback_response);
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 
   // If the response is POTENTIALLY_UNWANTED the result should also be marked as
@@ -676,7 +705,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadSuccess) {
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::POTENTIALLY_UNWANTED));
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 }
 
@@ -725,7 +754,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadHTTPS) {
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
 }
 
@@ -777,7 +806,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadZip) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
   Mock::VerifyAndClearExpectations(sb_service_.get());
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 
@@ -796,7 +825,13 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadZip) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
+#if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+#else
+  // For !OS_WIN, no file types are currently supported. Hence the resulting
+  // verdict is UNKNOWN.
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
+#endif
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 
   // If the response is dangerous the result should also be marked as
@@ -815,7 +850,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadZip) {
 #if defined(OS_WIN)
   EXPECT_TRUE(IsResult(DownloadProtectionService::DANGEROUS));
 #else
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 #endif
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 }
@@ -853,7 +888,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadCorruptZip) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
   Mock::VerifyAndClearExpectations(sb_service_.get());
   Mock::VerifyAndClearExpectations(binary_feature_extractor_.get());
 }
@@ -905,7 +940,7 @@ TEST_F(DownloadProtectionServiceTest, CheckClientCrxDownloadSuccess) {
       base::Bind(&DownloadProtectionServiceTest::CheckDoneCallback,
                  base::Unretained(this)));
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 }
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadValidateRequest) {
@@ -1354,7 +1389,7 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadRequestTimeout) {
   // The request should time out because the HTTP request hasn't returned
   // anything yet.
   MessageLoop::current()->Run();
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 }
 
 TEST_F(DownloadProtectionServiceTest, TestDownloadItemDestroyed) {
@@ -1397,7 +1432,7 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadItemDestroyed) {
     // notification.
   }
 
-  EXPECT_TRUE(IsResult(DownloadProtectionService::SAFE));
+  EXPECT_TRUE(IsResult(DownloadProtectionService::UNKNOWN));
 }
 
 TEST_F(DownloadProtectionServiceTest, GetCertificateWhitelistStrings) {