Improve Zip File Scanning on Mac

chrome/common/safe_browsing/zip_analyzer.cc

Index: chrome/common/safe_browsing/zip_analyzer.cc
diff --git a/chrome/common/safe_browsing/zip_analyzer.cc b/chrome/common/safe_browsing/zip_analyzer.cc
index 50f3f25f75cab68d2b77a6c62d4dfa16059f210d..0e3f4f68aa60fefa547f11324efc467c1b8f2bd2 100644
--- a/chrome/common/safe_browsing/zip_analyzer.cc
+++ b/chrome/common/safe_browsing/zip_analyzer.cc
@@ -10,9 +10,15 @@
 #include <memory>
 #include <set>
 
+#if defined(OS_MACOSX)

[Robert Sesek, 2017/08/03 15:43:36]

Platform-specific includes usually come after the common ones, so move this to
after line 30.

[mortonm, 2017/08/03 17:09:12]

Done.

+#include <mach-o/fat.h>
+#include <mach-o/loader.h>
+#endif  // OS_MACOSX
+
 #include "base/i18n/streaming_utf8_validator.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "build/build_config.h"

[Robert Sesek, 2017/08/03 15:43:36]

note: build_config.h is probably required for the OS_MACOSX define above

[mortonm, 2017/08/03 17:09:12]

Acknowledged.

 #include "chrome/common/safe_browsing/archive_analyzer_results.h"
 #include "chrome/common/safe_browsing/binary_feature_extractor.h"
 #include "chrome/common/safe_browsing/download_protection_util.h"
@@ -60,6 +66,19 @@ void HashingFileWriter::ComputeDigest(uint8_t* digest, size_t digest_length) {
   sha256_->Finish(digest, digest_length);
 }
 
+#if defined(OS_MACOSX)
+bool StringIsMachOMagic(std::string bytes) {
+  if (bytes.length() < sizeof(uint32_t))
+    return false;
+
+  uint32_t magic;
+  memcpy(&magic, bytes.c_str(), sizeof(uint32_t));
+
+  return magic == FAT_MAGIC || magic == FAT_CIGAM || magic == MH_MAGIC ||

[Robert Sesek, 2017/08/03 15:43:36]

Rather than duplicating this, can you use MachOImageReader::IsMachOMagicValue?

[mortonm, 2017/08/03 17:09:12]

Good call, thanks.

+         magic == MH_CIGAM || magic == MH_MAGIC_64 || magic == MH_CIGAM_64;
+}
+#endif  // OS_MACOSX
+
 void AnalyzeContainedFile(
     const scoped_refptr<BinaryFeatureExtractor>& binary_feature_extractor,
     const base::FilePath& file_path,
@@ -73,7 +92,8 @@ void AnalyzeContainedFile(
       download_protection_util::GetDownloadType(file_path));
   archived_binary->set_length(reader->current_entry_info()->original_size());
   HashingFileWriter writer(temp_file);
-  if (reader->ExtractCurrentEntry(&writer)) {
+  if (reader->ExtractCurrentEntry(&writer,
+                                  std::numeric_limits<uint64_t>::max())) {
     uint8_t digest[crypto::kSHA256Length];
     writer.ComputeDigest(&digest[0], arraysize(digest));
     archived_binary->mutable_digests()->set_sha256(&digest[0],
@@ -117,6 +137,18 @@ void AnalyzeZipFile(base::File zip_file,
       continue;
     }
     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);
+    current_entry_is_executable =
+        FileTypePolicies::GetInstance()->IsCheckedBinaryFile(file) ||
+        StringIsMachOMagic(magic);
+#else
+    current_entry_is_executable =
+        FileTypePolicies::GetInstance()->IsCheckedBinaryFile(file);
+#endif  // OS_MACOSX
+
     if (FileTypePolicies::GetInstance()->IsArchiveFile(file)) {
       DVLOG(2) << "Downloaded a zipped archive: " << file.value();
       results->has_archive = true;
@@ -128,11 +160,22 @@ void AnalyzeZipFile(base::File zip_file,
         archived_archive->set_file_basename(file_basename_utf8);
       archived_archive->set_download_type(
           ClientDownloadRequest::ZIPPED_ARCHIVE);
-    } else if (FileTypePolicies::GetInstance()->IsCheckedBinaryFile(file)) {
-      DVLOG(2) << "Downloaded a zipped executable: " << file.value();
-      results->has_executable = true;
-      AnalyzeContainedFile(binary_feature_extractor, file, &reader, &temp_file,
-                           results->archived_binary.Add());
+    } else if (current_entry_is_executable) {
+#if defined(OS_MACOSX)
+      // This check prevents running analysis on .app files since they are
+      // really just directories and will cause binary feature extraction
+      // to fail.
+      if (file.Extension().compare(".app") == 0) {
+        DVLOG(2) << "Downloaded a zipped .app directory: " << file.value();
+      } else {
+#endif  // OS_MACOSX
+        DVLOG(2) << "Downloaded a zipped executable: " << file.value();
+        results->has_executable = true;
+        AnalyzeContainedFile(binary_feature_extractor, file, &reader,
+                             &temp_file, results->archived_binary.Add());
+#if defined(OS_MACOSX)
+      }
+#endif  // OS_MACOSX
     } else {
       DVLOG(3) << "Ignoring non-binary file: " << file.value();
     }