diff --git a/CHANGELOG.md b/CHANGELOG.md index e50b8c3bc..3e1185b39 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/odr/internal/cfb/cfb_impl.cpp b/src/odr/internal/cfb/cfb_impl.cpp index 114795682..d6a67e27f 100644 --- a/src/odr/internal/cfb/cfb_impl.cpp +++ b/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(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(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(); } diff --git a/src/odr/internal/cfb/cfb_util.cpp b/src/odr/internal/cfb/cfb_util.cpp index 67c1e39be..3c49a2c3c 100644 --- a/src/odr/internal/cfb/cfb_util.cpp +++ b/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(m_entry.size)) { + throw FileReadError(); + } + return static_cast(m_entry.size); + } [[nodiscard]] std::string name() const override { return m_entry.get_name(); } diff --git a/test/src/internal/cfb/cfb_archive_test.cpp b/test/src/internal/cfb/cfb_archive_test.cpp index 19cdd95c3..f4fb09c89 100644 --- a/test/src/internal/cfb/cfb_archive_test.cpp +++ b/test/src/internal/cfb/cfb_archive_test.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -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,8 +131,14 @@ 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(std::move(bytes))); + std::make_shared(directory_archive())); std::vector paths; for (const auto &entry : archive) { paths.push_back(entry.path().string()); @@ -137,3 +146,44 @@ TEST(CfbArchive, nested_children_finish_before_outer_siblings) { EXPECT_EQ(paths, (std::vector{"", "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(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(std::make_shared(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)); +}