From e1c84a471cf77b0028f7537f6b2343b329e3f009 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 14 Nov 2026 07:13:44 +0000 Subject: [PATCH] fix(xfl): validate shifted EOCD candidates --- crates/xfl/src/lib.rs | 45 ++++++++++++++++++++++++++++++++++++++--- crates/xfl/src/tests.rs | 27 +++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/crates/xfl/src/lib.rs b/crates/xfl/src/lib.rs index 488e69ceb..55b786f9a 100644 --- a/crates/xfl/src/lib.rs +++ b/crates/xfl/src/lib.rs @@ -271,7 +271,9 @@ fn find_end(data: &[u8]) -> Result<(u64, u64, u64, usize), ZipError> { let count = u64_at(data, at + 32)?; 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).map(|(_, shift)| (at, count == 0 && shift > 0)) + locate_directory(data, count, dir_size, dir_offset, at) + .filter(|&(dir, _)| directory_is_exact(data, dir, dir_size, count)) + .map(|(_, shift)| (at, count == 0 && shift > 0)) }); let Some((_, shifted_empty)) = valid else { continue; @@ -281,7 +283,9 @@ fn find_end(data: &[u8]) -> Result<(u64, u64, u64, usize), ZipError> { continue; } } else { - let Some((_, shift)) = locate_directory(data, count.into(), size.into(), offset.into(), candidate) 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())) + else { continue; }; if count == 0 && shift > 0 { @@ -338,9 +342,10 @@ fn find_zip64_end(data: &[u8], locator: usize) -> Option { if !shifted { offset.checked_add(size) == Some(at) } else { + let empty_directory = size == 0 && u64_at(data, at + 24) == Some(0) && u64_at(data, at + 32) == Some(0); at > stated && offset.checked_add(size) == Some(stated) - && at.checked_sub(size).is_some_and(|dir| dir > offset && u32_at(data, dir) == Some(CENTRAL)) + && at.checked_sub(size).is_some_and(|dir| dir > offset && (empty_directory || u32_at(data, dir) == Some(CENTRAL))) } }; if valid(stated, false) { @@ -366,6 +371,40 @@ fn locate_directory(data: &[u8], count: u64, size: u64, stated: u64, limit: usiz None } +/// 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 { + 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 + // entry counts here. + if count > MAX_ENTRIES as u64 { + 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 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; + } + cursor == end +} + impl<'a> Archive<'a> { pub fn open(data: &'a [u8]) -> Result, ZipError> { let (count, dir_size, dir_at, dir_limit) = find_end(data)?; diff --git a/crates/xfl/src/tests.rs b/crates/xfl/src/tests.rs index 76de11450..c141a3b9d 100644 --- a/crates/xfl/src/tests.rs +++ b/crates/xfl/src/tests.rs @@ -261,6 +261,25 @@ fn central_directory_must_be_consumed_exactly() { assert_eq!(Archive::open(&z).err(), Some(ZipError::Damaged("the central directory has trailing data"))); } +#[test] +fn nonempty_comment_eocd_candidate_does_not_mask_valid_archive() { + let mut bytes = zip(&[("a.xml", b"x", false)]); + let end = bytes.len() - 22; + // The fake EOCD sits in the real EOCD's comment and claims the real directory plus the + // real EOCD as a shifted directory. Candidate selection must reject its trailing bytes. + bytes[end + 20..end + 22].copy_from_slice(&22u16.to_le_bytes()); + bytes.extend_from_slice(&END.to_le_bytes()); + bytes.extend_from_slice(&[0; 4]); + bytes.extend_from_slice(&1u16.to_le_bytes()); + bytes.extend_from_slice(&1u16.to_le_bytes()); + bytes.extend_from_slice(&73u32.to_le_bytes()); + bytes.extend_from_slice(&0u32.to_le_bytes()); + bytes.extend_from_slice(&0u16.to_le_bytes()); + + let archive = Archive::open(&bytes).unwrap(); + assert_eq!(archive.read("a.xml").unwrap().unwrap(), b"x"); +} + #[test] fn deeply_nested_folders_are_refused() { let ok = format!("{}a.xml", "f/".repeat(MAX_DEPTH)); @@ -339,6 +358,14 @@ fn zip64_count_sentinel_keeps_a_comment_eocd_from_masking_the_archive() { assert_eq!(archive.read("a.xml").unwrap().unwrap(), b"x"); } +#[test] +fn empty_relative_zip64_directory_opens_after_sfx_stub() { + let mut bytes = b"MZ self-extracting stub".to_vec(); + bytes.extend_from_slice(&write_at(&[], true, 0)); + let archive = Archive::open(&bytes).unwrap(); + assert_eq!(archive.names().count(), 0); +} + #[test] fn self_extracting_archives_support_relative_and_adjusted_offsets() { let entries = [Raw::new("DOMDocument.xml", b"", true), Raw::new("LIBRARY/a.xml", b"", false)];