fix: validate CFB mini streams and legacy sizes

CHANGELOG.md

@@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- CFB validates header fields, directory names, and mini-stream bounds. Version
+  3 files tolerate the uninitialized high size word allowed by the format.
+
 - ZIP preserves full entry names, distinguishes iterators from different
   archives, and isolates miniz error state during concurrent reads. Entry
   sizes and stream offsets are checked before narrowing.

src/odr/internal/cfb/cfb_impl.cpp

@@ -41,7 +41,8 @@ namespace odr::internal::cfb::impl {
 std::string CompoundFileEntry::get_name() const {
   // [MS-CFB] 2.6.1: `name_len` counts bytes including the terminating NUL and
   // never exceeds the 64-byte name field.
-  if (name_len > sizeof(name)) {
+  if (name_len > sizeof(name) || name_len % 2 != 0 ||
+      (name_len >= 2 && name[name_len / 2 - 1] != u'\0')) {
     throw CfbFileCorrupted();
   }
   if (name_len < 2) {
@@ -65,6 +66,10 @@ CompoundFileReader::CompoundFileReader(std::istream &in,
       !(m_header.major_version == 4 && m_header.sector_shift == 12)) {
     throw CfbFileCorrupted();
   }
+  if (m_header.byte_order != 0xfffe || m_header.mini_sector_shift != 6 ||
+      m_header.mini_stream_cutoff_size != 4096) {
+    throw CfbFileCorrupted();
+  }
   m_sector_size = std::uint64_t{1} << m_header.sector_shift;
 
   // The file must contain at least 3 sectors
@@ -73,6 +78,9 @@ CompoundFileReader::CompoundFileReader(std::istream &in,
   }
 
   parse_entry(in, RootId, m_root);
+  if (m_root.type != 5) {
+    throw CfbFileCorrupted();
+  }
 
   m_mini_stream_start_sector = m_root.start_sector_location;
 }
@@ -92,6 +100,10 @@ void CompoundFileReader::parse_entry(std::istream &in,
   const std::uint64_t address = sector_offset_to_address(sector_offset);
   in.seekg(static_cast<std::streampos>(address));
   impl::parse_entry(in, entry);
+  if (m_header.major_version == 3) {
+    // [MS-CFB] 2.6.1: older writers leave the high DWORD uninitialized.
+    entry.size = static_cast<std::uint32_t>(entry.size);
+  }
 }
 
 CompoundFileEntry
@@ -117,6 +129,9 @@ void CompoundFileReader::read_file(std::istream &in,
         " > " + std::to_string(entry.size - offset));
   }
 
+  if (len == 0) {
+    return;
+  }
   if (entry.size < m_header.mini_stream_cutoff_size) {
     read_mini_stream(in, {entry.start_sector_location, offset}, buffer, len);
   } else {
@@ -165,7 +180,11 @@ void CompoundFileReader::read_mini_stream(std::istream &in,
         mini_sector_offset_to_address(in, current_sector_offset);
     const std::size_t copy_length =
         std::min(length, m_mini_sector_size - current_sector_offset.offset);
-    if (address + copy_length > m_file_size) {
+    const std::uint64_t mini_offset =
+        current_sector_offset.offset +
+        current_sector_offset.sector * m_mini_sector_size;
+    if (copy_length > m_root.size - mini_offset ||
+        address + copy_length > m_file_size) {
       throw CfbFileCorrupted();
     }
 
@@ -226,7 +245,7 @@ std::uint64_t CompoundFileReader::mini_sector_offset_to_address(
       sector_offset.offset + sector_offset.sector * m_mini_sector_size;
 
   if (sector_offset.sector >= MaxSector ||
-      sector_offset.offset >= m_mini_sector_size || address >= m_file_size) {
+      sector_offset.offset >= m_mini_sector_size || address >= m_root.size) {
     throw CfbFileCorrupted();
   }
 

src/odr/internal/cfb/cfb_util.cpp

@@ -114,7 +114,12 @@ class FileInCfb final : public abstract::File {
   [[nodiscard]] FileLocation location() const noexcept override {
     return m_archive->file()->location();
   }
-  [[nodiscard]] std::size_t size() const override { return m_entry.size; }
+  [[nodiscard]] std::size_t size() const override {
+    if (!std::in_range<std::size_t>(m_entry.size)) {
+      throw FileReadError();
+    }
+    return static_cast<std::size_t>(m_entry.size);
+  }
 
   [[nodiscard]] std::string name() const override { return m_entry.get_name(); }
 

test/src/internal/cfb/cfb_archive_test.cpp

@@ -15,6 +15,7 @@
 #include <cstring>
 #include <limits>
 #include <memory>
+#include <sstream>
 #include <string_view>
 #include <vector>
 
@@ -80,7 +81,9 @@ TEST(CfbArchive, an_entry_stream_outlives_its_file_wrapper) {
   EXPECT_EQ(internal::util::stream::read(*stream), expected);
 }
 
-TEST(CfbArchive, nested_children_finish_before_outer_siblings) {
+namespace {
+
+std::string directory_archive() {
   impl::CompoundFileHeader header{};
   std::memcpy(header.signature.data(), impl::CompoundFileReader::MAGIC, 8);
   header.minor_version = 0x3e;
@@ -128,12 +131,59 @@ TEST(CfbArchive, nested_children_finish_before_outer_siblings) {
   std::memcpy(bytes.data(), &header, sizeof(header));
   std::memcpy(bytes.data() + 512, fat.data(), sizeof(fat));
   std::memcpy(bytes.data() + 1024, entries.data(), sizeof(entries));
+  return bytes;
+}
+
+} // namespace
+
+TEST(CfbArchive, nested_children_finish_before_outer_siblings) {
   const cfb::util::Archive archive(
-      std::make_shared<MemoryFile>(std::move(bytes)));
+      std::make_shared<MemoryFile>(directory_archive()));
   std::vector<std::string> paths;
   for (const auto &entry : archive) {
     paths.push_back(entry.path().string());
   }
   EXPECT_EQ(paths,
             (std::vector<std::string>{"", "AA", "AA/D", "BB", "CC", "DD"}));
 }
+
+TEST(CfbArchive, rejects_unsupported_header_layouts) {
+  for (const std::size_t field : {28U, 32U, 56U}) {
+    auto bytes = directory_archive();
+    bytes[field] ^= 1;
+    EXPECT_THROW((void)cfb::util::Archive(std::make_shared<MemoryFile>(bytes)),
+                 CfbFileCorrupted);
+  }
+}
+
+TEST(CfbArchive, names_have_even_lengths_and_a_terminator) {
+  impl::CompoundFileEntry entry{};
+  entry.name[0] = u'a';
+  entry.name_len = 3;
+  EXPECT_THROW((void)entry.get_name(), CfbFileCorrupted);
+  entry.name_len = 2;
+  EXPECT_THROW((void)entry.get_name(), CfbFileCorrupted);
+  entry.name_len = 4;
+  EXPECT_EQ(entry.get_name(), "a");
+}
+
+TEST(CfbArchive, version_three_ignores_the_high_size_word) {
+  auto bytes = directory_archive();
+  constexpr std::uint64_t size = (std::uint64_t{0x12345678} << 32) | 7;
+  std::memcpy(bytes.data() + 1024 + 3 * 128 + 120, &size, sizeof(size));
+  const auto archive =
+      std::make_shared<cfb::util::Archive>(std::make_shared<MemoryFile>(bytes));
+  EXPECT_EQ(archive->find(RelPath("AA/D"))->file()->size(), 7);
+}
+
+TEST(CfbArchive, mini_sectors_must_be_inside_the_root_stream) {
+  const auto bytes = directory_archive();
+  std::istringstream in(bytes);
+  const impl::CompoundFileReader reader(in, bytes.size());
+  impl::CompoundFileEntry entry{};
+  entry.size = 1;
+  entry.start_sector_location = 0;
+  char byte{};
+  EXPECT_THROW(reader.read_file(in, entry, 0, &byte, 1), CfbFileCorrupted);
+  EXPECT_NO_THROW(reader.read_file(in, entry, 1, &byte, 0));
+}