Validate safe_browsing::dmg::UDIFBlock data before attempting to read at its offsets.

chrome/utility/safe_browsing/mac/udif.cc

 // 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.
 
 #include "chrome/utility/safe_browsing/mac/udif.h"
 
 #include <CoreFoundation/CoreFoundation.h>
 #include <bzlib.h>
 #include <libkern/OSByteOrder.h>
 #include <uuid/uuid.h>
@@ skipping 163 matching lines @@
   // Reserved fields are skipped.
   ConvertBigEndian(&block->checksum);
   ConvertBigEndian(&block->chunk_count);
   // Note: This deliberately does not swap the chunks themselves.
 }
 
 // UDIFBlock takes a raw, big-endian block data pointer and stores, in host
 // endian, the data for both the block and the chunk.
 class UDIFBlock {
  public:
-  explicit UDIFBlock(const UDIFBlockData* block_data) : block(*block_data) {
-    ConvertBigEndian(&block);
-    for (uint32_t i = 0; i < block.chunk_count; ++i) {
-      chunks.push_back(block_data->chunks[i]);
-      ConvertBigEndian(&chunks[i]);
+  explicit UDIFBlock(uint16_t sector_size)
+      : sector_size_(sector_size),
+        block_() {}
+
+  bool ParseBlockData(const UDIFBlockData* block_data) {
+    block_ = *block_data;
+    ConvertBigEndian(&block_);
+
+    // Make sure the number of sectors doesn't overflow.
+    auto block_size = base::CheckedNumeric<size_t>(sector_count()) *
+                      sector_size_;
+    if (!block_size.IsValid()) {
+      DLOG(ERROR) << "UDIF block size overflows";
+      return false;
     }
+
+    // Make sure that the chunk data isn't larger than the block reports.
+    base::CheckedNumeric<size_t> chunk_sectors(0);
+    for (uint32_t i = 0; i < block_.chunk_count; ++i) {
+      chunks_.push_back(block_data->chunks[i]);
+      UDIFBlockChunk* chunk = &chunks_[i];
+      ConvertBigEndian(chunk);
+
+      chunk_sectors += chunk->sector_count;
+      if (!chunk_sectors.IsValid() ||
+          chunk_sectors.ValueOrDie() > sector_count()) {
+        DLOG(ERROR) << "Total chunk sectors larger than reported block sectors";
+        return false;
+      }
+
+      auto chunk_end_offset =
+          base::CheckedNumeric<size_t>(chunk->compressed_offset) +
+          chunk->compressed_length;
+      if (!chunk_end_offset.IsValid() ||
+          chunk->compressed_length > block_size.ValueOrDie()) {

[Mark Mentovai, 2016/07/12 15:22:23]

Should this be checking that the chunk_end_offset > block_size?

[Robert Sesek, 2016/07/12 16:16:35]

No, because of the way these values are nested. It should be
>block_size+(start_sector*sector_size). But per offline investigation, the UDRO
image tests fail if we perform this validation. These offsets appear to be
absolute in the file. We don't need to validate that it's within the file bounds
because the seek or read will fail if we attempt to access that.

+        DLOG(ERROR) << "UDIF chunk data length " << i << " overflows";
+        return false;
+      }
+    }
+
+    return true;
   }
 
-  uint32_t signature() const { return block.signature; }
-  uint32_t version() const { return block.version; }
-  uint64_t start_sector() const { return block.start_sector; }
-  uint64_t sector_count() const { return block.sector_count; }
-  uint64_t chunk_count() const { return chunks.size(); }
+  uint32_t signature() const { return block_.signature; }
+  uint32_t version() const { return block_.version; }
+  uint64_t start_sector() const { return block_.start_sector; }
+  uint64_t sector_count() const { return block_.sector_count; }
+  uint64_t chunk_count() const { return chunks_.size(); }
 
   const UDIFBlockChunk* chunk(uint32_t i) const {
     if (i >= chunk_count())
       return nullptr;
-    return &chunks[i];
+    return &chunks_[i];
   }
 
  private:
-  UDIFBlockData block;
-  std::vector<UDIFBlockChunk> chunks;
+  const uint16_t sector_size_;

[Mark Mentovai, 2016/07/12 15:22:23]

Better if this is last, but I don’t think it needs to be a member at all. Pass
it into ParseBlockData(), the only place it’s used, instead of the constructor.

[Robert Sesek, 2016/07/12 16:16:35]

Done.

+  UDIFBlockData block_;
+  std::vector<UDIFBlockChunk> chunks_;
 
   DISALLOW_COPY_AND_ASSIGN(UDIFBlock);
 };
 
 #pragma pack(pop)
 
 namespace {
 
 const size_t kSectorSize = 512;
 
@@ skipping 124 matching lines @@
 
 std::unique_ptr<ReadStream> UDIFParser::GetPartitionReadStream(
     size_t part_number) {
   DCHECK_LT(part_number, blocks_.size());
   return base::WrapUnique(
       new UDIFPartitionReadStream(stream_, block_size_, blocks_[part_number]));
 }
 
 bool UDIFParser::ParseBlkx() {
   UDIFResourceFile trailer;
-  if (stream_->Seek(-sizeof(trailer), SEEK_END) == -1)
+  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;
+  }
+
   std::vector<uint8_t> plist_bytes(trailer.plist_length, 0);
 
   if (stream_->Seek(trailer.plist_offset, SEEK_SET) == -1)
     return false;
 
   if (!stream_->ReadExact(&plist_bytes[0], trailer.plist_length)) {
     DLOG(ERROR) << "Failed to read blkx plist data";
     return false;
   }
 
@@ skipping 47 matching lines @@
         base::mac::CFCast<CFDictionaryRef>(CFArrayGetValueAtIndex(blkx, i));
     auto data = base::mac::GetValueFromDictionary<CFDataRef>(block_dictionary,
                                                              CFSTR("Data"));
     if (!data) {
       DLOG(ERROR) << "Skipping block " << i
                   << " because it has no Data section";
       continue;
     }
 
     // Copy the block table out of the plist.
-    auto block_data =
-        reinterpret_cast<const UDIFBlockData*>(CFDataGetBytePtr(data));
-    std::unique_ptr<UDIFBlock> block(new UDIFBlock(block_data));
+    std::unique_ptr<UDIFBlock> block(new UDIFBlock(block_size_));
+    if (!block->ParseBlockData(
+            reinterpret_cast<const UDIFBlockData*>(CFDataGetBytePtr(data)))) {
+      DLOG(ERROR) << "Failed to parse UDIF block data";
+      return false;
+    }
 
     if (block->signature() != UDIFBlockData::kSignature) {
       DLOG(ERROR) << "Skipping block " << i << " because its signature does not"
                   << " match, is 0x" << std::hex << block->signature();
       continue;
     }
     if (block->version() != UDIFBlockData::kVersion) {
       DLOG(ERROR) << "Skipping block " << i << "because its version does not "
                   << "match, is " << block->version();
       continue;
@@ skipping 23 matching lines @@
       }
     }
 
     blocks_.push_back(std::move(block));
     partition_names_.push_back(partition_name);
   }
 
   return true;
 }
 
-bool UDIFParser::ReadBlockChunk(const UDIFBlockChunk* chunk,
-                                std::vector<uint8_t>* decompressed_data) {
-  UDIFBlockChunkReadStream chunk_read_stream(stream_, block_size_, chunk);
-  decompressed_data->resize(chunk_read_stream.length_in_bytes());
-  return chunk_read_stream.ReadExact(&(*decompressed_data)[0],
-                                     decompressed_data->size());
-}
-
 namespace {
 
 UDIFPartitionReadStream::UDIFPartitionReadStream(
     ReadStream* stream,
     uint16_t block_size,
     const UDIFBlock* partition_block)
     : stream_(stream),
       block_size_(block_size),
       block_(partition_block),
       current_chunk_(0),
@@ skipping 288 matching lines @@
                 << chunk_->compressed_offset;
     return false;
   }
   return true;
 }
 
 }  // namespace
 
 }  // namespace dmg
 }  // namespace safe_browsing