Mac Archive Type Sniffing

chrome/utility/safe_browsing/mac/udif.cc

Index: chrome/utility/safe_browsing/mac/udif.cc
diff --git a/chrome/utility/safe_browsing/mac/udif.cc b/chrome/utility/safe_browsing/mac/udif.cc
index 0c4be3fa310747ef84226969a5ea536f630e8b6c..4dac399ed0175d97f6099b74eae843307cc284d3 100644
--- a/chrome/utility/safe_browsing/mac/udif.cc
+++ b/chrome/utility/safe_browsing/mac/udif.cc
@@ -33,12 +33,6 @@ namespace dmg {
 //
 // Note that all fields are stored in big endian.
 
-struct UDIFChecksum {
-  uint32_t type;
-  uint32_t size;
-  uint32_t data[32];
-};
-
 static void ConvertBigEndian(UDIFChecksum* checksum) {
   ConvertBigEndian(&checksum->type);
   ConvertBigEndian(&checksum->size);
@@ -47,46 +41,6 @@ static void ConvertBigEndian(UDIFChecksum* checksum) {
   }
 }
 
-// The trailer structure for a UDIF file.
-struct UDIFResourceFile {
-  static const uint32_t kSignature = 'koly';
-  static const uint32_t kVersion = 4;
-
-  uint32_t signature;
-  uint32_t version;
-  uint32_t header_size;  // Size of this structure.
-  uint32_t flags;
-  uint64_t running_data_fork_offset;
-  uint64_t data_fork_offset;
-  uint64_t data_fork_length;
-  uint64_t rsrc_fork_offset;
-  uint64_t rsrc_fork_length;
-  uint32_t segment_number;
-  uint32_t segment_count;
-  uuid_t   segment_id;
-
-  UDIFChecksum data_checksum;
-
-  uint64_t plist_offset;  // Offset and length of the blkx plist.
-  uint64_t plist_length;
-
-  uint8_t  reserved1[64];
-
-  uint64_t code_signature_offset;
-  uint64_t code_signature_length;
-
-  uint8_t  reserved2[40];
-
-  UDIFChecksum master_checksum;
-
-  uint32_t image_variant;
-  uint64_t sector_count;
-
-  uint32_t reserved3;
-  uint32_t reserved4;
-  uint32_t reserved5;
-};
-
 static void ConvertBigEndian(uuid_t* uuid) {
   // UUID is never consulted, so do not swap.
 }
@@ -348,11 +302,58 @@ UDIFParser::UDIFParser(ReadStream* stream)
     : stream_(stream),
       partition_names_(),
       blocks_(),
-      block_size_(kSectorSize) {
+      block_size_(kSectorSize),
+      trailer_successfully_parsed_(false) {
+  trailer_successfully_parsed_ = ParseTrailer();

[Jialiu Lin, 2017/06/09 18:48:49]

A better way to do this:
1. Don't call ParseTrailer() in the constructor. 
2. Make ParseTrailer() a public function
3. set trailer_successfully_parsed_ at the end of ParseTrailer() function.
4. call ParseTrailer() the first thing in ParseBlkx
5. In mac_archieve_type_sniffer, you do 
...
dmg::UDIFParser parser(&read_stream);
return parser.ParseTrailer();
...

6. you probably don't need GetTrailer() at all.

[mortonm, 2017/06/09 19:47:21]

As we just discussed, I'm going to leave GetTrailer() in, since we will want to
obtain the DMG signatures from the trailer in the near future. Regardless, I
have moved ParseTrailer() out of the constructor and made it a public method.

[Robert Sesek, 2017/06/09 20:02:32]

I'd strongly prefer to not expose the UDIF internal types. For extracting
signatures, the DMGParser/UDIFParser could expose a method that just provides
signature information.

[mortonm, 2017/06/09 20:23:11]

Ok. I've taken out GetTrailer(). We will add a GetCodeSignature() function in a
later CL and accessor functions for any other part of the trailer that we need.

 }
 
 UDIFParser::~UDIFParser() {}
 
+bool UDIFParser::ParseTrailer() {
+  // Makes sure that trailer is only ever parsed single time during execution of
+  // constuctor, so position of ReadStream is not moved after constructor runs.
+  if (trailer_successfully_parsed_)
+    return true;
+
+  off_t trailer_start = stream_->Seek(-sizeof(trailer_), SEEK_END);
+  if (trailer_start == -1)
+    return false;
+
+  if (!stream_->ReadType(&trailer_)) {
+    DLOG(ERROR) << "Failed to read UDIFResourceFile";
+    return false;
+  }
+  ConvertBigEndian(&trailer_);
+
+  if (trailer_.signature != trailer_.kSignature) {
+    DLOG(ERROR) << "blkx signature does not match, is 0x" << std::hex
+                << trailer_.signature;
+    return false;
+  }
+  if (trailer_.version != trailer_.kVersion) {
+    DLOG(ERROR) << "blkx version does not match, is " << trailer_.version;
+    return false;
+  }
+
+  auto plist_end = base::CheckedNumeric<size_t>(trailer_.plist_offset) +
+                   trailer_.plist_length;
+  if (!plist_end.IsValid() ||
+      plist_end.ValueOrDie() > base::checked_cast<size_t>(trailer_start)) {
+    DLOG(ERROR) << "blkx plist extends past UDIF trailer";
+    return false;
+  }
+
+  return true;
+}
+
+UDIFResourceFile* UDIFParser::GetTrailer() {
+  if (!trailer_successfully_parsed_) {
+    return NULL;
+  } else {
+    return &trailer_;
+  }
+}
+
 bool UDIFParser::Parse() {
   if (!ParseBlkx())
     return false;
@@ -407,41 +408,15 @@ std::unique_ptr<ReadStream> UDIFParser::GetPartitionReadStream(
 }
 
 bool UDIFParser::ParseBlkx() {
-  UDIFResourceFile trailer;
-  off_t trailer_start = stream_->Seek(-sizeof(trailer), SEEK_END);
-  if (trailer_start == -1)
+  if (!trailer_successfully_parsed_)
     return false;
 
-  if (!stream_->ReadType(&trailer)) {
-    DLOG(ERROR) << "Failed to read UDIFResourceFile";
-    return false;
-  }
-  ConvertBigEndian(&trailer);
-
-  if (trailer.signature != trailer.kSignature) {
-    DLOG(ERROR) << "blkx signature does not match, is 0x"
-                << std::hex << trailer.signature;
-    return false;
-  }
-  if (trailer.version != trailer.kVersion) {
-    DLOG(ERROR) << "blkx version does not match, is " << trailer.version;
-    return false;
-  }
-
-  auto plist_end = base::CheckedNumeric<size_t>(trailer.plist_offset) +
-                   trailer.plist_length;
-  if (!plist_end.IsValid() ||
-      plist_end.ValueOrDie() > base::checked_cast<size_t>(trailer_start)) {
-    DLOG(ERROR) << "blkx plist extends past UDIF trailer";
-    return false;
-  }
-
-  std::vector<uint8_t> plist_bytes(trailer.plist_length, 0);
+  std::vector<uint8_t> plist_bytes(trailer_.plist_length, 0);
 
-  if (stream_->Seek(trailer.plist_offset, SEEK_SET) == -1)
+  if (stream_->Seek(trailer_.plist_offset, SEEK_SET) == -1)
     return false;
 
-  if (!stream_->ReadExact(&plist_bytes[0], trailer.plist_length)) {
+  if (!stream_->ReadExact(&plist_bytes[0], trailer_.plist_length)) {
     DLOG(ERROR) << "Failed to read blkx plist data";
     return false;
   }