Type sniff DMGs inside ZIPs

Since OS X type sniffs to determine if a file is executable, we should
type sniff within a ZIP to identify DMGs with no file extension as
executable files.

Change-Id: I8e5e19205d46e60be8cccb3e259fcb705c1ca1f3
Bug: 866021
Reviewed-on: https://chromium-review.googlesource.com/1152183Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590020}

chrome/browser/safe_browsing/BUILD.gn

@@ -140,8 +140,6 @@ static_library("safe_browsing") {
         "client_side_model_loader.h",
         "download_protection/check_client_download_request.cc",
         "download_protection/check_client_download_request.h",
-        "download_protection/disk_image_type_sniffer_mac.cc",
-        "download_protection/disk_image_type_sniffer_mac.h",
         "download_protection/download_feedback.cc",
         "download_protection/download_feedback.h",
         "download_protection/download_feedback_service.cc",
@@ -212,6 +210,7 @@ static_library("safe_browsing") {
       ]
       deps += [
         ":advanced_protection",
+        "//chrome/common/safe_browsing:disk_image_type_sniffer_mac",
         "//chrome/services/file_util/public/cpp",
         "//components/content_settings/core/browser:browser",
         "//components/language/core/common:common",

chrome/browser/safe_browsing/download_protection/check_client_download_request.h

@@ -31,7 +31,7 @@
 #include "url/gurl.h"
 
 #if defined(OS_MACOSX)
-#include "chrome/browser/safe_browsing/download_protection/disk_image_type_sniffer_mac.h"
+#include "chrome/common/safe_browsing/disk_image_type_sniffer_mac.h"
 #include "chrome/services/file_util/public/cpp/sandboxed_dmg_analyzer_mac.h"
 #endif
 

chrome/common/safe_browsing/BUILD.gn

@@ -89,6 +89,17 @@ if (safe_browsing_mode == 1) {
       "//components/safe_browsing:csd_proto",
     ]
   }
+
+  source_set("disk_image_type_sniffer_mac") {
+    sources = [
+      "disk_image_type_sniffer_mac.cc",
+      "disk_image_type_sniffer_mac.h",
+    ]
+
+    deps = [
+      "//base:base",
+    ]
+  }
 }
 
 source_set("safe_browsing") {
@@ -96,6 +107,10 @@ source_set("safe_browsing") {
     ":file_type_policies",
   ]
 
+  if (is_mac) {
+    deps += [ ":disk_image_type_sniffer_mac" ]
+  }
+
   if (safe_browsing_mode == 1) {
     sources = [
       "binary_feature_extractor.cc",

chrome/browser/safe_browsing/download_protection/disk_image_type_sniffer_mac.cc → chrome/common/safe_browsing/disk_image_type_sniffer_mac.cc

@@ -2,9 +2,8 @@
 // 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/disk_image_type_sniffer_mac.h"
+#include "chrome/common/safe_browsing/disk_image_type_sniffer_mac.h"
 #include "base/threading/scoped_blocking_call.h"
-#include "content/public/browser/browser_thread.h"
 
 namespace safe_browsing {
 
@@ -20,6 +19,8 @@ DiskImageTypeSnifferMac::DiskImageTypeSnifferMac() {}
 
 // static
 bool DiskImageTypeSnifferMac::IsAppleDiskImage(const base::FilePath& dmg_file) {
+  // TODO(drubery): Macs accept DMGs with koly blocks at the beginning of the
+  // file. Investigate if this is a problem, and if so, update this function.
   base::ScopedBlockingCall scoped_blocking_call(base::BlockingType::MAY_BLOCK);
 
   base::File file(dmg_file, base::File::FLAG_OPEN | base::File::FLAG_READ);
@@ -35,7 +36,26 @@ bool DiskImageTypeSnifferMac::IsAppleDiskImage(const base::FilePath& dmg_file) {
       kSizeKolySignatureInBytes)
     return false;
 
-  return (memcmp(data, kKolySignature, kSizeKolySignatureInBytes) == 0);
+  return IsAppleDiskImageTrailer(base::span<uint8_t>(
+      reinterpret_cast<uint8_t*>(data), kSizeKolySignatureInBytes));
+}
+
+// static
+bool DiskImageTypeSnifferMac::IsAppleDiskImageTrailer(
+    const base::span<const uint8_t>& trailer) {
+  if (trailer.size() < kSizeKolySignatureInBytes)
+    return false;
+
+  const base::span<const uint8_t> subspan =
+      trailer.last(kSizeKolySignatureInBytes);
+
+  return (memcmp(subspan.data(), kKolySignature, kSizeKolySignatureInBytes) ==
+          0);
+}
+
+// static
+size_t DiskImageTypeSnifferMac::AppleDiskImageTrailerSize() {
+  return kSizeKolyTrailerInBytes;
 }
 
 DiskImageTypeSnifferMac::~DiskImageTypeSnifferMac() = default;

chrome/browser/safe_browsing/download_protection/disk_image_type_sniffer_mac.h → chrome/common/safe_browsing/disk_image_type_sniffer_mac.h

@@ -2,9 +2,10 @@
 // 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_DISK_IMAGE_TYPE_SNIFFER_MAC_H_
-#define CHROME_BROWSER_SAFE_BROWSING_DISK_IMAGE_TYPE_SNIFFER_MAC_H_
+#ifndef CHROME_COMMON_SAFE_BROWSING_DISK_IMAGE_TYPE_SNIFFER_MAC_H_
+#define CHROME_COMMON_SAFE_BROWSING_DISK_IMAGE_TYPE_SNIFFER_MAC_H_
 
+#include "base/containers/span.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
@@ -23,6 +24,12 @@ class DiskImageTypeSnifferMac
   // FILE thread.
   static bool IsAppleDiskImage(const base::FilePath& dmg_file);
 
+  // Returns true when the trailer is a valid trailer for a DMG type.
+  static bool IsAppleDiskImageTrailer(const base::span<const uint8_t>& trailer);
+
+  // Returns the size of a DMG trailer.
+  static size_t AppleDiskImageTrailerSize();
+
  private:
   friend class base::RefCountedThreadSafe<DiskImageTypeSnifferMac>;
 
@@ -33,4 +40,4 @@ class DiskImageTypeSnifferMac
 
 }  // namespace safe_browsing
 
-#endif  // CHROME_BROWSER_SAFE_BROWSING_DISK_IMAGE_TYPE_SNIFFE_MAC_H_
+#endif  // CHROME_COMMON_SAFE_BROWSING_DISK_IMAGE_TYPE_SNIFFE_MAC_H_

chrome/browser/safe_browsing/download_protection/disk_image_type_sniffer_mac_unittest.cc → chrome/common/safe_browsing/disk_image_type_sniffer_mac_unittest.cc

@@ -2,7 +2,7 @@
 // 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/disk_image_type_sniffer_mac.h"
+#include "chrome/common/safe_browsing/disk_image_type_sniffer_mac.h"
 
 #include "base/files/file_path.h"
 #include "base/macros.h"
@@ -103,5 +103,13 @@ INSTANTIATE_TEST_CASE_P(DiskImageTypeSnifferMacTestInstantiation,
                         DiskImageTypeSnifferMacTest,
                         testing::ValuesIn(cases));
 
+TEST(DiskImageTypeSnifferMacTest, IsAppleDiskImageTrailerIsCorrect) {
+  uint8_t good_header[4] = {'k', 'o', 'l', 'y'};
+  EXPECT_TRUE(DiskImageTypeSnifferMac::IsAppleDiskImageTrailer(good_header));
+
+  uint8_t bad_header[6] = {'f', 'o', 'o', 'b', 'a', 'r'};
+  EXPECT_FALSE(DiskImageTypeSnifferMac::IsAppleDiskImageTrailer(bad_header));
+}
+
 }  // namespace
 }  // namespace safe_browsing

chrome/common/safe_browsing/zip_analyzer.cc

@@ -27,6 +27,8 @@
 #if defined(OS_MACOSX)
 #include <mach-o/fat.h>
 #include <mach-o/loader.h>
+#include "base/containers/span.h"
+#include "chrome/common/safe_browsing/disk_image_type_sniffer_mac.h"
 #include "chrome/common/safe_browsing/mach_o_image_reader_mac.h"
 #endif  // OS_MACOSX
 
@@ -116,6 +118,47 @@ void AnalyzeContainedBinary(
   }
 }
 
+// Helper class to get a certain size trailer of the extracted file. Extracts
+// the file into memory, retaining only the last |trailer_size_bytes| bytes.
+class TrailerWriterDelegate : public zip::WriterDelegate {
+ public:
+  explicit TrailerWriterDelegate(size_t trailer_size_bytes);
+
+  // zip::WriterDelegate implementation:
+  bool PrepareOutput() override { return true; }
+  bool WriteBytes(const char* data, int num_bytes) override;
+  void SetTimeModified(const base::Time& time) override {}
+
+  const std::string& trailer() { return trailer_; }
+
+ private:
+  size_t trailer_size_bytes_;
+  std::string trailer_;
+};
+
+TrailerWriterDelegate::TrailerWriterDelegate(size_t trailer_size_bytes)
+    : trailer_size_bytes_(trailer_size_bytes), trailer_() {}
+
+bool TrailerWriterDelegate::WriteBytes(const char* data, int num_bytes) {
+  // TODO(drubery): WriterDelegate::WriteBytes should probably have |num_bytes|
+  // by a size_t. Investigate how difficult it would be to migrate
+  // implementations of WriterDelegate over.
+  base::CheckedNumeric<size_t> num_bytes_size(num_bytes);
+  if (!num_bytes_size.IsValid())
+    return false;
+
+  if (num_bytes_size.ValueOrDie() >= trailer_size_bytes_) {
+    trailer_ = std::string(data + num_bytes - trailer_size_bytes_,
+                           trailer_size_bytes_);
+  } else {
+    trailer_.append(data, num_bytes);
+    if (trailer_.size() > trailer_size_bytes_) {
+      trailer_ = trailer_.substr(trailer_.size() - trailer_size_bytes_);
+    }
+  }
+  return true;
+}
+
 }  // namespace
 
 void AnalyzeZipFile(base::File zip_file,
@@ -145,12 +188,38 @@ void AnalyzeZipFile(base::File zip_file,
     }
     const base::FilePath& file = reader.current_entry_info()->file_path();
     bool current_entry_is_executable;
+
 #if defined(OS_MACOSX)
     std::string magic;
     reader.ExtractCurrentEntryToString(sizeof(uint32_t), &magic);
+
+    std::string dmg_header;
+    reader.ExtractCurrentEntryToString(
+        DiskImageTypeSnifferMac::AppleDiskImageTrailerSize(), &dmg_header);
+
     current_entry_is_executable =
         FileTypePolicies::GetInstance()->IsCheckedBinaryFile(file) ||
-        StringIsMachOMagic(magic);
+        StringIsMachOMagic(magic) ||
+        DiskImageTypeSnifferMac::IsAppleDiskImageTrailer(
+            base::span<const uint8_t>(
+                reinterpret_cast<const uint8_t*>(dmg_header.c_str()),
+                dmg_header.size()));
+
+    // We can skip checking the trailer if we already know the file is
+    // executable.
+    if (!current_entry_is_executable) {
+      TrailerWriterDelegate trailer_writer(
+          DiskImageTypeSnifferMac::AppleDiskImageTrailerSize());
+      reader.ExtractCurrentEntry(
+          &trailer_writer,
+          FileTypePolicies::GetInstance()->GetMaxFileSizeToAnalyze("dmg"));
+      const std::string& trailer = trailer_writer.trailer();
+      current_entry_is_executable =
+          DiskImageTypeSnifferMac::IsAppleDiskImageTrailer(
+              base::span<const uint8_t>(
+                  reinterpret_cast<const uint8_t*>(trailer.data()),
+                  trailer.size()));
+    }
 #else
     current_entry_is_executable =
         FileTypePolicies::GetInstance()->IsCheckedBinaryFile(file);

chrome/test/BUILD.gn

@@ -3901,7 +3901,6 @@ test("unit_tests") {
       "../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",
       "../browser/safe_browsing/download_protection/download_protection_service_unittest.cc",
@@ -3935,6 +3934,7 @@ test("unit_tests") {
       "../common/safe_browsing/binary_feature_extractor_mac_unittest.cc",
       "../common/safe_browsing/binary_feature_extractor_unittest.cc",
       "../common/safe_browsing/binary_feature_extractor_win_unittest.cc",
+      "../common/safe_browsing/disk_image_type_sniffer_mac_unittest.cc",
       "../common/safe_browsing/download_protection_util_unittest.cc",
       "../common/safe_browsing/ipc_protobuf_message_test_messages.h",
       "../common/safe_browsing/ipc_protobuf_message_unittest.cc",