Cap the number of archived_binaries added to download pings.

Make it configurable via FileTypePolicies component update.

Bug: 774664
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ic4e3691716c08419546c52fb0f703f95a4762380
Reviewed-on: https://chromium-review.googlesource.com/767663Reviewed-by: Varun Khaneja <vakh@chromium.org>
Commit-Queue: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516954}

chrome/browser/resources/safe_browsing/download_file_types.asciipb

@@ -8,8 +8,9 @@
 ##
 ## Top level settings
 ##
-version_id: 14
+version_id: 15
 sampled_ping_probability: 0.01
+max_archived_binaries_to_report: 10
 default_file_type {
   uma_value: 18
   ping_setting: SAMPLED_PING

chrome/browser/safe_browsing/download_protection/check_client_download_request.cc

@@ -485,6 +485,20 @@ void CheckClientDownloadRequest::StartExtractZipFeatures() {
   analyzer_->Start();
 }
 
+// static
+void CheckClientDownloadRequest::CopyArchivedBinaries(
+    const ArchivedBinaries& src_binaries,
+    ArchivedBinaries* dest_binaries) {
+  // Limit the number of entries so we don't clog the backend.
+  // We can expand this limit by pushing a new download_file_types update.
+  int limit = FileTypePolicies::GetInstance()->GetMaxArchivedBinariesToReport();
+
+  dest_binaries->Clear();
+  for (int i = 0; i < limit && i < src_binaries.size(); i++) {
+    *dest_binaries->Add() = src_binaries[i];
+  }
+}
+
 void CheckClientDownloadRequest::OnZipAnalysisFinished(
     const ArchiveAnalyzerResults& results) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -502,7 +516,7 @@ void CheckClientDownloadRequest::OnZipAnalysisFinished(
   archive_is_valid_ =
       (results.success ? ArchiveValid::VALID : ArchiveValid::INVALID);
   archived_executable_ = results.has_executable;
-  archived_binary_.CopyFrom(results.archived_binary);
+  CopyArchivedBinaries(results.archived_binary, &archived_binary_);
   DVLOG(1) << "Zip analysis finished for " << item_->GetFullPath().value()
            << ", has_executable=" << results.has_executable
            << ", has_archive=" << results.has_archive
@@ -658,7 +672,6 @@ void CheckClientDownloadRequest::CheckUrlAgainstWhitelist() {
   }
 
   const GURL& url = url_chain_.back();
-  // TODO(vakh): This may acquire a lock on the SB DB on the IO thread.
   if (url.is_valid() && database_manager_->MatchDownloadWhitelistUrl(url)) {
     DVLOG(2) << url << " is on the download whitelist.";
     RecordCountOfWhitelistedDownload(URL_WHITELIST);

chrome/browser/safe_browsing/download_protection/check_client_download_request.h

@@ -13,6 +13,7 @@
 #include "base/callback.h"
 #include "base/callback_list.h"
 #include "base/files/file_path.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/supports_user_data.h"
 #include "base/task/cancelable_task_tracker.h"
@@ -66,6 +67,9 @@ class CheckClientDownloadRequest
   friend struct BrowserThread::DeleteOnThread<BrowserThread::UI>;
   friend class base::DeleteHelper<CheckClientDownloadRequest>;
 
+  using ArchivedBinaries =
+      google::protobuf::RepeatedPtrField<ClientDownloadRequest_ArchivedBinary>;
+
   ~CheckClientDownloadRequest() override;
   // Performs file feature extraction and SafeBrowsing ping for downloads that
   // don't match the URL whitelist.
@@ -76,6 +80,9 @@ class CheckClientDownloadRequest
   void StartExtractZipFeatures();
   void OnZipAnalysisFinished(const ArchiveAnalyzerResults& results);
 
+  static void CopyArchivedBinaries(const ArchivedBinaries& src_binaries,
+                                   ArchivedBinaries* dest_binaries);
+
 #if defined(OS_MACOSX)
   void StartExtractDmgFeatures();
   void ExtractFileOrDmgFeatures(bool download_file_has_koly_signature);
@@ -123,8 +130,7 @@ class CheckClientDownloadRequest
 
   ClientDownloadRequest_SignatureInfo signature_info_;
   std::unique_ptr<ClientDownloadRequest_ImageHeaders> image_headers_;
-  google::protobuf::RepeatedPtrField<ClientDownloadRequest_ArchivedBinary>
-      archived_binary_;
+  ArchivedBinaries archived_binary_;
   CheckDownloadCallback callback_;
   // Will be NULL if the request has been canceled.
   DownloadProtectionService* service_;
@@ -154,6 +160,9 @@ class CheckClientDownloadRequest
   base::CancelableTaskTracker cancelable_task_tracker_;
   base::WeakPtrFactory<CheckClientDownloadRequest> weakptr_factory_;
 
+  FRIEND_TEST_ALL_PREFIXES(CheckClientDownloadRequestTest,
+                           CheckLimitArchivedExtensions);
+
   DISALLOW_COPY_AND_ASSIGN(CheckClientDownloadRequest);
 };
 

chrome/browser/safe_browsing/download_protection/check_client_download_request_unittest.cc

+// Copyright 2017 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/download_protection/check_client_download_request.h"
+
+#include "base/memory/ref_counted.h"
+#include "build/build_config.h"
+#include "chrome/common/safe_browsing/file_type_policies_test_util.h"
+#include "content/public/test/test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace safe_browsing {
+
+// TODO(nparker): Move some testing from download_protection_service_unittest.cc
+// to this file, and add speicific tests here.
+
+class CheckClientDownloadRequestTest : public testing::Test {
+ public:
+  CheckClientDownloadRequestTest() {}
+
+ protected:
+  void SetUp() override {}
+
+  void TearDown() override {}
+
+  void SetMaxArchivedBinariesToReport(uint64_t target_limit) {
+    std::unique_ptr<DownloadFileTypeConfig> config =
+        policies_.DuplicateConfig();
+    config->set_max_archived_binaries_to_report(target_limit);
+    policies_.SwapConfig(config);
+  }
+
+ protected:
+  // This will effectivly mask the global Singleton while this is in scope.
+  FileTypePoliciesTestOverlay policies_;
+};
+
+TEST_F(CheckClientDownloadRequestTest, CheckLimitArchivedExtensions) {
+  CheckClientDownloadRequest::ArchivedBinaries src, dest;
+
+  for (int i = 0; i < 4; i++) {
+    src.Add();
+  }
+
+  SetMaxArchivedBinariesToReport(0);
+  dest.Clear();
+  CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
+  EXPECT_EQ(dest.size(), 0);
+
+  SetMaxArchivedBinariesToReport(2);
+  dest.Clear();
+  CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
+  EXPECT_EQ(dest.size(), 2);
+}
+
+}  // namespace safe_browsing

chrome/common/safe_browsing/download_file_types.proto

@@ -60,11 +60,15 @@ message DownloadFileType {
   repeated PlatformSettings platform_settings = 5;
 };
 
-// Next id: 5
+// Next id: 6
 message DownloadFileTypeConfig {
   // All required
   optional uint32 version_id = 1;
   optional float sampled_ping_probability = 2;
   repeated DownloadFileType file_types = 3;
   optional DownloadFileType default_file_type = 4;
+
+  // Limits on repeated fields in the ClientDownloadRequest (i.e. the
+  // download ping). Limits are per-ping.
+  optional uint64 max_archived_binaries_to_report = 5;
 }

chrome/common/safe_browsing/file_type_policies.cc

@@ -261,4 +261,14 @@ uint64_t FileTypePolicies::GetMaxFileSizeToAnalyze(
       .max_file_size_to_analyze();
 }
 
+uint64_t FileTypePolicies::GetMaxArchivedBinariesToReport() const {
+  AutoLock lock(lock_);
+  if (!config_ || !config_->has_max_archived_binaries_to_report()) {
+    // The resource bundle may be corrupted.
+    DCHECK(false);
+    return 10;  // reasonable default
+  }
+  return config_->max_archived_binaries_to_report();
+}
+
 }  // namespace safe_browsing

chrome/common/safe_browsing/file_type_policies.h

@@ -82,6 +82,9 @@ class FileTypePolicies {
   // supported for the given file extension.
   uint64_t GetMaxFileSizeToAnalyze(const std::string& ascii_ext) const;
 
+  // Return max number of archived_binaries we should add to a download ping.
+  uint64_t GetMaxArchivedBinariesToReport() const;
+
  protected:
   // Creator must call one of Populate* before calling other methods.
   FileTypePolicies();

chrome/test/BUILD.gn

@@ -3440,6 +3440,7 @@ test("unit_tests") {
       "../browser/safe_browsing/client_side_detection_host_unittest.cc",
       "../browser/safe_browsing/client_side_detection_service_unittest.cc",
       "../browser/safe_browsing/client_side_model_loader_unittest.cc",
+      "../browser/safe_browsing/download_protection/check_client_download_request_unittest.cc",
       "../browser/safe_browsing/download_protection/disk_image_type_sniffer_mac_unittest.cc",
       "../browser/safe_browsing/download_protection/download_feedback_service_unittest.cc",
       "../browser/safe_browsing/download_protection/download_feedback_unittest.cc",