storytold/filmcraft
2
0
Fork 0
mirror of https://github.com/storytold/filmcraft.git synced 2026-11-02 18:09:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge PR #1718: cfb: reject NUL in entry names, checked sector arithmetic, allocation-free name compare

Browse Source
This commit is contained in:
aiongg
2026-10-31 02:14:06 +00:00
parent 9bae3a8fc4 158163650d
commit 985ffb285f
3 changed files with 121 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/cfb/src/lib.rs
+6 -5
View File
@@ -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<u16> = a.encode_utf16().collect();
let ub: Vec<u16> = 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(())
crates/cfb/src/read.rs
+26 -18
View File
@@ -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<usize>,
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<String>) -> 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<usize>) -> Result<Vec<u8>> {
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<u32> = 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);
crates/cfb/src/tests.rs
+89
View File
@@ -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<u16> = a.encode_utf16().collect();
let ub: Vec<u16> = 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));
}
}
}