Skip malware scanning if we already know the result

If the URL is whitelisted, or the file is known to be dangerous, we
should not perform malware scanning. We still should perform DLP
scanning, if it's enabled.

Change-Id: I568602ef3ad3826413fe75b10a792a511fa05e1f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2030052
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737108}

chrome/browser/safe_browsing/download_protection/check_client_download_request.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_utils.h"
 #include "chrome/browser/safe_browsing/dm_token_utils.h"
 #include "chrome/browser/safe_browsing/download_protection/check_client_download_request_base.h"
+#include "chrome/browser/safe_browsing/download_protection/deep_scanning_request.h"
 #include "chrome/browser/safe_browsing/download_protection/download_feedback_service.h"
 #include "chrome/browser/safe_browsing/download_protection/download_item_request.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_service.h"
@@ -46,6 +47,43 @@
 
 namespace safe_browsing {
 
+namespace {
+
+// This function is called when the result of malware scanning is already known
+// (via |reason|), but we still want to perform DLP scanning.
+void MaybeOverrideDlpScanResult(DownloadCheckResultReason reason,
+                                CheckDownloadRepeatingCallback callback,
+                                DownloadCheckResult deep_scan_result) {
+  if (reason == REASON_DOWNLOAD_DANGEROUS) {
+    switch (deep_scan_result) {
+      case DownloadCheckResult::UNKNOWN:
+      case DownloadCheckResult::SENSITIVE_CONTENT_WARNING:
+      case DownloadCheckResult::DEEP_SCANNED_SAFE:
+        callback.Run(DownloadCheckResult::DANGEROUS);
+        return;
+
+      case DownloadCheckResult::ASYNC_SCANNING:
+      case DownloadCheckResult::BLOCKED_PASSWORD_PROTECTED:
+      case DownloadCheckResult::BLOCKED_TOO_LARGE:
+      case DownloadCheckResult::SENSITIVE_CONTENT_BLOCK:
+        callback.Run(deep_scan_result);
+        return;
+
+      case DownloadCheckResult::SAFE:
+      case DownloadCheckResult::UNCOMMON:
+      case DownloadCheckResult::DANGEROUS_HOST:
+      case DownloadCheckResult::WHITELISTED_BY_POLICY:
+      case DownloadCheckResult::PROMPT_FOR_SCANNING:
+      case DownloadCheckResult::DANGEROUS:
+      case DownloadCheckResult::POTENTIALLY_UNWANTED:
+        NOTREACHED();
+        return;
+    }
+  }
+}
+
+}  // namespace
+
 using content::BrowserThread;
 
 CheckClientDownloadRequest::CheckClientDownloadRequest(
@@ -222,16 +260,21 @@ bool CheckClientDownloadRequest::ShouldUploadBinary(
   if (reason == REASON_DOWNLOAD_DESTROYED)
     return false;
 
-  // If the download is known to be dangerous, we don't need to upload it.
-  if (reason == REASON_DOWNLOAD_DANGEROUS)
-    return false;
-
   return DeepScanningRequest::ShouldUploadItemByPolicy(item_);
 }
 
-void CheckClientDownloadRequest::UploadBinary() {
-  service()->UploadForDeepScanning(
-      item_, callback_, DeepScanningRequest::DeepScanTrigger::TRIGGER_POLICY);
+void CheckClientDownloadRequest::UploadBinary(
+    DownloadCheckResultReason reason) {
+  if (reason == REASON_DOWNLOAD_DANGEROUS || reason == REASON_WHITELISTED_URL) {
+    service()->UploadForDeepScanning(
+        item_,
+        base::BindRepeating(&MaybeOverrideDlpScanResult, reason, callback_),
+        DeepScanningRequest::DeepScanTrigger::TRIGGER_POLICY,
+        {DeepScanningRequest::DeepScanType::SCAN_DLP});
+  } else {
+    service()->UploadForDeepScanning(
+        item_, callback_, DeepScanningRequest::DeepScanTrigger::TRIGGER_POLICY);
+  }
 }
 
 void CheckClientDownloadRequest::NotifyRequestFinished(

chrome/browser/safe_browsing/download_protection/check_client_download_request.h

@@ -65,7 +65,7 @@ class CheckClientDownloadRequest : public CheckClientDownloadRequestBase,
   // Uploads the binary for deep scanning if the reason and policies indicate
   // it should be.
   bool ShouldUploadBinary(DownloadCheckResultReason reason) override;
-  void UploadBinary() override;
+  void UploadBinary(DownloadCheckResultReason reason) override;
 
   // Called when this request is completed.
   void NotifyRequestFinished(DownloadCheckResult result,

chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc

@@ -202,7 +202,7 @@ void CheckClientDownloadRequestBase::FinishRequest(
   }
 
   if (ShouldUploadBinary(reason)) {
-    UploadBinary();
+    UploadBinary(reason);
   } else {
     std::move(callback_).Run(result);
   }

chrome/browser/safe_browsing/download_protection/check_client_download_request_base.h

@@ -136,7 +136,7 @@ class CheckClientDownloadRequestBase {
 
   // If ShouldUploadBinary is true, actually performs the upload to Safe
   // Browsing for deep scanning.
-  virtual void UploadBinary() = 0;
+  virtual void UploadBinary(DownloadCheckResultReason reason) = 0;
 
   // Called whenever a request has completed.
   virtual void NotifyRequestFinished(DownloadCheckResult result,

chrome/browser/safe_browsing/download_protection/check_native_file_system_write_request.cc

@@ -156,7 +156,8 @@ bool CheckNativeFileSystemWriteRequest::ShouldUploadBinary(
   return false;
 }
 
-void CheckNativeFileSystemWriteRequest::UploadBinary() {}
+void CheckNativeFileSystemWriteRequest::UploadBinary(
+    DownloadCheckResultReason reason) {}
 
 bool CheckNativeFileSystemWriteRequest::ShouldPromptForDeepScanning(
     DownloadCheckResultReason reason) const {

chrome/browser/safe_browsing/download_protection/check_native_file_system_write_request.h

@@ -50,7 +50,7 @@ class CheckNativeFileSystemWriteRequest
                                   const std::string& request_data,
                                   const std::string& response_body) override;
   bool ShouldUploadBinary(DownloadCheckResultReason reason) override;
-  void UploadBinary() override;
+  void UploadBinary(DownloadCheckResultReason reason) override;
   bool ShouldPromptForDeepScanning(
       DownloadCheckResultReason reason) const override;
   void NotifyRequestFinished(DownloadCheckResult result,

chrome/browser/safe_browsing/download_protection/deep_scanning_request.cc

@@ -137,15 +137,33 @@ bool DeepScanningRequest::ShouldUploadItemByPolicy(
          ShouldUploadForMalwareScanByPolicy(item);
 }
 
+/* static */
+std::vector<DeepScanningRequest::DeepScanType> DeepScanningRequest::AllScans() {
+  return {DeepScanType::SCAN_DLP, DeepScanType::SCAN_MALWARE};
+}
+
 DeepScanningRequest::DeepScanningRequest(
     download::DownloadItem* item,
     DeepScanTrigger trigger,
     CheckDownloadRepeatingCallback callback,
     DownloadProtectionService* download_service)
+    : DeepScanningRequest(item,
+                          trigger,
+                          callback,
+                          download_service,
+                          DeepScanningRequest::AllScans()) {}
+
+DeepScanningRequest::DeepScanningRequest(
+    download::DownloadItem* item,
+    DeepScanTrigger trigger,
+    CheckDownloadRepeatingCallback callback,
+    DownloadProtectionService* download_service,
+    std::vector<DeepScanType> allowed_scans)
     : item_(item),
       trigger_(trigger),
       callback_(callback),
       download_service_(download_service),
+      allowed_scans_(allowed_scans),
       weak_ptr_factory_(this) {
   item_->AddObserver(this);
 }
@@ -177,7 +195,8 @@ void DeepScanningRequest::Start() {
     policy::DMToken dm_token = GetDMToken(profile);
     request->set_dm_token(dm_token.value());
 
-    if (ShouldUploadForDlpScanByPolicy(item_)) {
+    if (ShouldUploadForDlpScanByPolicy(item_) &&
+        ScanIsAllowed(DeepScanType::SCAN_DLP)) {
       DlpDeepScanningClientRequest dlp_request;
       dlp_request.set_content_source(
           DlpDeepScanningClientRequest::FILE_DOWNLOAD);
@@ -187,7 +206,8 @@ void DeepScanningRequest::Start() {
       request->set_request_dlp_scan(std::move(dlp_request));
     }
 
-    if (ShouldUploadForMalwareScanByPolicy(item_)) {
+    if (ShouldUploadForMalwareScanByPolicy(item_) &&
+        ScanIsAllowed(DeepScanType::SCAN_MALWARE)) {
       MalwareDeepScanningClientRequest malware_request;
       malware_request.set_population(
           MalwareDeepScanningClientRequest::POPULATION_ENTERPRISE);
@@ -303,4 +323,8 @@ void DeepScanningRequest::OpenDownload() {
   FinishRequest(DownloadCheckResult::UNKNOWN);
 }
 
+bool DeepScanningRequest::ScanIsAllowed(DeepScanType scan) {
+  return base::Contains(allowed_scans_, scan);
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/download_protection/deep_scanning_request.h

@@ -35,10 +35,22 @@ class DeepScanningRequest : public download::DownloadItem::Observer {
     TRIGGER_POLICY,
   };
 
+  // Enum representing the possible scans for a deep scanning request.
+  enum class DeepScanType {
+    // Scan for malware
+    SCAN_MALWARE,
+
+    // Scan for DLP policy violations
+    SCAN_DLP,
+  };
+
   // Checks the current policies to determine whether files must be uploaded by
   // policy.
   static bool ShouldUploadItemByPolicy(download::DownloadItem* item);
 
+  // Returns all scans supported.
+  static std::vector<DeepScanType> AllScans();
+
   // Scan the given |item|, with the given |trigger|. The result of the scanning
   // will be provided through |callback|. Take references to the owning
   // |download_service| and the |binary_upload_service| to upload to.
@@ -47,6 +59,14 @@ class DeepScanningRequest : public download::DownloadItem::Observer {
                       CheckDownloadRepeatingCallback callback,
                       DownloadProtectionService* download_service);
 
+  // Same as the previous constructor, but only allowing the scans listed in
+  // |allowed_scans| to run.
+  DeepScanningRequest(download::DownloadItem* item,
+                      DeepScanTrigger trigger,
+                      CheckDownloadRepeatingCallback callback,
+                      DownloadProtectionService* download_service,
+                      std::vector<DeepScanType> allowed_scans);
+
   ~DeepScanningRequest() override;
 
   // Begin the deep scanning request. This must be called on the UI thread.
@@ -73,6 +93,9 @@ class DeepScanningRequest : public download::DownloadItem::Observer {
   // Called to open the download. This is triggered by the timeout modal dialog.
   void OpenDownload();
 
+  // Whether the given |scan| is in the list of |allowed_scans_|.
+  bool ScanIsAllowed(DeepScanType scan);
+
   // The download item to scan. This is unowned, and could become nullptr if the
   // download is destroyed.
   download::DownloadItem* item_;
@@ -87,6 +110,9 @@ class DeepScanningRequest : public download::DownloadItem::Observer {
   // |download_service_| owns this class.
   DownloadProtectionService* download_service_;
 
+  // The scans allowed to be performed.
+  std::vector<DeepScanType> allowed_scans_;
+
   // The time when uploading starts.
   base::TimeTicks upload_start_time_;
 

chrome/browser/safe_browsing/download_protection/download_protection_service.cc

@@ -596,10 +596,11 @@ bool DownloadProtectionService::MaybeBeginFeedbackForDownload(
 void DownloadProtectionService::UploadForDeepScanning(
     download::DownloadItem* item,
     CheckDownloadRepeatingCallback callback,
-    DeepScanningRequest::DeepScanTrigger trigger) {
+    DeepScanningRequest::DeepScanTrigger trigger,
+    std::vector<DeepScanningRequest::DeepScanType> allowed_scans) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  auto request =
-      std::make_unique<DeepScanningRequest>(item, trigger, callback, this);
+  auto request = std::make_unique<DeepScanningRequest>(item, trigger, callback,
+                                                       this, allowed_scans);
   DeepScanningRequest* request_raw = request.get();
   auto insertion_result = deep_scanning_requests_.insert(
       std::make_pair(request_raw, std::move(request)));

chrome/browser/safe_browsing/download_protection/download_protection_service.h

@@ -193,11 +193,15 @@ class DownloadProtectionService {
   // Uploads |item| to Safe Browsing for deep scanning, using the upload
   // service attached to the profile |item| was downloaded in. This is
   // non-blocking, and the result we be provided through |callback|. |source| is
-  // used to identify the reason for deep scanning. This must be called on the
-  // UI thread.
-  void UploadForDeepScanning(download::DownloadItem* item,
-                             CheckDownloadRepeatingCallback callback,
-                             DeepScanningRequest::DeepScanTrigger trigger);
+  // used to identify the reason for deep scanning. Only the scan types listed
+  // in |allowed_scans| will be performed. This must be called on the UI
+  // thread.
+  void UploadForDeepScanning(
+      download::DownloadItem* item,
+      CheckDownloadRepeatingCallback callback,
+      DeepScanningRequest::DeepScanTrigger trigger,
+      std::vector<DeepScanningRequest::DeepScanType> allowed_scans =
+          DeepScanningRequest::AllScans());
 
  private:
   friend class PPAPIDownloadRequest;