From 158163650dce96b26eb374ee1c53688f90cdab4c Mon Sep 17 00:00:00 2001 From: vangel76 Date: Tue, 13 Oct 2026 20:51:37 +0200 Subject: [PATCH] cfb: reject NUL in entry names, checked sector arithmetic, allocation-free name compare (#1716) validate_name accepted U+0000, which the NUL-terminated directory entry cuts short on read-back. Sector and mini-sector offsets use checked arithmetic (32-bit usize overflow). compare_names no longer allocates. Dead FREESECT and step-counter checks removed, read_chain pre-sizes its buffer bounded by the file length, Entry::children doc corrected. Co-Authored-By: Claude Sonnet 5.5 --- crates/cfb/src/lib.rs | 11 ++--- crates/cfb/src/read.rs | 44 +++++++++++--------- crates/cfb/src/tests.rs | 89 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 23 deletions(-) diff --git a/crates/cfb/src/lib.rs b/crates/cfb/src/lib.rs index 12e376c20..ebb77fe34 100644 --- a/crates/cfb/src/lib.rs +++ b/crates/cfb/src/lib.rs @@ -102,15 +102,16 @@ fn upper(u: u16) -> u16 { /// The sibling order of directory entries (§2.6.4): shorter names first, then a code-unit /// comparison of the upper-cased names. pub fn compare_names(a: &str, b: &str) -> Ordering { - let ua: Vec = a.encode_utf16().collect(); - let ub: Vec = b.encode_utf16().collect(); - ua.len().cmp(&ub.len()).then_with(|| ua.iter().map(|&u| upper(u)).cmp(ub.iter().map(|&u| upper(u)))) + // No allocation: the iterators are walked in place (`child` calls this once per sibling). + let (la, lb) = (a.encode_utf16().count(), b.encode_utf16().count()); + la.cmp(&lb).then_with(|| a.encode_utf16().map(upper).cmp(b.encode_utf16().map(upper))) } -/// Check a directory entry name (§2.6.1): 1–31 UTF-16 code units, none of `/ \ : !`. +/// Check a directory entry name (§2.6.1): 1–31 UTF-16 code units, none of `/ \ : !`, and no NUL +/// (the stored name is NUL-terminated, so a NUL would cut the name short when read back). pub fn validate_name(name: &str) -> Result<()> { let n = name.encode_utf16().count(); - if n == 0 || n > MAX_NAME_LEN || name.contains(['/', '\\', ':', '!']) { + if n == 0 || n > MAX_NAME_LEN || name.contains(['/', '\\', ':', '!', '\0']) { return Err(Error::BadName(name.to_string())); } Ok(()) diff --git a/crates/cfb/src/read.rs b/crates/cfb/src/read.rs index 5ba3ed683..3773cdbd9 100644 --- a/crates/cfb/src/read.rs +++ b/crates/cfb/src/read.rs @@ -1,6 +1,6 @@ //! Reading compound files ([MS-CFB] §2.2–2.6). -use crate::{ENDOFCHAIN, Error, FREESECT, MAXREGSECT, MINI_SECTOR_SIZE, NOSTREAM, Result, SIGNATURE, compare_names}; +use crate::{ENDOFCHAIN, Error, MAXREGSECT, MINI_SECTOR_SIZE, NOSTREAM, Result, SIGNATURE, compare_names}; /// Directory entry object type (§2.6.1). #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -26,7 +26,9 @@ pub struct Entry { /// First sector (regular or mini) of a stream; the mini stream's for the root. pub start: u32, pub size: u64, - /// Child entries of a storage, in sibling-tree order (sorted by [`compare_names`]). + /// Child entries of a storage, in the in-order position of its sibling tree. For a well-formed + /// file that is sorted by [`compare_names`]; the reader does not validate the tree's order, so + /// a hostile file may give any order (lookups by name do not depend on it). pub children: Vec, left: u32, right: u32, @@ -59,6 +61,13 @@ fn u64_at(b: &[u8], at: usize) -> u64 { u64::from_le_bytes(b[at..at + 8].try_into().unwrap_or([0; 8])) } +/// Byte range `[start, end)` of unit `index + skip` of `unit` bytes each, or `None` when the +/// arithmetic overflows `usize` (reachable on 32-bit targets such as wasm32). +pub(crate) fn span(index: usize, skip: usize, unit: usize) -> Option<(usize, usize)> { + let start = index.checked_add(skip)?.checked_mul(unit)?; + Some((start, start.checked_add(unit)?)) +} + fn invalid(s: impl Into) -> Error { Error::Invalid(s.into()) } @@ -157,23 +166,25 @@ impl<'a> CompoundFile<'a> { /// Sector `n` (a whole sector; a truncated last sector is an error). fn sector(&self, n: u32) -> Result<&'a [u8]> { - let start = (n as usize + 1).checked_mul(self.sector_size).ok_or_else(|| invalid("sector number overflow"))?; - let end = start.checked_add(self.sector_size).ok_or_else(|| invalid("sector number overflow"))?; - if n > MAXREGSECT || end > self.data.len() { + if n > MAXREGSECT { return Err(invalid(format!("sector {n} is outside the file"))); } - Ok(&self.data[start..end]) + // sector 0 follows the header sector + let (start, end) = usize::try_from(n).ok().and_then(|i| span(i, 1, self.sector_size)).ok_or_else(|| invalid("sector number overflow"))?; + self.data.get(start..end).ok_or_else(|| invalid(format!("sector {n} is outside the file"))) } /// Concatenate a regular sector chain; `limit` stops once that many bytes are read. fn read_chain(&self, start: u32, limit: Option) -> Result> { - let mut out = Vec::new(); + // `limit` is at most the file length (callers check), and an unlimited chain holds at most + // `max_steps` sectors, so this capacity is bounded by the file's own size. + let max_steps = self.data.len() / self.sector_size + 1; + let mut out = Vec::with_capacity(limit.unwrap_or(0).min(self.data.len())); let mut s = start; let mut steps = 0usize; // A chain cannot visit more sectors than the file holds. Not `self.fat.len()`: the FAT can // be far longer (one FAT sector named by many header / DIFAT slots), and a looping chain // would then append a whole sector per FAT entry (hundreds of MB, or an abort). - let max_steps = self.data.len() / self.sector_size + 1; while s != ENDOFCHAIN { if s > MAXREGSECT { return Err(invalid(format!("bad sector {s:#x} in a chain"))); @@ -187,10 +198,8 @@ impl<'a> CompoundFile<'a> { if limit.is_some_and(|l| out.len() >= l) { break; } + // a free entry (`FREESECT`) is above `MAXREGSECT` and is rejected at the top of the loop s = *self.fat.get(s as usize).ok_or_else(|| invalid(format!("sector {s} has no FAT entry")))?; - if s == FREESECT { - return Err(invalid("chain runs into a free sector")); - } } if let Some(l) = limit { if out.len() < l { @@ -213,8 +222,11 @@ impl<'a> CompoundFile<'a> { if steps > self.minifat.len().max(1) { return Err(invalid("mini sector chain loops")); } - let at = s as usize * MINI_SECTOR_SIZE; - let b = self.mini_stream.get(at..at + MINI_SECTOR_SIZE).ok_or_else(|| invalid("mini sector outside the mini stream"))?; + let b = usize::try_from(s) + .ok() + .and_then(|i| span(i, 0, MINI_SECTOR_SIZE)) + .and_then(|(start, end)| self.mini_stream.get(start..end)) + .ok_or_else(|| invalid("mini sector outside the mini stream"))?; out.extend_from_slice(b); s = *self.minifat.get(s as usize).ok_or_else(|| invalid("mini sector has no mini FAT entry"))?; } @@ -234,7 +246,7 @@ impl<'a> CompoundFile<'a> { // in-order walk of the sibling tree, iterative, cycle-safe let mut stack: Vec = Vec::new(); let mut cur = self.entries[parent].child; - let mut steps = 0; + // terminates without a step counter: every push claims an unclaimed entry in `owner` while cur != NOSTREAM || !stack.is_empty() { while cur != NOSTREAM { let i = cur as usize; @@ -244,10 +256,6 @@ impl<'a> CompoundFile<'a> { owner[i] = true; stack.push(cur); cur = self.entries[i].left; - steps += 1; - if steps > n { - return Err(invalid("directory tree loops")); - } } if let Some(top) = stack.pop() { kids.push(top as usize); diff --git a/crates/cfb/src/tests.rs b/crates/cfb/src/tests.rs index 7e9854fe1..3f82d5229 100644 --- a/crates/cfb/src/tests.rs +++ b/crates/cfb/src/tests.rs @@ -291,3 +291,92 @@ fn chains_filling_the_file_still_read() { assert_eq!(CompoundFile::open(&b).unwrap().read_path("big").unwrap(), data); } } + +#[test] +fn nul_in_a_name_is_rejected() { + assert!(validate_name("a\0b").is_err()); + assert!(validate_name("\0").is_err()); + let mut w = Writer::default(); + assert!(matches!(w.stream(Writer::ROOT, "a\0b", vec![1]), Err(Error::BadName(_)))); + assert!(matches!(w.storage(Writer::ROOT, "x\0", [0; 16]), Err(Error::BadName(_)))); +} + +#[test] +fn span_is_checked() { + use crate::read::span; + assert_eq!(span(0, 1, 512), Some((512, 1024))); + assert_eq!(span(3, 0, 64), Some((192, 256))); + // On a 32-bit `usize` these are the overflows of `sector` and `read_mini_chain`; the checked + // arithmetic is the same code on 64-bit, so usize::MAX exercises it here. (A real 32-bit + // target is not run.) + assert_eq!(span(usize::MAX, 1, 512), None); + assert_eq!(span(usize::MAX, 0, 64), None); + assert_eq!(span(usize::MAX / 64, 0, 64), None, "start fits, end does not"); + assert_eq!(span(usize::MAX - 1, 1, 1), None); +} + +/// The allocating comparison compare_names used before the iterator rewrite. +fn compare_names_old(a: &str, b: &str) -> std::cmp::Ordering { + let ua: Vec = a.encode_utf16().collect(); + let ub: Vec = b.encode_utf16().collect(); + ua.len().cmp(&ub.len()).then_with(|| ua.iter().map(|&u| upper(u)).cmp(ub.iter().map(|&u| upper(u)))) +} + +#[test] +fn compare_names_matches_the_allocating_version() { + let names = [ + "", + "a", + "A", + "b", + "B", + "ab", + "AB", + "aB", + "abc", + "ABD", + "Abc", + "aBC", + "z", + "Z", + "{", + "_", + "ß", + "SS", + "ss", + "ü", + "Ü", + "é", + "É", + "dž", + "Dž", + "DŽ", + "ストリーム", + "ｓｔ", + "Ｓｔ", + "😀", + "😁", + "a😀", + "A😀", + "\u{ffff}", + "\u{10000}", + "Header-2", + "HEADER-2", + "header-2", + "Mobs-1901{a}", + "mobs-1901{A}", + ]; + for a in names { + for b in names { + assert_eq!(compare_names(a, b), compare_names_old(a, b), "{a:?} vs {b:?}"); + } + } + // every BMP scalar against a few fixed names + for c in (0u32..=0x10FFFF).filter_map(char::from_u32) { + let s = c.to_string(); + for other in ["a", "Z", "ü", "dž"] { + assert_eq!(compare_names(&s, other), compare_names_old(&s, other)); + assert_eq!(compare_names(other, &s), compare_names_old(other, &s)); + } + } +}