From cca7733f21005899f5bf149e823ded6f747263ba Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 14 Nov 2026 07:30:14 +0000 Subject: [PATCH] perf(xfl): cache candidate directory validation --- crates/xfl/src/lib.rs | 65 +++++++++++++++++++++++++++-------------- crates/xfl/src/tests.rs | 22 ++++++++++++++ 2 files changed, 65 insertions(+), 22 deletions(-) diff --git a/crates/xfl/src/lib.rs b/crates/xfl/src/lib.rs index 55b786f9a..83fdad40b 100644 --- a/crates/xfl/src/lib.rs +++ b/crates/xfl/src/lib.rs @@ -251,6 +251,9 @@ fn find_end(data: &[u8]) -> Result<(u64, u64, u64, usize), ZipError> { // last resort while searching for an older, structurally stronger candidate. let mut empty_sfx = None; let mut fallback = None; + // Fake EOCD records in a comment can all describe the same large directory. Cache binary + // directory jumps so checking each candidate does not rescan every entry. + let mut directory_jumps = HashMap::new(); for candidate in (lo..data.len().saturating_sub(21)).rev() { if u32_at(data, candidate) != Some(END) || !u16_at(data, candidate + 20) @@ -272,7 +275,7 @@ fn find_end(data: &[u8]) -> Result<(u64, u64, u64, usize), ZipError> { let dir_size = u64_at(data, at + 40)?; let dir_offset = u64_at(data, at + 48)?; locate_directory(data, count, dir_size, dir_offset, at) - .filter(|&(dir, _)| directory_is_exact(data, dir, dir_size, count)) + .filter(|&(dir, _)| directory_is_exact(data, dir, dir_size, count, &mut directory_jumps)) .map(|(_, shift)| (at, count == 0 && shift > 0)) }); let Some((_, shifted_empty)) = valid else { @@ -284,7 +287,7 @@ fn find_end(data: &[u8]) -> Result<(u64, u64, u64, usize), ZipError> { } } else { let Some((_, shift)) = locate_directory(data, count.into(), size.into(), offset.into(), candidate) - .filter(|&(dir, _)| directory_is_exact(data, dir, size.into(), count.into())) + .filter(|&(dir, _)| directory_is_exact(data, dir, size.into(), count.into(), &mut directory_jumps)) else { continue; }; @@ -371,9 +374,35 @@ fn locate_directory(data: &[u8], count: u64, size: u64, stated: u64, limit: usiz None } +fn central_entry_end(data: &[u8], cursor: usize) -> Option { + if u32_at(data, cursor) != Some(CENTRAL) { + return None; + } + let header_end = cursor.checked_add(46).filter(|&at| at <= data.len())?; + let name_len = usize::from(u16_at(data, header_end - 18)?); + let extra_len = usize::from(u16_at(data, header_end - 16)?); + let comment_len = usize::from(u16_at(data, header_end - 14)?); + header_end.checked_add(name_len)?.checked_add(extra_len)?.checked_add(comment_len).filter(|&end| end <= data.len()) +} + +/// Return the offset after `2^power` contiguous central-directory records, memoizing offsets +/// shared by competing EOCD candidates in untrusted comments. +fn central_directory_jump(data: &[u8], start: usize, power: u8, jumps: &mut HashMap<(usize, u8), Option>) -> Option { + if let Some(&cached) = jumps.get(&(start, power)) { + return cached; + } + let end = if power == 0 { + central_entry_end(data, start) + } else { + central_directory_jump(data, start, power - 1, jumps).and_then(|middle| central_directory_jump(data, middle, power - 1, jumps)) + }; + jumps.insert((start, power), end); + end +} + /// Checks that the declared central-directory entries consume the entire directory. This is -/// used while choosing among EOCD-shaped signatures, where a comment may resemble an end record. -fn directory_is_exact(data: &[u8], start: usize, size: u64, count: u64) -> bool { +/// used while choosing among EOCD-shaped signatures, where comments may contain many fake records. +fn directory_is_exact(data: &[u8], start: usize, size: u64, count: u64, jumps: &mut HashMap<(usize, u8), Option>) -> bool { let Ok(size) = usize::try_from(size) else { return false }; let Some(end) = start.checked_add(size).filter(|&end| end <= data.len()) else { return false }; // Preserve the existing TooMany diagnostic in `Archive::open` without walking attacker-sized @@ -382,25 +411,17 @@ fn directory_is_exact(data: &[u8], start: usize, size: u64, count: u64) -> bool return true; } let mut cursor = start; - for _ in 0..count { - let Some(header_end) = cursor.checked_add(46).filter(|&at| at <= end) else { - return false; - }; - if u32_at(data, cursor) != Some(CENTRAL) { - return false; + let mut remaining = count; + let mut power = 0; + while remaining != 0 { + if remaining & 1 != 0 { + let Some(next) = central_directory_jump(data, cursor, power, jumps) else { + return false; + }; + cursor = next; } - let Some(name_len) = u16_at(data, header_end - 18).map(usize::from) else { return false }; - let Some(extra_len) = u16_at(data, header_end - 16).map(usize::from) else { return false }; - let Some(comment_len) = u16_at(data, header_end - 14).map(usize::from) else { return false }; - let Some(entry_end) = header_end - .checked_add(name_len) - .and_then(|at| at.checked_add(extra_len)) - .and_then(|at| at.checked_add(comment_len)) - .filter(|&at| at <= end) - else { - return false; - }; - cursor = entry_end; + remaining >>= 1; + power += 1; } cursor == end } diff --git a/crates/xfl/src/tests.rs b/crates/xfl/src/tests.rs index c141a3b9d..ee4b2cf33 100644 --- a/crates/xfl/src/tests.rs +++ b/crates/xfl/src/tests.rs @@ -280,6 +280,28 @@ fn nonempty_comment_eocd_candidate_does_not_mask_valid_archive() { assert_eq!(archive.read("a.xml").unwrap().unwrap(), b"x"); } +#[test] +fn repeated_eocd_directory_candidates_reuse_entry_validation() { + let entries = (0..1024).map(|index| Raw::new(&format!("entry-{index:04}.xml"), b"", false)).collect::>(); + let mut bytes = write(&entries, false); + let eocd = bytes.len() - 22; + bytes[eocd + 20..eocd + 22].copy_from_slice(&1000u16.to_le_bytes()); + bytes.extend_from_slice(&vec![0; 1000]); + let directory_start = u32_at(&bytes, eocd + 16).unwrap() as usize; + let directory_size = u64::from(u32_at(&bytes, eocd + 12).unwrap()); + let count = u64::from(u16_at(&bytes, eocd + 10).unwrap()); + let mut jumps = std::collections::HashMap::new(); + + assert!(directory_is_exact(&bytes, directory_start, directory_size, count, &mut jumps)); + let indexed_records = jumps.len(); + // Each size models a fake shifted EOCD in the archive comment that starts at the same + // central-directory record but claims additional bytes through the real EOCD. + for extra in 1..=1000 { + assert!(!directory_is_exact(&bytes, directory_start, directory_size + extra, count, &mut jumps)); + } + assert_eq!(jumps.len(), indexed_records, "repeated candidates must reuse the indexed entry jumps"); +} + #[test] fn deeply_nested_folders_are_refused() { let ok = format!("{}a.xml", "f/".repeat(MAX_DEPTH));