From 356828e2157d030483c91b2774a59bd8e583556e Mon Sep 17 00:00:00 2001
From: Tiffany Song <[email protected]>
Date: Wed, 05 Aug 2026 08:43:54 -0700
Subject: [PATCH] safe_browsing: Use offset table and keyLength in HFS+ B-tree

HFSBTreeIterator walked catalog leaf records sequentially from the
BTNodeDescriptor and discarded each record's keyLength, assuming the
catalog data immediately follows the node name. Per TN1150, record i is
located via the trailing offset table at node[nodeSize - 2*(i+1)] and
its data begins at recordOffset + sizeof(keyLength) + keyLength. Images
that use either of those degrees of freedom were parsed differently from
other HFS+ implementations.

ReadCurrentLeaf() now reads and validates the numRecords + 1 entry
offset table into record_offsets_. Next() seeks to the current record's
offset, reads keyLength, and seeks to the resulting data offset before
reading the catalog record. Thread records no longer need their
variable-length body skipped, so the AdvanceLeafPast<T>() helper is
removed.

Also introduces HFSIterator::IsFile() and updates IsDirectory() to
safely encapsulate record type and pointer validation across all entry
inspection helpers.

Adds a unit test that builds a minimal HFS+ image in memory whose single
catalog leaf uses a padded keyLength on record 0 and places record 1 at
a non-contiguous offset reachable only via the offset table, and
verifies the iterator surfaces both entries and the file's data fork.

Internal CL: https://chrome-internal-review.git.corp.google.com/c/chrome/experimental/chromium/src/+/9625330

Fixed: 513473551
Change-Id: Icb5d5c1f2ed12ef6c0e81b80506333602c64a5f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8196986
Commit-Queue: Tiffany Song <[email protected]>
Reviewed-by: Javier Castro <[email protected]>
Reviewed-by: Yaw Frempong <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1674190}
---

diff --git a/chrome/utility/safe_browsing/mac/hfs.cc b/chrome/utility/safe_browsing/mac/hfs.cc
index bf214ab0..996130d7 100644
--- a/chrome/utility/safe_browsing/mac/hfs.cc
+++ b/chrome/utility/safe_browsing/mac/hfs.cc
@@ -227,12 +227,6 @@
   template <typename T>
   const T* GetLeafObjectHostEndian();
 
-  // Advances the position of `leaf_iterator_` past the next sizeof(T) bytes, if
-  // possible, and returns true. Returns false if the new position would exceed
-  // the total size of the `leaf_data_`.
-  template <typename T>
-  bool AdvanceLeafPast();
-
   // Checks if the HFS+ catalog key is a Mac OS X reserved key that should not
   // have it or its contents iterated over.
   bool IsKeyUnexported(const std::u16string& path);
@@ -267,6 +261,11 @@
   // Keeps track of our current position within the current `leaf_data_`.
   std::unique_ptr<base::BufferIterator<uint8_t>> leaf_iterator_;
 
+  // The per-record byte offsets into `leaf_data_` for the current leaf, taken
+  // from the offset table at the end of the node. There are `numRecords + 1`
+  // entries; the last entry marks the start of the node's free space.
+  std::vector<uint16_t> record_offsets_;
+
   // Points to the BTNodeDescriptor at the start of `leaf_data_`.
   raw_ptr<const BTNodeDescriptor> current_leaf_ = nullptr;
   // The record read at the current position of the `leaf_iterator_`.
@@ -326,8 +325,7 @@
     keep_going = catalog_->Next();
     if (keep_going) {
       if (!catalog_->current_record()->unexported &&
-          (catalog_->current_record()->record_type == kHFSPlusFolderRecord ||
-           catalog_->current_record()->record_type == kHFSPlusFileRecord)) {
+          (IsDirectory() || IsFile())) {
         return true;
       }
       keep_going = catalog_->HasNext();
@@ -338,27 +336,38 @@
 }
 
 bool HFSIterator::IsDirectory() {
-  return catalog_->current_record()->record_type == kHFSPlusFolderRecord;
+  return catalog_->current_record()->record_type == kHFSPlusFolderRecord &&
+         catalog_->current_record()->folder;
+}
+
+bool HFSIterator::IsFile() {
+  return catalog_->current_record()->record_type == kHFSPlusFileRecord &&
+         catalog_->current_record()->file;
 }
 
 bool HFSIterator::IsSymbolicLink() {
-  if (IsDirectory())
+  if (IsDirectory()) {
     return S_ISLNK(catalog_->current_record()->folder->bsdInfo.fileMode);
-  else
+  }
+  if (IsFile()) {
     return S_ISLNK(catalog_->current_record()->file->bsdInfo.fileMode);
+  }
+  return false;
 }
 
 bool HFSIterator::IsHardLink() {
-  if (IsDirectory())
+  if (IsDirectory()) {
     return false;
+  }
   const HFSPlusCatalogFile* file = catalog_->current_record()->file;
   return file->userInfo.fdType == kHardLinkFileType &&
          file->userInfo.fdCreator == kHFSPlusCreator;
 }
 
 bool HFSIterator::IsDecmpfsCompressed() {
-  if (IsDirectory())
+  if (IsDirectory()) {
     return false;
+  }
   const HFSPlusCatalogFile* file = catalog_->current_record()->file;
   return file->bsdInfo.ownerFlags & UF_COMPRESSED;
 }
@@ -368,8 +377,9 @@
 }
 
 std::unique_ptr<ReadStream> HFSIterator::GetReadStream() {
-  if (IsDirectory() || IsHardLink())
+  if (IsDirectory() || IsHardLink()) {
     return nullptr;
+  }
 
   DCHECK_EQ(kHFSPlusFileRecord, catalog_->current_record()->record_type);
   return std::make_unique<HFSForkReadStream>(
@@ -569,8 +579,28 @@
 
   CHECK(leaf_iterator_);
 
-  // Skip keyLength.
-  if (!AdvanceLeafPast<uint16_t>()) {
+  // Position at the start of the current record using the node's offset table.
+  if (static_cast<size_t>(current_leaf_records_read_) + 1u >=
+      record_offsets_.size()) {
+    return false;
+  }
+  const uint16_t record_offset = record_offsets_[current_leaf_records_read_];
+  const uint16_t record_end = record_offsets_[current_leaf_records_read_ + 1];
+  leaf_iterator_->Seek(record_offset);
+
+  auto key_length = CopyLeafDataHostEndian<uint16_t>();
+  if (!key_length.has_value()) {
+    return false;
+  }
+
+  // The data portion of a keyed record begins immediately after the key,
+  // located at `record_offset + sizeof(keyLength) + keyLength`.
+  base::CheckedNumeric<size_t> data_offset = record_offset;
+  data_offset += sizeof(uint16_t);
+  data_offset += *key_length;
+  if (*key_length > header_.maxKeyLength || !data_offset.IsValid() ||
+      data_offset.ValueOrDie() > record_end) {
+    DLOG(ERROR) << "Catalog record key extends past record";
     return false;
   }
 
@@ -583,6 +613,12 @@
   if (!key_string_length.has_value()) {
     return false;
   }
+  if (leaf_iterator_->position() +
+          static_cast<size_t>(*key_string_length) * sizeof(uint16_t) >
+      data_offset.ValueOrDie()) {
+    DLOG(ERROR) << "Catalog key node name extends past key";
+    return false;
+  }
 
   // Read and byte-swap the variable-length key string.
   std::u16string key(*key_string_length, '\0');
@@ -597,6 +633,7 @@
 
   // Read the record type and then rewind as the field is part of the catalog
   // structure that is read next.
+  leaf_iterator_->Seek(data_offset.ValueOrDie());
   size_t rewind_to = leaf_iterator_->position();
   auto record_type = CopyLeafDataHostEndian<int16_t>();
   if (!record_type.has_value()) {
@@ -609,6 +646,11 @@
 
   switch (current_record_.record_type) {
     case kHFSPlusFolderRecord: {
+      if ((data_offset + sizeof(HFSPlusCatalogFolder))
+              .ValueOrDefault(SIZE_MAX) > record_end) {
+        DLOG(ERROR) << "Folder record data extends past record";
+        return false;
+      }
       const HFSPlusCatalogFolder* folder =
           GetLeafObjectHostEndian<HFSPlusCatalogFolder>();
       if (!folder) {
@@ -642,6 +684,11 @@
       break;

diff --git a/chrome/utility/safe_browsing/mac/hfs_unittest.cc b/chrome/utility/safe_browsing/mac/hfs_unittest.cc
index ba975781..fbbced6 100644
--- a/chrome/utility/safe_browsing/mac/hfs_unittest.cc
+++ b/chrome/utility/safe_browsing/mac/hfs_unittest.cc
@@ -4,12 +4,14 @@
 
 #include "chrome/utility/safe_browsing/mac/hfs.h"
 
+#include <libkern/OSByteOrder.h>
 #include <stddef.h>
 #include <stdint.h>
 
 #include <array>
 #include <memory>
 #include <string_view>
+#include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/containers/span.h"
@@ -222,6 +224,250 @@
   EXPECT_EQ(0u, maybe_data->size());
 }
 
+// Builds a minimal HFS+ image in memory whose catalog leaf node lays out
+// records using the trailing record-offset table and a `keyLength` larger than
+// the catalog key's name would imply. The iterator must locate records via the
+// offset table and locate each record's data via `keyLength`, as described in
+// TN1150 "Node Structure".
+TEST(HFSBTreeIteratorTest, LeafRecordsLocatedByOffsetTableAndKeyLength) {
+  constexpr uint32_t kBlockSize = 4096;
+  constexpr uint16_t kNodeSize = 4096;
+  constexpr uint32_t kCatalogStartBlock = 1;
+  constexpr uint32_t kCatalogBlockCount = 2;
+  constexpr uint32_t kFileDataBlock = 3;
+  constexpr size_t kCatalogOffset = kCatalogStartBlock * kBlockSize;
+  constexpr size_t kLeafOffset = kCatalogOffset + kNodeSize;
+  constexpr std::string_view kFileData = "hello";
+
+  std::vector<uint8_t> image(4 * kBlockSize, 0);
+
+  auto put16 = [&](size_t at, uint16_t v) {
+    v = OSSwapHostToBigInt16(v);
+    base::span(image).subspan(at).copy_prefix_from(base::byte_span_from_ref(v));
+  };
+  auto put32 = [&](size_t at, uint32_t v) {
+    v = OSSwapHostToBigInt32(v);
+    base::span(image).subspan(at).copy_prefix_from(base::byte_span_from_ref(v));
+  };
+
+  // HFSPlusVolumeHeader at byte 1024.
+  constexpr size_t kVH = 1024;
+  HFSPlusVolumeHeader header = {};
+  header.signature = OSSwapHostToBigInt16(kHFSPlusSigWord);  // signature
+  header.version = OSSwapHostToBigInt16(kHFSPlusVersion);    // version
+  header.blockSize = OSSwapHostToBigInt32(kBlockSize);       // blockSize
+  header.totalBlocks =
+      OSSwapHostToBigInt32(image.size() / kBlockSize);  // totalBlocks
+
+  header.catalogFile.logicalSize =
+      OSSwapHostToBigInt64(kCatalogBlockCount * kBlockSize);
+  header.catalogFile.totalBlocks = OSSwapHostToBigInt32(kCatalogBlockCount);
+  header.catalogFile.extents[0].startBlock =
+      OSSwapHostToBigInt32(kCatalogStartBlock);
+  header.catalogFile.extents[0].blockCount =
+      OSSwapHostToBigInt32(kCatalogBlockCount);
+  base::span(image)
+      .subspan(kVH, sizeof(header))
+      .copy_from(base::byte_span_from_ref(header));
+
+  // Catalog header node (node 0): BTNodeDescriptor + BTHeaderRec.
+  BTNodeDescriptor header_descriptor = {};
+  header_descriptor.kind = kBTHeaderNode;
+  header_descriptor.numRecords = OSSwapHostToBigInt16(3);
+  base::span(image)
+      .subspan(kCatalogOffset, sizeof(header_descriptor))
+      .copy_from(base::byte_span_from_ref(header_descriptor));
+
+  BTHeaderRec header_rec = {};
+  header_rec.treeDepth = OSSwapHostToBigInt16(1);
+  header_rec.rootNode = OSSwapHostToBigInt32(1);
+  header_rec.leafRecords = OSSwapHostToBigInt32(2);
+  header_rec.firstLeafNode = OSSwapHostToBigInt32(1);
+  header_rec.lastLeafNode = OSSwapHostToBigInt32(1);
+  header_rec.nodeSize = OSSwapHostToBigInt16(kNodeSize);
+  header_rec.maxKeyLength =
+      OSSwapHostToBigInt16(kHFSPlusCatalogKeyMaximumLength);
+  header_rec.totalNodes = OSSwapHostToBigInt32(2);
+  base::span(image)
+      .subspan(kCatalogOffset + sizeof(header_descriptor), sizeof(header_rec))
+      .copy_from(base::byte_span_from_ref(header_rec));
+
+  // Catalog leaf node (node 1).
+  BTNodeDescriptor leaf_descriptor = {};
+  leaf_descriptor.kind = static_cast<uint8_t>(kBTLeafNode);
+  leaf_descriptor.height = 1;
+  leaf_descriptor.numRecords = OSSwapHostToBigInt16(2);
+  base::span(image)
+      .subspan(kLeafOffset, sizeof(leaf_descriptor))
+      .copy_from(base::byte_span_from_ref(leaf_descriptor));
+
+  // Record 0: root folder. The key declares a length larger than the bytes
+  // occupied by parentID + nodeName, leaving padding before the data.
+  constexpr uint16_t kRecord0 = sizeof(leaf_descriptor);
+  constexpr uint16_t kRecord0KeyLength = 32;
+  put16(kLeafOffset + kRecord0, kRecord0KeyLength);     // keyLength
+  put32(kLeafOffset + kRecord0 + 2, kHFSRootParentID);  // parentID
+  put16(kLeafOffset + kRecord0 + 6, 1);                 // nodeName.length
+  put16(kLeafOffset + kRecord0 + 8, 'V');               // nodeName.unicode[0]
+
+  constexpr size_t kFolderData =
+      kLeafOffset + kRecord0 + sizeof(uint16_t) + kRecord0KeyLength;
+  HFSPlusCatalogFolder folder = {};
+  folder.recordType = OSSwapHostToBigInt16(kHFSPlusFolderRecord);
+  folder.folderID = OSSwapHostToBigInt32(kHFSRootFolderID);
+  base::span(image)
+      .subspan(kFolderData, sizeof(folder))
+      .copy_from(base::byte_span_from_ref(folder));
+  static_assert(sizeof(HFSPlusCatalogFolder) == 88);
+  constexpr uint16_t kRecord0End = kRecord0 + sizeof(uint16_t) +
+                                   kRecord0KeyLength +
+                                   sizeof(HFSPlusCatalogFolder);
+
+  // Record 1: file. Placed at a non-contiguous offset relative to record 0.
+  constexpr uint16_t kRecord1 = 600;
+  static_assert(kRecord1 > kRecord0End);
+  constexpr uint16_t kRecord1KeyLength = 8;
+  put16(kLeafOffset + kRecord1, kRecord1KeyLength);     // keyLength
+  put32(kLeafOffset + kRecord1 + 2, kHFSRootFolderID);  // parentID
+  put16(kLeafOffset + kRecord1 + 6, 1);                 // nodeName.length
+  put16(kLeafOffset + kRecord1 + 8, 'f');               // nodeName.unicode[0]
+
+  constexpr size_t kFileRec =
+      kLeafOffset + kRecord1 + sizeof(uint16_t) + kRecord1KeyLength;
+  HFSPlusCatalogFile file = {};
+  file.recordType = OSSwapHostToBigInt16(kHFSPlusFileRecord);
+  file.fileID = OSSwapHostToBigInt32(kHFSFirstUserCatalogNodeID);
+  file.dataFork.logicalSize = OSSwapHostToBigInt64(kFileData.size());
+  file.dataFork.totalBlocks = OSSwapHostToBigInt32(1);
+  file.dataFork.extents[0].startBlock = OSSwapHostToBigInt32(kFileDataBlock);
+  file.dataFork.extents[0].blockCount = OSSwapHostToBigInt32(1);
+  base::span(image)
+      .subspan(kFileRec, sizeof(file))
+      .copy_from(base::byte_span_from_ref(file));
+
+  static_assert(sizeof(file) == 248);
+  constexpr uint16_t kRecord1End =
+      kRecord1 + sizeof(uint16_t) + kRecord1KeyLength + sizeof(file);
+
+  // Record offset table at the end of the leaf, in reverse order.
+  put16(kLeafOffset + kNodeSize - 2, kRecord0);
+  put16(kLeafOffset + kNodeSize - 4, kRecord1);
+  put16(kLeafOffset + kNodeSize - 6, kRecord1End);
+
+  // File data fork contents.
+  base::span(image)
+      .subspan(kFileDataBlock * kBlockSize)
+      .copy_prefix_from(base::as_byte_span(kFileData));
+
+  MemoryReadStream stream(image);
+  HFSIterator hfs_reader(&stream);
+  EXPECT_TRUE(hfs_reader.Open());
+
+  bool next1 = hfs_reader.Next();
+  EXPECT_TRUE(next1);
+  if (next1) {
+    EXPECT_TRUE(hfs_reader.IsDirectory());
+    EXPECT_FALSE(hfs_reader.IsFile());
+    EXPECT_EQ(u"V", hfs_reader.GetPath());
+  }
+
+  bool next2 = hfs_reader.Next();
+  EXPECT_TRUE(next2);
+  if (next2) {
+    EXPECT_FALSE(hfs_reader.IsDirectory());
+    EXPECT_TRUE(hfs_reader.IsFile());
+    EXPECT_FALSE(hfs_reader.IsHardLink());
+    EXPECT_EQ(u"V/f", hfs_reader.GetPath());
+
+    std::unique_ptr<ReadStream> file_stream = hfs_reader.GetReadStream();
+    EXPECT_TRUE(file_stream);
+    if (file_stream) {
+      auto data = ReadEntireStream(*file_stream);
+      EXPECT_TRUE(data.has_value());
+      if (data.has_value()) {
+        EXPECT_EQ(kFileData, base::as_string_view(*data));
+      }
+    }
+  }
+
+  EXPECT_FALSE(hfs_reader.Next());
+}
+
+TEST(HFSBTreeIteratorTest, InconsistentOffsetTableRejected) {
+  constexpr uint32_t kBlockSize = 4096;
+  constexpr uint16_t kNodeSize = 4096;
+  constexpr uint32_t kCatalogStartBlock = 1;
+  constexpr uint32_t kCatalogBlockCount = 2;
+  constexpr size_t kCatalogOffset = kCatalogStartBlock * kBlockSize;
+  constexpr size_t kLeafOffset = kCatalogOffset + kNodeSize;
+
+  std::vector<uint8_t> image(3 * kBlockSize, 0);
+
+  auto put16 = [&](size_t at, uint16_t v) {
+    v = OSSwapHostToBigInt16(v);
+    base::span(image).subspan(at).copy_prefix_from(base::byte_span_from_ref(v));
+  };
+
+  // HFSPlusVolumeHeader at byte 1024.
+  constexpr size_t kVH = 1024;
+  HFSPlusVolumeHeader header = {};
+  header.signature = OSSwapHostToBigInt16(kHFSPlusSigWord);
+  header.version = OSSwapHostToBigInt16(kHFSPlusVersion);
+  header.blockSize = OSSwapHostToBigInt32(kBlockSize);
+  header.totalBlocks = OSSwapHostToBigInt32(image.size() / kBlockSize);
+
+  header.catalogFile.logicalSize =
+      OSSwapHostToBigInt64(kCatalogBlockCount * kBlockSize);
+  header.catalogFile.totalBlocks = OSSwapHostToBigInt32(kCatalogBlockCount);
+  header.catalogFile.extents[0].startBlock =
+      OSSwapHostToBigInt32(kCatalogStartBlock);
+  header.catalogFile.extents[0].blockCount =
+      OSSwapHostToBigInt32(kCatalogBlockCount);
+  base::span(image)
+      .subspan(kVH, sizeof(header))
+      .copy_from(base::byte_span_from_ref(header));
+
+  // Catalog header node (node 0): BTNodeDescriptor + BTHeaderRec.
+  BTNodeDescriptor header_descriptor = {};
+  header_descriptor.kind = kBTHeaderNode;
+  header_descriptor.numRecords = OSSwapHostToBigInt16(3);
+  base::span(image)
+      .subspan(kCatalogOffset, sizeof(header_descriptor))
+      .copy_from(base::byte_span_from_ref(header_descriptor));
+
+  BTHeaderRec header_rec = {};
+  header_rec.treeDepth = OSSwapHostToBigInt16(1);
+  header_rec.rootNode = OSSwapHostToBigInt32(1);
+  header_rec.leafRecords = OSSwapHostToBigInt32(1);
+  header_rec.firstLeafNode = OSSwapHostToBigInt32(1);
+  header_rec.lastLeafNode = OSSwapHostToBigInt32(1);
+  header_rec.nodeSize = OSSwapHostToBigInt16(kNodeSize);
+  header_rec.maxKeyLength =
+      OSSwapHostToBigInt16(kHFSPlusCatalogKeyMaximumLength);
+  header_rec.totalNodes = OSSwapHostToBigInt32(2);
+  base::span(image)
+      .subspan(kCatalogOffset + sizeof(header_descriptor), sizeof(header_rec))
+      .copy_from(base::byte_span_from_ref(header_rec));
+
+  // Catalog leaf node (node 1).
+  BTNodeDescriptor leaf_descriptor = {};
+  leaf_descriptor.kind = static_cast<uint8_t>(kBTLeafNode);
+  leaf_descriptor.height = 1;
+  leaf_descriptor.numRecords = OSSwapHostToBigInt16(1);
+  base::span(image)
+      .subspan(kLeafOffset, sizeof(leaf_descriptor))
+      .copy_from(base::byte_span_from_ref(leaf_descriptor));
+
+  // Record offset table: set offset[0] to an invalid small value (< 14).
+  put16(kLeafOffset + kNodeSize - 2, 10);
+  put16(kLeafOffset + kNodeSize - 4, 100);
+
+  MemoryReadStream stream(image);
+  HFSIterator hfs_reader(&stream);
+  EXPECT_TRUE(hfs_reader.Open());
+  EXPECT_FALSE(hfs_reader.Next());
+}
+
 INSTANTIATE_TEST_SUITE_P(HFSIteratorTest,
                          HFSFileReadTest,
                          testing::Values("hfs_plus.img",