Add a Feature for Enterprise CSD server and the functionality

This CL adds two new base::Features, SafeBrowsingEnterpriseCsd and
SafeBrowsingDisableConsumerCsdForEnterprise to control
whether using the Enterprise CSD server is enabled. It also makes two
behavior changes depending on these flags. When enabled:
- The consumer download ping is skipped
- The |csd| field in the upload request is populated with the download
  metadata.

Bug: 1165815
Change-Id: I00e3cc17ed56eff93803ab50232bb00497667afe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2626031
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843689}

chrome/browser/safe_browsing/cloud_content_scanning/test_binary_upload_service.cc

@@ -18,6 +18,7 @@ TestBinaryUploadService::TestBinaryUploadService()
 void TestBinaryUploadService::MaybeUploadForDeepScanning(
     std::unique_ptr<Request> request) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  last_request_ = request->content_analysis_request();
   content::GetUIThreadTaskRunner({})->PostTask(
       FROM_HERE, base::BindOnce(&Request::FinishRequest, std::move(request),
                                 saved_result_, saved_response_));

chrome/browser/safe_browsing/cloud_content_scanning/test_binary_upload_service.h

@@ -9,6 +9,7 @@
 
 #include "chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.h"
 #include "chrome/browser/safe_browsing/services_delegate.h"
+#include "components/enterprise/common/proto/connectors.pb.h"
 
 namespace safe_browsing {
 
@@ -22,9 +23,13 @@ class TestBinaryUploadService : public BinaryUploadService {
                    enterprise_connectors::ContentAnalysisResponse response);
 
   bool was_called() { return was_called_; }
+  const enterprise_connectors::ContentAnalysisRequest& last_request() {
+    return last_request_;
+  }
   void ClearWasCalled();
 
  private:
+  enterprise_connectors::ContentAnalysisRequest last_request_;
   Result saved_result_ = Result::UNKNOWN;
   enterprise_connectors::ContentAnalysisResponse saved_response_;
   bool was_called_ = false;

chrome/browser/safe_browsing/download_protection/deep_scanning_request.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/safe_browsing/cloud_content_scanning/file_analysis_request.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_service.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_util.h"
+#include "chrome/browser/safe_browsing/download_protection/download_request_maker.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/views/safe_browsing/deep_scanning_failure_modal_dialog.h"
@@ -32,6 +33,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/core/features.h"
+#include "components/safe_browsing/core/proto/csd.pb.h"
 #include "components/url_matcher/url_matcher.h"
 #include "content/public/browser/download_item_utils.h"
 
@@ -223,23 +225,14 @@ void DeepScanningRequest::Start() {
   Profile* profile = Profile::FromBrowserContext(
       content::DownloadItemUtils::GetBrowserContext(item_));
 
-  PrepareRequest(request.get(), profile);
-
-  upload_start_time_ = base::TimeTicks::Now();
-  BinaryUploadService* binary_upload_service =
-      download_service_->GetBinaryUploadService(profile);
-  if (binary_upload_service) {
-    binary_upload_service->MaybeUploadForDeepScanning(std::move(request));
-  } else {
-    OnScanComplete(BinaryUploadService::Result::UNKNOWN,
-                   enterprise_connectors::ContentAnalysisResponse());
-  }
-
   base::UmaHistogramEnumeration("SBClientDownload.DeepScanTrigger", trigger_);
+
+  PrepareRequest(std::move(request), profile);
 }
 
-void DeepScanningRequest::PrepareRequest(BinaryUploadService::Request* request,
-                                         Profile* profile) {
+void DeepScanningRequest::PrepareRequest(
+    std::unique_ptr<FileAnalysisRequest> request,
+    Profile* profile) {
   if (trigger_ == DeepScanTrigger::TRIGGER_POLICY)
     request->set_device_token(analysis_settings_.dm_token);
 
@@ -254,6 +247,38 @@ void DeepScanningRequest::PrepareRequest(BinaryUploadService::Request* request,
 
   for (const std::string& tag : analysis_settings_.tags)
     request->add_tag(tag);
+
+  if (base::FeatureList::IsEnabled(kSafeBrowsingEnterpriseCsd) &&
+      trigger_ == DeepScanTrigger::TRIGGER_POLICY) {
+    download_request_maker_ = std::make_unique<DownloadRequestMaker>(
+        new BinaryFeatureExtractor(), item_);
+    download_request_maker_->Start(
+        base::BindOnce(&DeepScanningRequest::OnDownloadRequestReady,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(request)));
+  } else {
+    OnDownloadRequestReady(std::move(request), nullptr);
+  }
+}
+
+void DeepScanningRequest::OnDownloadRequestReady(
+    std::unique_ptr<FileAnalysisRequest> deep_scan_request,
+    std::unique_ptr<ClientDownloadRequest> download_request) {
+  if (download_request) {
+    deep_scan_request->set_csd(*download_request);
+  }
+
+  upload_start_time_ = base::TimeTicks::Now();
+  Profile* profile = Profile::FromBrowserContext(
+      content::DownloadItemUtils::GetBrowserContext(item_));
+  BinaryUploadService* binary_upload_service =
+      download_service_->GetBinaryUploadService(profile);
+  if (binary_upload_service) {
+    binary_upload_service->MaybeUploadForDeepScanning(
+        std::move(deep_scan_request));
+  } else {
+    OnScanComplete(BinaryUploadService::Result::UNKNOWN,
+                   enterprise_connectors::ContentAnalysisResponse());
+  }
 }
 
 void DeepScanningRequest::OnScanComplete(

chrome/browser/safe_browsing/download_protection/deep_scanning_request.h

@@ -8,6 +8,7 @@
 #include "chrome/browser/enterprise/connectors/common.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.h"
+#include "chrome/browser/safe_browsing/cloud_content_scanning/file_analysis_request.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_util.h"
 #include "components/enterprise/common/proto/connectors.pb.h"
 
@@ -18,6 +19,7 @@ class DownloadItem;
 namespace safe_browsing {
 
 class DownloadProtectionService;
+class DownloadRequestMaker;
 
 // This class encapsulates the process of uploading a file to Safe Browsing for
 // deep scanning and reporting the result.
@@ -61,6 +63,12 @@ class DeepScanningRequest : public download::DownloadItem::Observer {
   void Start();
 
  private:
+  // Callback when the |download_request_maker_| is finished assembling the
+  // download metadata request.
+  void OnDownloadRequestReady(
+      std::unique_ptr<FileAnalysisRequest> deep_scan_request,
+      std::unique_ptr<ClientDownloadRequest> download_request);
+
   // Callbacks for when |binary_upload_service_| finishes uploading.
   void OnScanComplete(BinaryUploadService::Result result,
                       enterprise_connectors::ContentAnalysisResponse response);
@@ -82,7 +90,8 @@ class DeepScanningRequest : public download::DownloadItem::Observer {
   void OpenDownload();
 
   // Populates a request with the appropriate data depending on the used proto.
-  void PrepareRequest(BinaryUploadService::Request* request, Profile* profile);
+  void PrepareRequest(std::unique_ptr<FileAnalysisRequest> deep_scan_request,
+                      Profile* profile);
 
   // The download item to scan. This is unowned, and could become nullptr if the
   // download is destroyed.
@@ -104,6 +113,9 @@ class DeepScanningRequest : public download::DownloadItem::Observer {
   // The settings to apply to this scan.
   enterprise_connectors::AnalysisSettings analysis_settings_;
 
+  // Used to assemble the download metadata.
+  std::unique_ptr<DownloadRequestMaker> download_request_maker_;
+
   base::WeakPtrFactory<DeepScanningRequest> weak_ptr_factory_;
 };
 

chrome/browser/safe_browsing/download_protection/download_protection_service.cc

@@ -39,6 +39,7 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 #include "components/safe_browsing/core/common/safebrowsing_switches.h"
+#include "components/safe_browsing/core/features.h"
 #include "components/sessions/content/session_tab_helper.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_task_traits.h"
@@ -184,13 +185,25 @@ bool DownloadProtectionService::MaybeCheckClientDownload(
       content::DownloadItemUtils::GetBrowserContext(item));
   bool safe_browsing_enabled =
       profile && IsSafeBrowsingEnabled(*profile->GetPrefs());
+  auto settings = DeepScanningRequest::ShouldUploadBinary(item);
+
+  if (base::FeatureList::IsEnabled(kSafeBrowsingEnterpriseCsd) &&
+      base::FeatureList::IsEnabled(
+          kSafeBrowsingDisableConsumerCsdForEnterprise) &&
+      settings.has_value()) {
+    UploadForDeepScanning(item, std::move(callback),
+                          DeepScanningRequest::DeepScanTrigger::TRIGGER_POLICY,
+                          std::move(settings.value()));
+    return true;
+  }
 
   if (safe_browsing_enabled) {
     CheckClientDownload(item, std::move(callback));
     return true;
   }
 
-  auto settings = DeepScanningRequest::ShouldUploadBinary(item);
+  // TODO(https://crbug.com/1165815): Remove this check once the enterpise CSD
+  // check has fully launched.
   if (settings.has_value()) {
     UploadForDeepScanning(item, std::move(callback),
                           DeepScanningRequest::DeepScanTrigger::TRIGGER_POLICY,

chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc

@@ -4043,4 +4043,119 @@ TEST_P(DeepScanningDownloadTest, SafeVerdictPrecedence) {
 
 INSTANTIATE_TEST_SUITE_P(, DeepScanningDownloadTest, testing::Bool());
 
+// Test fixture with the enterprise CSD checks either enabled or disabled.
+class EnterpriseCsdDownloadTest : public DownloadProtectionServiceTestBase,
+                                  public ::testing::WithParamInterface<bool> {
+ public:
+  EnterpriseCsdDownloadTest() {
+    // Enable the feature early to prevent race condition trying to access
+    // the enabled features set.  This happens for example when the history
+    // service is started below.
+    if (flag_enabled())
+      EnableFeatures({kSafeBrowsingEnterpriseCsd,
+                      kSafeBrowsingDisableConsumerCsdForEnterprise});
+    else
+      DisableFeatures({kSafeBrowsingEnterpriseCsd,
+                       kSafeBrowsingDisableConsumerCsdForEnterprise});
+  }
+
+  bool flag_enabled() const { return GetParam(); }
+};
+
+TEST_P(EnterpriseCsdDownloadTest, SkipsConsumerCsdWhenEnabled) {
+  NiceMockDownloadItem item;
+  PrepareBasicDownloadItem(&item, {"http://www.evil.com/a.exe"},  // url_chain
+                           "http://www.google.com/",              // referrer
+                           FILE_PATH_LITERAL("a.tmp"),            // tmp_path
+                           FILE_PATH_LITERAL("a.exe"));           // final_path
+  content::DownloadItemUtils::AttachInfo(&item, profile(), nullptr);
+
+  if (!flag_enabled()) {
+    EXPECT_CALL(*sb_service_->mock_database_manager(),
+                MatchDownloadWhitelistUrl(_))
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*binary_feature_extractor_.get(), CheckSignature(tmp_path_, _));
+    EXPECT_CALL(*binary_feature_extractor_.get(),
+                ExtractImageFeatures(
+                    tmp_path_, BinaryFeatureExtractor::kDefaultOptions, _, _));
+  }
+
+  SetAnalysisConnector(profile()->GetPrefs(),
+                       enterprise_connectors::FILE_DOWNLOADED, R"(
+                         {
+                           "service_provider": "google",
+                           "enable": [
+                             {"url_list": ["*"], "tags": ["malware"]}
+                           ]
+                         })");
+
+  TestBinaryUploadService* test_upload_service =
+      static_cast<TestBinaryUploadService*>(
+          BinaryUploadServiceFactory::GetForProfile(profile()));
+
+  PrepareResponse(ClientDownloadResponse::SAFE, net::HTTP_OK, net::OK);
+  test_upload_service->SetResponse(
+      BinaryUploadService::Result::SUCCESS,
+      enterprise_connectors::ContentAnalysisResponse());
+
+  RunLoop run_loop;
+  download_service_->MaybeCheckClientDownload(
+      &item,
+      base::BindRepeating(&DownloadProtectionServiceTest::CheckDoneCallback,
+                          base::Unretained(this), run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_EQ(HasClientDownloadRequest(), !flag_enabled());
+  EXPECT_TRUE(test_upload_service->was_called());
+}
+
+TEST_P(EnterpriseCsdDownloadTest, PopulatesCsdFieldWhenEnabled) {
+  NiceMockDownloadItem item;
+  PrepareBasicDownloadItem(&item, {"http://www.evil.com/a.exe"},  // url_chain
+                           "http://www.google.com/",              // referrer
+                           FILE_PATH_LITERAL("a.tmp"),            // tmp_path
+                           FILE_PATH_LITERAL("a.exe"));           // final_path
+  content::DownloadItemUtils::AttachInfo(&item, profile(), nullptr);
+  if (!flag_enabled()) {
+    EXPECT_CALL(*sb_service_->mock_database_manager(),
+                MatchDownloadWhitelistUrl(_))
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*binary_feature_extractor_.get(), CheckSignature(tmp_path_, _));
+    EXPECT_CALL(*binary_feature_extractor_.get(),
+                ExtractImageFeatures(
+                    tmp_path_, BinaryFeatureExtractor::kDefaultOptions, _, _));
+  }
+
+  SetAnalysisConnector(profile()->GetPrefs(),
+                       enterprise_connectors::FILE_DOWNLOADED,
+                       R"({
+                           "service_provider": "google",
+                           "enable": [
+                             {"url_list": ["*"], "tags": ["malware"]}
+                           ]
+                         })");
+
+  TestBinaryUploadService* test_upload_service =
+      static_cast<TestBinaryUploadService*>(
+          BinaryUploadServiceFactory::GetForProfile(profile()));
+
+  PrepareResponse(ClientDownloadResponse::SAFE, net::HTTP_OK, net::OK);
+  test_upload_service->SetResponse(
+      BinaryUploadService::Result::SUCCESS,
+      enterprise_connectors::ContentAnalysisResponse());
+
+  RunLoop run_loop;
+  download_service_->MaybeCheckClientDownload(
+      &item,
+      base::BindRepeating(&DownloadProtectionServiceTest::CheckDoneCallback,
+                          base::Unretained(this), run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_TRUE(test_upload_service->was_called());
+  EXPECT_EQ(test_upload_service->last_request().request_data().has_csd(),
+            flag_enabled());
+}
+
+INSTANTIATE_TEST_SUITE_P(, EnterpriseCsdDownloadTest, testing::Bool());
+
 }  // namespace safe_browsing

components/safe_browsing/core/features.cc

@@ -85,6 +85,13 @@ const base::Feature kPasswordProtectionForSignedInUsers{
 const base::Feature kPromptAppForDeepScanning{
     "SafeBrowsingPromptAppForDeepScanning", base::FEATURE_ENABLED_BY_DEFAULT};
 
+const base::Feature kSafeBrowsingEnterpriseCsd{
+    "SafeBrowsingEnterpriseCsd", base::FEATURE_DISABLED_BY_DEFAULT};
+
+const base::Feature kSafeBrowsingDisableConsumerCsdForEnterprise{
+    "SafeBrowsingDisableConsumerCsdForEnterprise",
+    base::FEATURE_DISABLED_BY_DEFAULT};
+
 const base::Feature kRealTimeUrlLookupEnabled{
     "SafeBrowsingRealTimeUrlLookupEnabled", base::FEATURE_ENABLED_BY_DEFAULT};
 

components/safe_browsing/core/features.h

@@ -69,6 +69,14 @@ extern const base::Feature kPasswordProtectionForSignedInUsers;
 // Controls whether Chrome prompts Advanced Protection users for deep scanning.
 extern const base::Feature kPromptAppForDeepScanning;
 
+// Controls whether we are performing enterprise download checks for users with
+// the appropriate policies enabled.
+extern const base::Feature kSafeBrowsingEnterpriseCsd;
+
+// Controls whether we are disabling consumer download checks for users using
+// the enterprise download checks.
+extern const base::Feature kSafeBrowsingDisableConsumerCsdForEnterprise;
+
 // Controls whether Safe Browsing uses separate NetworkContexts for each
 // profile.
 extern const base::Feature kSafeBrowsingSeparateNetworkContexts;