[Mac] Fix crash in safe_browsing::dmg::AnalayzeDMGFile()

This could occur if a detached code signature file is shorter than its
typical prologue.

Bug: 826761
Test: unit_tests --gtest_filter=DMGAnalyzerTest.InvalidDetachedCodeSignature
Change-Id: Ic11b12eb9ce7d7b787b7668af43e2c6ddb3abd52
Reviewed-on: https://chromium-review.googlesource.com/1114925
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570530}

chrome/test/BUILD.gn

@@ -3847,6 +3847,7 @@ test("unit_tests") {
       "../renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc",
       "../renderer/safe_browsing/phishing_url_feature_extractor_unittest.cc",
       "../renderer/safe_browsing/scorer_unittest.cc",
+      "../utility/safe_browsing/mac/dmg_analyzer_unittest.cc",
       "../utility/safe_browsing/mac/dmg_test_utils.cc",
       "../utility/safe_browsing/mac/dmg_test_utils.h",
       "../utility/safe_browsing/mac/hfs_unittest.cc",

chrome/utility/safe_browsing/mac/dmg_analyzer.cc

@@ -126,38 +126,46 @@ constexpr uint8_t kDERPKCS7SignedData[] = {0x30, 0x80, 0x06, 0x09, 0x2a,
 }  // namespace
 
 void AnalyzeDMGFile(base::File dmg_file, ArchiveAnalyzerResults* results) {
-  MachOFeatureExtractor feature_extractor;
-  results->success = false;
-
   FileReadStream read_stream(dmg_file.GetPlatformFile());
   DMGIterator iterator(&read_stream);
-  if (!iterator.Open())
+  AnalyzeDMGFile(&iterator, results);
+}
+
+void AnalyzeDMGFile(DMGIterator* iterator, ArchiveAnalyzerResults* results) {
+  results->success = false;
+
+  if (!iterator->Open())
     return;
 
-  results->signature_blob = iterator.GetCodeSignature();
+  MachOFeatureExtractor feature_extractor;
 
-  while (iterator.Next()) {
-    std::unique_ptr<ReadStream> stream = iterator.GetReadStream();
+  results->signature_blob = iterator->GetCodeSignature();
+
+  while (iterator->Next()) {
+    std::unique_ptr<ReadStream> stream = iterator->GetReadStream();
     if (!stream)
       continue;
 
-    std::string path = base::UTF16ToUTF8(iterator.GetPath());
+    std::string path = base::UTF16ToUTF8(iterator->GetPath());
 
     bool is_detached_code_signature_file = base::EndsWith(
         path, "_CodeSignature/CodeSignature", base::CompareCase::SENSITIVE);
 
     if (is_detached_code_signature_file) {
+      results->has_executable = true;
+
       std::vector<uint8_t> signature_contents;
       if (!ReadEntireStream(stream.get(), &signature_contents))
         continue;
 
+      if (signature_contents.size() < base::size(kDERPKCS7SignedData))
+        continue;
+
       if (memcmp(kDERPKCS7SignedData, signature_contents.data(),
                  base::size(kDERPKCS7SignedData)) != 0) {
         continue;
       }
 
-      results->has_executable = true;
-
       ClientDownloadRequest_DetachedCodeSignature* detached_signature =
           results->detached_code_signatures.Add();
       detached_signature->set_file_name(path);

chrome/utility/safe_browsing/mac/dmg_analyzer.h

@@ -13,8 +13,15 @@ struct ArchiveAnalyzerResults;
 
 namespace dmg {
 
+class DMGIterator;
+
+// Analyzes the given |dmg_file| for executable content and places the results
+// in |results|.
 void AnalyzeDMGFile(base::File dmg_file, ArchiveAnalyzerResults* results);
 
+// Helper function exposed for testing. Called by the above overload.
+void AnalyzeDMGFile(DMGIterator* iterator, ArchiveAnalyzerResults* results);
+
 }  // namespace dmg
 }  // namespace safe_browsing
 

chrome/utility/safe_browsing/mac/dmg_analyzer_unittest.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/utility/safe_browsing/mac/dmg_analyzer.h"
+
+#include <string>
+#include <vector>
+
+#include "base/base_paths.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/common/safe_browsing/archive_analyzer_results.h"
+#include "chrome/utility/safe_browsing/mac/dmg_iterator.h"
+#include "chrome/utility/safe_browsing/mac/read_stream.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace safe_browsing {
+namespace dmg {
+namespace {
+
+class MockDMGIterator : public DMGIterator {
+ public:
+  struct Entry {
+    std::string path;
+    std::vector<uint8_t> data;
+  };
+
+  using FileList = std::vector<Entry>;
+
+  MockDMGIterator(bool open_ok, const FileList& file_entries)
+      : MockDMGIterator(open_ok, file_entries, std::vector<uint8_t>()) {}
+  MockDMGIterator(bool open_ok,
+                  const FileList& file_entries,
+                  const std::vector<uint8_t>& code_signature)
+      : DMGIterator(nullptr),
+        open_ok_(open_ok),
+        entries_(file_entries),
+        index_(-1),
+        code_signature_(code_signature) {}
+
+  bool Open() override { return open_ok_; }
+
+  const std::vector<uint8_t>& GetCodeSignature() override {
+    return code_signature_;
+  }
+
+  bool Next() override { return ++index_ < entries_.size(); }
+
+  base::string16 GetPath() override {
+    EXPECT_LT(index_, entries_.size());
+    return base::UTF8ToUTF16(entries_[index_].path);
+  }
+
+  std::unique_ptr<ReadStream> GetReadStream() override {
+    EXPECT_LT(index_, entries_.size());
+    const std::vector<uint8_t>& data = entries_[index_].data;
+    return std::make_unique<MemoryReadStream>(data.data(), data.size());
+  }
+
+ private:
+  bool open_ok_;
+  FileList entries_;
+  size_t index_;
+  std::vector<uint8_t> code_signature_;
+};
+
+TEST(DMGAnalyzerTest, FailToOpen) {
+  MockDMGIterator iterator(false, MockDMGIterator::FileList());
+  safe_browsing::ArchiveAnalyzerResults results;
+  AnalyzeDMGFile(&iterator, &results);
+
+  EXPECT_FALSE(results.success);
+  EXPECT_FALSE(results.has_archive);
+  EXPECT_FALSE(results.has_executable);
+  EXPECT_TRUE(results.archived_binary.empty());
+}
+
+TEST(DMGAnalyzerTest, EmptyDMG) {
+  MockDMGIterator iterator(true, MockDMGIterator::FileList());
+  safe_browsing::ArchiveAnalyzerResults results;
+  AnalyzeDMGFile(&iterator, &results);
+
+  EXPECT_TRUE(results.success);
+  EXPECT_FALSE(results.has_archive);
+  EXPECT_FALSE(results.has_executable);
+  EXPECT_TRUE(results.archived_binary.empty());
+}
+
+TEST(DMGAnalyzerTest, DetachedCodeSignature) {
+  base::FilePath real_code_signature_file;
+  ASSERT_TRUE(
+      base::PathService::Get(base::DIR_SOURCE_ROOT, &real_code_signature_file));
+  real_code_signature_file = real_code_signature_file.AppendASCII("chrome")
+                                 .AppendASCII("test")
+                                 .AppendASCII("data")
+                                 .AppendASCII("safe_browsing")
+                                 .AppendASCII("mach_o")
+                                 .AppendASCII("shell-script.app")
+                                 .AppendASCII("Contents")
+                                 .AppendASCII("_CodeSignature")
+                                 .AppendASCII("CodeSignature");
+
+  std::string real_code_signature;
+  ASSERT_TRUE(
+      base::ReadFileToString(real_code_signature_file, &real_code_signature));
+
+  MockDMGIterator::FileList file_list{
+      {"DMG/App.app/Contents/_CodeSignature/CodeSignature",
+       {real_code_signature.begin(), real_code_signature.end()}},
+  };
+
+  MockDMGIterator iterator(true, file_list);
+  safe_browsing::ArchiveAnalyzerResults results;
+  AnalyzeDMGFile(&iterator, &results);
+
+  EXPECT_TRUE(results.success);
+  EXPECT_TRUE(results.has_executable);
+  EXPECT_TRUE(results.archived_binary.empty());
+  ASSERT_EQ(1, results.detached_code_signatures.size());
+  EXPECT_EQ(real_code_signature,
+            results.detached_code_signatures.Get(0).contents());
+}
+
+TEST(DMGAnalyzerTest, InvalidDetachedCodeSignature) {
+  MockDMGIterator::FileList file_list{
+      {"DMG/App.app/Contents/_CodeSignature/CodeSignature", {0x30, 0x80}},
+  };
+
+  MockDMGIterator iterator(true, file_list);
+  safe_browsing::ArchiveAnalyzerResults results;
+  AnalyzeDMGFile(&iterator, &results);
+
+  EXPECT_TRUE(results.success);
+  EXPECT_TRUE(results.has_executable);
+  EXPECT_TRUE(results.archived_binary.empty());
+  EXPECT_EQ(0, results.detached_code_signatures.size());
+}
+
+}  // namespace
+}  // namespace dmg
+}  // namespace safe_browsing

chrome/utility/safe_browsing/mac/dmg_iterator.h

@@ -35,21 +35,21 @@ class DMGIterator {
   // method. If this returns false, it is illegal to call any other methods
   // on this class. If this returns true, the iterator is advanced to an
   // invalid element before the first item.
-  bool Open();
+  virtual bool Open();
 
   // Returns the raw code signature file metadata. This will be empty for DMGs
   // that are not signed.
-  const std::vector<uint8_t>& GetCodeSignature();
+  virtual const std::vector<uint8_t>& GetCodeSignature();
 
   // Advances the iterator to the next file item. Returns true on success
   // and false on end-of-iterator.
-  bool Next();
+  virtual bool Next();
 
   // Returns the full path in a DMG filesystem to the current file item.
-  base::string16 GetPath();
+  virtual base::string16 GetPath();
 
   // Returns a ReadStream for the current file item.
-  std::unique_ptr<ReadStream> GetReadStream();
+  virtual std::unique_ptr<ReadStream> GetReadStream();
 
  private:
   UDIFParser udif_;  // The UDIF parser that accesses the partitions.