Refactor content analysis in CheckClientDownloadRequest to its own class

There are two problems with the CheckClientDownloadRequest that this CL
takes steps towards resolving. First, the control flow of the
CheckClientDownloadRequest is very difficult to follow due to a large
number of private helper methods. Secondly, it uses several techniques
to prevent use-after-free (ref-counting, weak pointers, and cancellable
tasks). This CL encapsulates the logic that was in
CheckClientDownloadRequest that was related to analyzing the content
of the file into a singly-owned class, so that those two concerns can
be more easily addressed.

TODO: The FileAnalyzer still needs to be tested for RAR files.

Bug: 889986
Change-Id: I81c02bd3491c116a949dc959f2f02208511209bd
Reviewed-on: https://chromium-review.googlesource.com/c/1272275
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600785}

chrome/browser/safe_browsing/BUILD.gn

@@ -150,6 +150,8 @@ static_library("safe_browsing") {
         "download_protection/download_protection_util.h",
         "download_protection/download_url_sb_client.cc",
         "download_protection/download_url_sb_client.h",
+        "download_protection/file_analyzer.cc",
+        "download_protection/file_analyzer.h",
         "download_protection/path_sanitizer.cc",
         "download_protection/path_sanitizer.h",
         "download_protection/ppapi_download_request.cc",

chrome/browser/safe_browsing/download_protection/check_client_download_request.h

@@ -19,6 +19,7 @@
 #include "base/task/cancelable_task_tracker.h"
 #include "build/build_config.h"
 #include "chrome/browser/safe_browsing/download_protection/download_protection_util.h"
+#include "chrome/browser/safe_browsing/download_protection/file_analyzer.h"
 #include "chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h"
 #include "chrome/browser/safe_browsing/ui_manager.h"
 #include "chrome/common/safe_browsing/binary_feature_extractor.h"
@@ -79,7 +80,7 @@ class CheckClientDownloadRequest
   // Performs file feature extraction and SafeBrowsing ping for downloads that
   // don't match the URL whitelist.
   void AnalyzeFile();
-  void OnFileFeatureExtractionDone();
+  void OnFileFeatureExtractionDone(FileAnalyzer::Results results);
   void StartExtractFileFeatures();
   void ExtractFileFeatures(const base::FilePath& file_path);
   void StartExtractRarFeatures();
@@ -114,8 +115,6 @@ class CheckClientDownloadRequest
   bool CertificateChainIsWhitelisted(
       const ClientDownloadRequest_CertificateChain& chain);
 
-  enum class ArchiveValid { UNSET, VALID, INVALID };
-
   // The DownloadItem we are checking. Will be NULL if the request has been
   // canceled. Must be accessed only on UI thread.
   download::DownloadItem* item_;
@@ -129,7 +128,7 @@ class CheckClientDownloadRequest
   GURL tab_referrer_url_;
 
   bool archived_executable_;
-  ArchiveValid archive_is_valid_;
+  FileAnalyzer::ArchiveValid archive_is_valid_;
 
 #if defined(OS_MACOSX)
   std::unique_ptr<std::vector<uint8_t>> disk_image_signature_;
@@ -148,14 +147,7 @@ class CheckClientDownloadRequest
   scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
   const bool pingback_enabled_;
   std::unique_ptr<network::SimpleURLLoader> loader_;
-  scoped_refptr<SandboxedRarAnalyzer> rar_analyzer_;
-  scoped_refptr<SandboxedZipAnalyzer> zip_analyzer_;
-  base::TimeTicks rar_analysis_start_time_;
-  base::TimeTicks zip_analysis_start_time_;
-#if defined(OS_MACOSX)
-  scoped_refptr<SandboxedDMGAnalyzer> dmg_analyzer_;
-  base::TimeTicks dmg_analysis_start_time_;
-#endif
+  std::unique_ptr<FileAnalyzer> file_analyzer_;
   bool finished_;
   ClientDownloadRequest::DownloadType type_;
   std::string client_download_request_data_;

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;
-
-  const int max_to_try = 12;
-  for (int i = 0; i < max_to_try; i++) {
-    src.Add();
-  }
-
-  // First check against the value set in .asciipb, which is currently 10
-  // If that is raised above |max_to_try|, raise the latter.
-  dest.Clear();
-  CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
-  EXPECT_EQ(10, dest.size());
-
-  SetMaxArchivedBinariesToReport(2);
-  dest.Clear();
-  CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
-  EXPECT_EQ(2, dest.size());
-
-  SetMaxArchivedBinariesToReport(100000);
-  dest.Clear();
-  CheckClientDownloadRequest::CopyArchivedBinaries(src, &dest);
-  EXPECT_EQ(max_to_try, dest.size());
-}
-
-}  // namespace safe_browsing

chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc

@@ -44,6 +44,7 @@
 #include "chrome/common/extensions/api/safe_browsing_private.h"
 #include "chrome/common/safe_browsing/binary_feature_extractor.h"
 #include "chrome/common/safe_browsing/file_type_policies_test_util.h"
+#include "chrome/common/safe_browsing/mock_binary_feature_extractor.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/download/public/common/download_danger_type.h"
@@ -173,25 +174,6 @@ class FakeSafeBrowsingService : public TestSafeBrowsingService {
   DISALLOW_COPY_AND_ASSIGN(FakeSafeBrowsingService);
 };
 
-class MockBinaryFeatureExtractor : public BinaryFeatureExtractor {
- public:
-  MockBinaryFeatureExtractor() {}
-  MOCK_METHOD2(CheckSignature,
-               void(const base::FilePath&,
-                    ClientDownloadRequest_SignatureInfo*));
-  MOCK_METHOD4(ExtractImageFeatures,
-               bool(const base::FilePath&,
-                    ExtractHeadersOption,
-                    ClientDownloadRequest_ImageHeaders*,
-                    google::protobuf::RepeatedPtrField<std::string>*));
-
- protected:
-  ~MockBinaryFeatureExtractor() override {}
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(MockBinaryFeatureExtractor);
-};
-
 using NiceMockDownloadItem = NiceMock<download::MockDownloadItem>;
 
 }  // namespace
@@ -1622,8 +1604,18 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadReportLargeDmg) {
       unsigned_dmg,                                             // tmp_path
       temp_dir_.GetPath().Append(FILE_PATH_LITERAL("a.dmg")));  // final_path
 
-  EXPECT_CALL(item, GetTotalBytes())
-      .WillRepeatedly(Return(std::numeric_limits<int64_t>::max()));
+  // Set the max file size to unpack to 0, so that this DMG is now "too large"
+  std::unique_ptr<DownloadFileTypeConfig> config = policies_.DuplicateConfig();
+  for (int i = 0; i < config->file_types_size(); i++) {
+    if (config->file_types(i).extension() == "dmg") {
+      for (int j = 0; j < config->file_types(i).platform_settings_size(); j++) {
+        config->mutable_file_types(i)
+            ->mutable_platform_settings(j)
+            ->set_max_file_size_to_analyze(0);
+      }
+    }
+  }
+  policies_.SwapConfig(config);
 
   RunLoop run_loop;
   download_service_->CheckClientDownload(

chrome/browser/safe_browsing/download_protection/file_analyzer.h

+// Copyright 2018 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.
+
+#ifndef CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_FILE_ANALYZER_H_
+#define CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_FILE_ANALYZER_H_
+
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "build/build_config.h"
+#include "chrome/common/safe_browsing/binary_feature_extractor.h"
+#include "chrome/services/file_util/public/cpp/sandboxed_rar_analyzer.h"
+#include "chrome/services/file_util/public/cpp/sandboxed_zip_analyzer.h"
+#include "components/safe_browsing/proto/csd.pb.h"
+#include "third_party/protobuf/src/google/protobuf/repeated_field.h"
+
+#if defined(OS_MACOSX)
+#include "chrome/common/safe_browsing/disk_image_type_sniffer_mac.h"
+#include "chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.h"
+#endif
+
+namespace safe_browsing {
+
+// This class does the file content analysis for a user download, extracting the
+// features that will be sent to the SB backend. This class lives on the UI
+// thread, which is where the result callback will be invoked.
+class FileAnalyzer {
+ public:
+  enum class ArchiveValid { UNSET, VALID, INVALID };
+
+  // This struct holds the possible features extracted from a file.
+  struct Results {
+    Results();
+    Results(const Results& other);
+    ~Results();
+
+    // When analyzing a ZIP or RAR, the type becomes clarified by content
+    // inspection (does it contain binaries/archives?). So we return a type.
+    ClientDownloadRequest::DownloadType type;
+
+    // For archive files, whether the archive is valid. Has unspecified contents
+    // for non-archive files.
+    ArchiveValid archive_is_valid;
+
+    // For archive files, whether the archive contains an executable. Has
+    // unspecified contents for non-archive files.
+    bool archived_executable;
+
+    // For archive files, whether the archive contains an archive. Has
+    // unspecified contents for non-archive files.
+    bool archived_archive;
+
+    // For archive files, the features extracted from each contained
+    // archive/binary.
+    google::protobuf::RepeatedPtrField<ClientDownloadRequest::ArchivedBinary>
+        archived_binaries;
+
+    // For executables, information about the signature of the executable.
+    ClientDownloadRequest::SignatureInfo signature_info;
+
+    // For executables, information about the file headers.
+    ClientDownloadRequest::ImageHeaders image_headers;
+
+#if defined(OS_MACOSX)
+    // For DMG files, the signature of the DMG.
+    std::vector<uint8_t> disk_image_signature;
+
+    // For DMG files, any detached code signatures in the DMG.
+    google::protobuf::RepeatedPtrField<
+        ClientDownloadRequest::DetachedCodeSignature>
+        detached_code_signatures;
+#endif
+  };
+
+  explicit FileAnalyzer(
+      scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor);
+  ~FileAnalyzer();
+  void Start(const base::FilePath& target_path,
+             const base::FilePath& tmp_path,
+             base::OnceCallback<void(Results)> callback);
+
+ private:
+  void StartExtractFileFeatures();
+  void OnFileAnalysisFinished(FileAnalyzer::Results results);
+
+  void StartExtractZipFeatures();
+  void OnZipAnalysisFinished(const ArchiveAnalyzerResults& archive_results);
+
+  void StartExtractRarFeatures();
+  void OnRarAnalysisFinished(const ArchiveAnalyzerResults& archive_results);
+
+#if defined(OS_MACOSX)
+  void StartExtractDmgFeatures();
+  void ExtractFileOrDmgFeatures(bool download_file_has_koly_signature);
+  void OnDmgAnalysisFinished(
+      const safe_browsing::ArchiveAnalyzerResults& archive_results);
+#endif
+
+  base::FilePath target_path_;
+  base::FilePath tmp_path_;
+  scoped_refptr<BinaryFeatureExtractor> binary_feature_extractor_;
+  base::OnceCallback<void(Results)> callback_;
+  Results results_;
+
+  scoped_refptr<SandboxedZipAnalyzer> zip_analyzer_;
+  base::TimeTicks zip_analysis_start_time_;
+
+  scoped_refptr<SandboxedRarAnalyzer> rar_analyzer_;
+  base::TimeTicks rar_analysis_start_time_;
+
+#if defined(OS_MACOSX)
+  scoped_refptr<SandboxedDMGAnalyzer> dmg_analyzer_;
+  base::TimeTicks dmg_analysis_start_time_;
+#endif
+
+  base::WeakPtrFactory<FileAnalyzer> weakptr_factory_;
+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_BROWSER_SAFE_BROWSING_DOWNLOAD_PROTECTION_FILE_ANALYZER_H_

chrome/common/safe_browsing/BUILD.gn

@@ -100,6 +100,20 @@ if (safe_browsing_mode == 1) {
       "//base:base",
     ]
   }
+
+  source_set("mock_binary_feature_extractor") {
+    testonly = true
+
+    sources = [
+      "mock_binary_feature_extractor.cc",
+      "mock_binary_feature_extractor.h",
+    ]
+
+    deps = [
+      ":safe_browsing",
+      "//testing/gmock",
+    ]
+  }
 }
 
 source_set("safe_browsing") {

chrome/common/safe_browsing/mock_binary_feature_extractor.cc

+// Copyright 2018 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/common/safe_browsing/mock_binary_feature_extractor.h"
+
+namespace safe_browsing {
+
+MockBinaryFeatureExtractor::MockBinaryFeatureExtractor() {}
+
+MockBinaryFeatureExtractor::~MockBinaryFeatureExtractor() {}
+
+}  // namespace safe_browsing

chrome/common/safe_browsing/mock_binary_feature_extractor.h

+// Copyright 2018 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.
+
+#ifndef CHROME_COMMON_SAFE_BROWSING_MOCK_BINARY_FEATURE_EXTRACTOR_H_
+#define CHROME_COMMON_SAFE_BROWSING_MOCK_BINARY_FEATURE_EXTRACTOR_H_
+
+#include "chrome/common/safe_browsing/binary_feature_extractor.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace safe_browsing {
+
+class MockBinaryFeatureExtractor : public BinaryFeatureExtractor {
+ public:
+  MockBinaryFeatureExtractor();
+  MOCK_METHOD2(CheckSignature,
+               void(const base::FilePath&,
+                    ClientDownloadRequest_SignatureInfo*));
+  MOCK_METHOD4(ExtractImageFeatures,
+               bool(const base::FilePath&,
+                    ExtractHeadersOption,
+                    ClientDownloadRequest_ImageHeaders*,
+                    google::protobuf::RepeatedPtrField<std::string>*));
+
+ protected:
+  ~MockBinaryFeatureExtractor() override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockBinaryFeatureExtractor);
+};
+
+}  // namespace safe_browsing
+
+#endif  // CHROME_COMMON_SAFE_BROWSING_MOCK_BINARY_FEATURE_EXTRACTOR_H_

chrome/test/BUILD.gn

@@ -3921,10 +3921,10 @@ 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/download_feedback_service_unittest.cc",
       "../browser/safe_browsing/download_protection/download_feedback_unittest.cc",
       "../browser/safe_browsing/download_protection/download_protection_service_unittest.cc",
+      "../browser/safe_browsing/download_protection/file_analyzer_unittest.cc",
       "../browser/safe_browsing/download_protection/path_sanitizer_unittest.cc",
       "../browser/safe_browsing/download_protection/two_phase_uploader_unittest.cc",
       "../browser/safe_browsing/incident_reporting/binary_integrity_analyzer_mac_unittest.cc",
@@ -3975,6 +3975,7 @@ test("unit_tests") {
     ]
     deps += [
       ":test_proto",
+      "../common/safe_browsing:mock_binary_feature_extractor",
       "//chrome/services/file_util/public/cpp:unit_tests",
       "//components/safe_browsing:ping_manager_unittest",
       "//components/safe_browsing/browser:unittests",