Mac Archive Type Sniffing

chrome/utility/safe_browsing/mac/udif.h

 // Copyright 2015 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.
 
 #ifndef CHROME_UTILITY_SAFE_BROWSING_MAC_UDIF_H_
 #define CHROME_UTILITY_SAFE_BROWSING_MAC_UDIF_H_
 
 #include <CoreFoundation/CoreFoundation.h>
 #include <stddef.h>
 #include <stdint.h>
@@ skipping 29 matching lines @@
 //
 // The following references are useful for understanding the UDIF format:
 //   - http://newosxbook.com/DMG.html
 //   - http://www.macdisk.com/dmgen.php
 class UDIFParser {
  public:
   // Constructs an instance from a stream.
   explicit UDIFParser(ReadStream* stream);
   ~UDIFParser();
 
+  // Parses the trailer structure. Only actually parses data from the file the
+  // first time it is called -- after that just returns the previous result when
+  // parsing was attempted.
+  bool ParseTrailer();

[Robert Sesek, 2017/06/12 16:38:39]

This should be private, callers should only call Parse(), and then a getter can
be exposed to get trailer_successfuly_parsed().

[mortonm, 2017/06/12 18:31:16]

Yeah, that does seem like a better interface. One potential issue is that this
code path is only concerned with determining whether the downloaded file has the
"koly" trailer. If we call the whole Parse() code path (i.e. ParseTrailer() and
ParseBlkx()) then for such files the entire unpacking of the DMG will happen
twice - since we're not actually using the results of the unpacking here. I
guess this isn't a big deal, since the only time this happens is for files
without a disk-image-type extension (e.g. DMG) that do have a "koly" signature,
which should be rare and possibly only done by malware.

I went ahead and made the change anyway.

[mortonm, 2017/06/12 20:04:01]

Actually, there is another issue to consider here. In the current workflow, a
file with a 'koly' signature and no disk-image-type extension will be fully
unpacked on the file thread without any dedicated sandbox to contain the
processing. This could be a security/reliability liability -- not sure.

We think a possible way forward is to leave the ParseTrailer() function as
public for now, as there is soon going to be a refactoring effort to move the
classes defined in download_protection_service.cc (e.g.
CheckClientDownloadRequest) to their own .h files. At that point, we can make
the ParseTrailer() function private and declare CheckClientDownloadRequest as a
friend class of UDIFParser.

[Robert Sesek, 2017/06/12 20:52:34]

Yes, that's definitely a security issue. We shouldn't be parsing really any
untrusted content in the browser process.
OK.

[mortonm, 2017/06/12 21:39:49]

As addressed in the later comment, we think we need to do the signature checking
in a utility process anyway.

+
   // Parses the UDIF file. This method must be called before any other method.
   // If this returns false, it is not legal to call any other methods.
   bool Parse();
 
   // Returns the number of partitions in this UDIF image.
   size_t GetNumberOfPartitions();
 
   // Returns the partition name for a given partition number, in the range of
   // [0,GetNumberOfPartitions()). This will include the number, name, and type
   // of partition. E.g., "disk image (Apple_HFS : 2)".
   std::string GetPartitionName(size_t part_number);
 
   // Returns the partition type as a string for the given partition number.
   // E.g., "Apple_HFS" and "Apple_Free".
   std::string GetPartitionType(size_t part_number);
 
   // Returns the size of the partition in bytes.
   size_t GetPartitionSize(size_t part_number);
 
   // Returns a stream of the raw partition data for the given partition
   // number.
   std::unique_ptr<ReadStream> GetPartitionReadStream(size_t part_number);
 
  private:
-  // Parses the blkx plist trailer structure.
+  // Parses the blkx plist structure.
   bool ParseBlkx();
 
   ReadStream* const stream_;  // The stream backing the UDIF image. Weak.
   std::vector<std::string> partition_names_;  // The names of all partitions.
   // All blocks in the UDIF image.
   std::vector<std::unique_ptr<const UDIFBlock>> blocks_;
   uint16_t block_size_;  // The image's block size, in bytes.
+  uint32_t trailer_signature_;
+  uint64_t plist_length_;
+  uint64_t plist_offset_;
+  bool trailer_successfully_parsed_;
 
   DISALLOW_COPY_AND_ASSIGN(UDIFParser);
 };
 
 }  // namespace dmg
 }  // namespace safe_browsing
 
 #endif  // CHROME_UTILITY_SAFE_BROWSING_MAC_UDIF_H_