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..eeb3ae5e57a2170bbc3dae2b435cba933a834f07 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)
+#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"
 #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 ||
+         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,
@@ -116,7 +135,21 @@ void AnalyzeZipFile(base::File zip_file,
       DVLOG(1) << "Failed to open current entry in 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.ExtractFromBeginningOfCurrentEntryToString(sizeof(uint32_t), false,

[satorux1, 2017/07/21 05:27:02]

Sorry for not taking a look at the caller earlier, but I'm trying to understand
why you needed to introduce this new function.

Seems to me that the following would be OK:

  reader.ExtractCurrentEntryToString(sizeof(uint32_t), &magic);

If the content is larger than 4GB, the function will stop reading at the maximum
size. It'll return false but |magic| should contain the 4GB of data.

[mortonm, 2017/07/21 16:29:56]

In this case, we only want to read the first 4 bytes of the current entry to
determine whether it is a Mac executable (and thus has the MAGIC header for
MachO files). We don't want to do an in-memory copy of a large file into a
string just to be able to check the first 4 bytes. The existing functionality in
zip_reader only allows for reading an entire file while specifying the max
length to extract, but we can't just specify |4| here because for all files
larger than 4 bytes the extraction would simply fail.

In the current implementation, when extraction fails,
ExtractCurrentEntryToString() does not copy any data to the output string. Note
that this function returns without copying data to output:
https://cs.chromium.org/chromium/src/third_party/zlib/google/zip_reader.cc?rc...

Moreover, the change isn't as simple as just copying the data to the output even
when extraction fails, due to the way ExtractCurrentEntry() is written as well
as the WriteBytes function for StringWriterDelegate:
https://cs.chromium.org/chromium/src/third_party/zlib/google/zip_reader.cc?rc...

Unless i'm missing something, I think the new function is necessary for reading
the first 4 bytes of an entry

[satorux1, 2017/07/21 23:00:09]

My bad. sizeof(uint32_t) is of course 4, not 4GB. :( Sorry for the confusion.

What I meant was that the following would work:

  reader.ExtractCurrentEntryToString(sizeof(sizeof(uint32_t), &magic);   //
sizeof(sizeof(uint32_t) == 4

But you are right that it doesn't work currently. I'd rather think that fixing
the behavior is better than introducing a new function that looks similar but
behaves slightly different.

[mortonm, 2017/07/24 17:51:43]

Done. Required adding an extra parameter to each of the existing functions, but
this seemed like the most straightforward way.

+                                                      &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 +161,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();
     }