Mac Archive Type Sniffing

chrome/utility/safe_browsing/mac/udif.h

Index: chrome/utility/safe_browsing/mac/udif.h
diff --git a/chrome/utility/safe_browsing/mac/udif.h b/chrome/utility/safe_browsing/mac/udif.h
index fbb17c9280743e2611deba7d9955657f8e175413..1bdef19188295aa52888a45cdd25ae92bef884bf 100644
--- a/chrome/utility/safe_browsing/mac/udif.h
+++ b/chrome/utility/safe_browsing/mac/udif.h
@@ -47,6 +47,11 @@ class UDIFParser {
   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();
@@ -71,7 +76,7 @@ class UDIFParser {
   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.
@@ -79,6 +84,10 @@ class UDIFParser {
   // 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);
 };