Improve Zip File Scanning on Mac

chrome/common/safe_browsing/zip_analyzer.cc

 // Copyright (c) 2012 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/zip_analyzer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #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"
 #include "chrome/common/safe_browsing/file_type_policies.h"
 #include "components/safe_browsing/csd.pb.h"
 #include "crypto/secure_hash.h"
 #include "crypto/sha2.h"
 #include "third_party/zlib/google/zip_reader.h"
 
 namespace safe_browsing {
@@ skipping 27 matching lines @@
   if (!zip::FileWriterDelegate::WriteBytes(data, num_bytes))
     return false;
   sha256_->Update(data, num_bytes);
   return true;
 }
 
 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,
     zip::ZipReader* reader,
     base::File* temp_file,
     ClientDownloadRequest::ArchivedBinary* archived_binary) {
   std::string file_basename(file_path.BaseName().AsUTF8Unsafe());
   if (base::StreamingUtf8Validator::Validate(file_basename))
     archived_binary->set_file_basename(file_basename);
   archived_binary->set_download_type(
@@ skipping 36 matching lines @@
   bool advanced = true;
   for (; reader.HasMore(); advanced = reader.AdvanceToNextEntry()) {
     if (!advanced) {
       DVLOG(1) << "Could not advance to next entry, aborting zip scan.";
       return;
     }
     if (!reader.OpenCurrentEntryInZip()) {
       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;
       archived_archive_filenames.insert(file.BaseName());
       ClientDownloadRequest::ArchivedBinary* archived_archive =
           results->archived_binary.Add();
       std::string file_basename_utf8(file.BaseName().AsUTF8Unsafe());
       if (base::StreamingUtf8Validator::Validate(file_basename_utf8))
         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();
     }
   }
   results->archived_archive_filenames.assign(archived_archive_filenames.begin(),
                                              archived_archive_filenames.end());
   results->success = true;
 }
 
 }  // namespace zip_analyzer
 }  // namespace safe_browsing