Propagate the SafeBrowsing download protection verdict to the DownloadItem.

chrome/browser/download/chrome_download_manager_delegate.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/download/chrome_download_manager_delegate.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/file_util.h"
@@ skipping 68 matching lines @@
   // history service thread is still reading the persistent state, we do not
   // insert the new DownloadItem into 'history_downloads_' or inform our
   // observers at this point. OnCreateDownloadEntryComplete() handles that
   // finalization of the the download creation as a callback from the history
   // thread.
   DownloadItem* download =
       download_manager_->GetActiveDownloadItem(download_id);
   if (!download)
     return false;
 
 #if defined(ENABLE_SAFE_BROWSING)

[Randy Smith (Not in Mondays), 2011/11/16 18:04:04]

GetDownloadProtectionService() isn't inside of an #ifdef; does that mean we
could get rid of this ifdef as well?

[noelutz, 2011/11/16 23:15:07]

I think it's needed. changed.

[Randy Smith (Not in Mondays), 2011/11/17 16:49:31]

Hmmm.  So it would seem to me that we could also arrange for a stub definition
of DownloadProtectionService/GetDownloadProtectionService in the case where
ENABLE_SAFE_BROWSING wasn't defined, which would result in cleaner code at point
of call.  What's the problem with that approach?

Note that this is a nit: I'm ok with the code as written.  I'd just like it
better if more of the #ifdef was in SafeBrowsing specific code.

[noelutz, 2011/11/17 17:05:44]

Agreed.  If that's OK I would like to do that in a separate CL.

-  SafeBrowsingService* sb_service =
-      g_browser_process->safe_browsing_service();
-  if (sb_service && sb_service->download_protection_service() &&
-      profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+  DownloadProtectionService* service = GetDownloadProtectionService();
+  if (service) {
     VLOG(2) << __FUNCTION__ << "() Start SB URL check for download = "
             << download->DebugString(false);
-    sb_service->download_protection_service()->CheckDownloadUrl(
+    service->CheckDownloadUrl(
         DownloadProtectionService::DownloadInfo::FromDownloadItem(*download),
         base::Bind(
             &ChromeDownloadManagerDelegate::CheckDownloadUrlDone,
             this,
             download->id()));
     return false;
   }
 #endif
   CheckDownloadUrlDone(download_id, DownloadProtectionService::SAFE);
   return false;
 }
 
 void ChromeDownloadManagerDelegate::ChooseDownloadPath(
     TabContents* tab_contents,
     const FilePath& suggested_path,
     void* data) {
   // Deletes itself.
   new DownloadFilePicker(
       download_manager_, tab_contents, suggested_path, data);
 }
 
 bool ChromeDownloadManagerDelegate::OverrideIntermediatePath(
     DownloadItem* item,
     FilePath* intermediate_path) {
-  if (item->IsDangerous()) {
-    // The download is not safe.  It's name is already set to an intermediate
-    // name, so no need to override.
+  if (item->needs_quarantine_file()) {
+    // The download might not be safe.  It's name is already set to an
+    // intermediate name, so no need to override.
     return false;
   }
 
   // The download is a safe download.  We need to rename it to its intermediate
   // '.crdownload' path.  The final name after user confirmation will be set
   // from DownloadItem::OnDownloadCompleting.
   *intermediate_path = download_util::GetCrDownloadPath(item->full_path());
   return true;
 }
 
@@ skipping 23 matching lines @@
   // See if there is already a pending SafeBrowsing check for that download.
   SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->id());
   if (it != safe_browsing_state_.end()) {
     SafeBrowsingState state = it->second;
     if (!state.pending) {
       safe_browsing_state_.erase(it);
     }
     return !state.pending;
   }
   // 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)) {
+  DownloadProtectionService* service = GetDownloadProtectionService();
+  if (service) {
     VLOG(2) << __FUNCTION__ << "() Start SB download check for download = "
             << item->DebugString(false);
-    sb_service->download_protection_service()->CheckClientDownload(
+    service->CheckClientDownload(
         DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
         base::Bind(
             &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
             this,
             item->id()));
     SafeBrowsingState state;
     state.pending = true;
     state.verdict = DownloadProtectionService::SAFE;
     safe_browsing_state_[item->id()] = state;
     return false;
@@ skipping 108 matching lines @@
 
   float progress = 0;
   int download_count = 0;
   bool progress_known =
       g_browser_process->download_status_updater()->GetProgress(
           &progress, &download_count);
   download_util::UpdateAppIconDownloadProgress(
       download_count, progress_known, progress);
 }
 
+DownloadProtectionService*
+    ChromeDownloadManagerDelegate::GetDownloadProtectionService() {
+#if defined(ENABLE_SAFE_BROWSING)
+  SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service();
+  if (sb_service && sb_service->download_protection_service() &&
+      profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+    return sb_service->download_protection_service();
+  }
+#endif
+  return NULL;
+}
+
 void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
     int32 download_id,
     DownloadProtectionService::DownloadCheckResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DownloadItem* download =
       download_manager_->GetActiveDownloadItem(download_id);
   if (!download)
     return;
 
   VLOG(2) << __FUNCTION__ << "() download = " << download->DebugString(false)
@@ skipping 11 matching lines @@
     int32 download_id,
     DownloadProtectionService::DownloadCheckResult result) {
   DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
   if (!item) {
     safe_browsing_state_.erase(download_id);  // Just in case.
     return;
   }
 
   VLOG(2) << __FUNCTION__ << "() download = " << item->DebugString(false)
           << " verdict = " << result;
-  // TODO(noelutz):
-  //  1) display a warning if the result is DANGEROUS.
-  //  2) make sure we haven't already displayed a warning for the URL.
-  //  3) disable the existing dangerous file warning for executables.
+  // We only mark the content as being dangerous if the download's safety state
+  // has not been set to DANGEROUS yet.  We don't want to show two warnings.
+  if (result == DownloadProtectionService::DANGEROUS &&
+      item->safety_state() == DownloadItem::SAFE)
+    item->MarkContentDangerous();
 
   SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->id());
   DCHECK(it != safe_browsing_state_.end() && it->second.pending);
   if (it != safe_browsing_state_.end()) {
     it->second.pending = false;
     it->second.verdict = result;
   }
   download_manager_->MaybeCompleteDownload(item);
 }
 
@@ skipping 64 matching lines @@
       state.suggested_path = download_prefs_->download_path();
     }
     state.suggested_path = state.suggested_path.Append(generated_name);
   } else {
     state.suggested_path = state.force_file_name;
   }
 
   if (!state.prompt_user_for_save_location && state.force_file_name.empty()) {
     state.is_dangerous_file =
         IsDangerousFile(*download, state, visited_referrer_before);
+    state.needs_quarantine_file = true;
   }
 
+#if defined(ENABLE_SAFE_BROWSING)
+  DownloadProtectionService* service = GetDownloadProtectionService();
+  // Return false if this type of files is handled by the enhanced SafeBrowsing
+  // download protection.
+  if (service && service->enabled() &&
+      service->IsSupportedFileType(state.suggested_path.BaseName())) {

[Randy Smith (Not in Mondays), 2011/11/16 17:52:41]

Asanka: I just wanted to explicitly check with you (with your file name
determination hat on) that we have the correct file type (all hints incorporated
in) at this point?

[asanka, 2011/11/16 18:34:05]

Good question.

At this point state.suggested_path contains the filename derived from the
request/response data (URL, Content-Disposition, suggested name, etc.) but not
the filename that the user might choose from a "Save As" dialog.  Note that the
extension might not be what you expect for the content type.

Noe: If we were to determine an innocuous extension (because of a missing or
malicious C-D header, malicious download attribute etc.) and the user later
picks a dangerous extension from the Save As dialog, do we handle it correctly?

[noelutz, 2011/11/16 23:15:07]

I don't think so.  I think that's good enough for now though.  Added a TODO. 
Note that the dangerous file warning has the same issue afaik.

+    state.is_dangerous_file = false;
+    state.needs_quarantine_file = true;
+  }
+#endif
+
   // We need to move over to the download thread because we don't want to stat
   // the suggested path on the UI thread.
   // We can only access preferences on the UI thread, so check the download path
   // now and pass the value to the FILE thread.
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&ChromeDownloadManagerDelegate::CheckIfSuggestedPathExists,
                  this, download->id(), state,
                  download_prefs_->download_path()));
 }
@@ skipping 14 matching lines @@
   FilePath dir = state.suggested_path.DirName();
   FilePath filename = state.suggested_path.BaseName();
   if (!file_util::PathIsWritable(dir)) {
     VLOG(1) << "Unable to write to directory \"" << dir.value() << "\"";
     state.prompt_user_for_save_location = true;
     PathService::Get(chrome::DIR_USER_DOCUMENTS, &state.suggested_path);
     state.suggested_path = state.suggested_path.Append(filename);
   }
 
   // If the download is deemed dangerous, we'll use a temporary name for it.
-  if (state.IsDangerous()) {
+  if (state.needs_quarantine_file) {
     state.target_name = FilePath(state.suggested_path).BaseName();
     // Create a temporary file to hold the file until the user approves its
     // download.
     FilePath::StringType file_name;
     FilePath path;
 #if defined(OS_WIN)
     string16 unconfirmed_prefix =
         l10n_util::GetStringUTF16(IDS_DOWNLOAD_UNCONFIRMED_PREFIX);
 #else
     std::string unconfirmed_prefix =
@@ skipping 31 matching lines @@
               << state.suggested_path.value() << "\"";
       state.prompt_user_for_save_location = true;
     }
   }
 
   // Create an empty file at the suggested path so that we don't allocate the
   // same "non-existant" path to multiple downloads.
   // See: http://code.google.com/p/chromium/issues/detail?id=3662
   if (!state.prompt_user_for_save_location &&
       state.force_file_name.empty()) {
-    if (state.IsDangerous())
+    if (state.needs_quarantine_file)
       file_util::WriteFile(state.suggested_path, "", 0);
     else
       file_util::WriteFile(download_util::GetCrDownloadPath(
           state.suggested_path), "", 0);
   }
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&ChromeDownloadManagerDelegate::OnPathExistenceAvailable,
                  this, download_id, state));
@@ skipping 14 matching lines @@
 bool ChromeDownloadManagerDelegate::IsDangerousFile(
     const DownloadItem& download,
     const DownloadStateInfo& state,
     bool visited_referrer_before) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Anything loaded directly from the address bar is OK.
   if (state.transition_type & content::PAGE_TRANSITION_FROM_ADDRESS_BAR)
     return false;
 
-  // Return false if this type of files is handled by the enhanced SafeBrowsing
-  // download protection.
-  // TODO(noelutz): implement that check.
-
   // Extensions that are not from the gallery are considered dangerous.
   if (IsExtensionDownload(&download)) {
     ExtensionService* service = profile_->GetExtensionService();
     if (!service || !service->IsDownloadFromGallery(download.GetURL(),
                                                     download.referrer_url()))
       return true;
   }
 
   // Anything the user has marked auto-open is OK if it's user-initiated.
   if (ShouldOpenFileBasedOnExtension(state.suggested_path) &&
@@ skipping 14 matching lines @@
     int32 download_id, int64 db_handle) {
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
   // DownloadItems with the same invalid db_handle, so we need to assign a
   // unique |db_handle| here.
   if (db_handle == DownloadItem::kUninitializedHandle)
     db_handle = download_history_->GetNextFakeDbHandle();
   download_manager_->OnItemAddedToPersistentStore(download_id, db_handle);
 }