Record Code Signature of Downloaded DMG files

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..f20622bd15c5a10962ba7cd0a6dbbf54d8c5e5c9 100644
--- a/chrome/utility/safe_browsing/mac/udif.cc
+++ b/chrome/utility/safe_browsing/mac/udif.cc
@@ -348,8 +348,9 @@ UDIFParser::UDIFParser(ReadStream* stream)
     : stream_(stream),
       partition_names_(),
       blocks_(),
-      block_size_(kSectorSize) {
-}
+      block_size_(kSectorSize),
+      signature_blob_data_(nullptr),
+      signature_blob_length_(0) {}
 
 UDIFParser::~UDIFParser() {}
 
@@ -360,6 +361,14 @@ bool UDIFParser::Parse() {
   return true;
 }
 
+uint64_t UDIFParser::GetDmgSignatureLength() {
+  return signature_blob_length_;
+}
+
+uint8_t* UDIFParser::GetDmgSignatureData() {
+  return signature_blob_data_;
+}
+
 size_t UDIFParser::GetNumberOfPartitions() {
   return blocks_.size();
 }
@@ -557,6 +566,39 @@ bool UDIFParser::ParseBlkx() {
     partition_names_.push_back(partition_name);
   }
 
+  // The offsets in the trailer could be garbage in DMGs that aren't signed.
+  // Need a sanity check that the DMG has legit values for these fields.
+  if (trailer.code_signature_length != 0 && trailer_start > 0 &&
+      trailer.code_signature_offset + trailer.code_signature_length <=

[Robert Sesek, 2017/06/26 18:56:33]

Use safe_numerics for this math. There are examples in this file.

[mortonm, 2017/06/27 16:36:23]

Done.

+          (uint64_t)trailer_start) {

[Robert Sesek, 2017/06/26 18:56:33]

C-style casts are banned by the styleguide.

[mortonm, 2017/06/27 16:36:23]

Done.

+    uint64_t temp_size = trailer.code_signature_length;
+    uint8_t* temp_data = new uint8_t[temp_size];

[Robert Sesek, 2017/06/26 18:56:33]

This is leaked. Rather than tracking size and bytes as two separate fields, use
a std::vector. That will also fix the ownership issue.

[mortonm, 2017/06/27 16:36:23]

Done.

+
+    if (temp_data == nullptr) {

[Robert Sesek, 2017/06/26 18:56:33]

This doesn't happen in Chromium (failure to allocate is fatal).

[mortonm, 2017/06/27 16:36:23]

So do you think I should put a cap on the size of signature field specified by
file metadata for which we try to allocate a buffer? Otherwise our analysis
could be evaded by simply setting the value at offset
trailer.code_signature_length to be arbitrarily large.

On the other hand this may make the DMG un-openable on Mac, so maybe we don't
care.

[Robert Sesek, 2017/06/28 18:21:06]

I could see putting a cap on the signature_length, but I don't know what value
to use. 10K, 1M, etc? It wouldn't totally thwart analysis, though, right? Just
the outer code signature extraction.

[mortonm, 2017/06/28 23:07:08]

As of now, if the metadata in trailer.code_signature_length has a huge value
(e.g. 5GB), the underlying memory allocator for the vector will likely fail for
the call to signature_blob_.resize(trailer.code_signature_length). I'm guessing
the exception thrown (http://www.cplusplus.com/reference/vector/vector/resize/)
will crash the utility process. Of course this necessitates the file being 5GB
as well.

I'm just gonna leave the code as is for now since this evasion would require a
very large DMG, which in a soon-to-come CL would not be unpacked by the DMG
analyzer anyway.

+      // If indicated signature size was too large for allocation, still want to
+      // continue processing DMG.
+      return true;
+    }
+
+    off_t code_signature_start =
+        stream_->Seek(trailer.code_signature_offset, SEEK_SET);
+    if (code_signature_start == -1)
+      return false;
+
+    size_t bytes_read = 0;
+
+    if (!stream_->Read(temp_data, temp_size, &bytes_read)) {

[Robert Sesek, 2017/06/26 18:56:33]

… and then using a vector, you std::vector<unit8_t>::resize to the
code_signature_length and then read directly into the vector member.

[mortonm, 2017/06/27 16:36:23]

Done.

+      DLOG(ERROR) << "Failed to read raw signature bytes";
+      return false;
+    }
+
+    if (bytes_read != temp_size)
+      return false;
+
+    signature_blob_length_ = temp_size;
+    signature_blob_data_ = temp_data;
+  }
+
   return true;
 }