From 46cb7eeaf019db2308fa6fae473133536cc21eb6 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Fri, 13 Nov 2026 23:41:15 +0000 Subject: [PATCH] fix(xfl): validate archive record boundaries --- crates/xfl/src/lib.rs | 15 ++++++++++++--- crates/xfl/src/tests.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/crates/xfl/src/lib.rs b/crates/xfl/src/lib.rs index 5629a0e0f..ac6dcbc4c 100644 --- a/crates/xfl/src/lib.rs +++ b/crates/xfl/src/lib.rs @@ -203,12 +203,12 @@ fn clean_path(name: &str) -> Result { return Err(ZipError::Unsafe(shown())); } let mut parts = Vec::new(); - for (i, part) in unix.split('/').enumerate() { + for part in unix.split('/') { match part { "" | "." => {} ".." => return Err(ZipError::Unsafe(shown())), // `C:`, `C:x`: a drive. - p if i == 0 && p.contains(':') => return Err(ZipError::Unsafe(shown())), + p if parts.is_empty() && p.contains(':') => return Err(ZipError::Unsafe(shown())), p => parts.push(p), } } @@ -234,7 +234,13 @@ fn method_name(method: u16) -> &'static str { fn find_end(data: &[u8]) -> Result<(u64, u64, u64, usize), ZipError> { // The end record is in the last 22 + 65535 bytes (it may carry a comment). let lo = data.len().saturating_sub(22 + 65_535); - let eocd = (lo..data.len().saturating_sub(21)).rev().find(|&i| u32_at(data, i) == Some(END)).ok_or(ZipError::NotZip)?; + let eocd = (lo..data.len().saturating_sub(21)) + .rev() + .find(|&i| { + u32_at(data, i) == Some(END) + && u16_at(data, i + 20).is_some_and(|comment| i.checked_add(22 + usize::from(comment)).is_some_and(|end| end <= data.len())) + }) + .ok_or(ZipError::NotZip)?; let field16 = |o: usize| u16_at(data, eocd + o).ok_or(ZipError::Damaged("the end record is cut short")); let field32 = |o: usize| u32_at(data, eocd + o).ok_or(ZipError::Damaged("the end record is cut short")); let (disk, dir_disk, on_disk, count) = (field16(4)?, field16(6)?, field16(8)?, field16(10)?); @@ -346,6 +352,9 @@ impl<'a> Archive<'a> { return Err(ZipError::Duplicate(shown())); } } + if i != dir.len() { + return Err(ZipError::Damaged("the central directory has trailing data")); + } // No two entries may share bytes (the overlapping-entries zip bomb). spans.sort_unstable_by_key(|s| s.0); if let Some(w) = spans.windows(2).find(|w| w[0].1 > w[1].0) { diff --git a/crates/xfl/src/tests.rs b/crates/xfl/src/tests.rs index e8e37b353..7f981d772 100644 --- a/crates/xfl/src/tests.rs +++ b/crates/xfl/src/tests.rs @@ -219,6 +219,37 @@ fn duplicate_names_are_refused() { assert_eq!(Archive::open(&z).unwrap().counts(), (1, 2)); } +#[test] +fn drive_prefix_after_normalized_dot_segments_is_refused() { + for name in ["./C:/LIBRARY/a.xml", "./C:\\LIBRARY\\a.xml"] { + assert!(matches!(Archive::open(&zip(&[(name, b"x", false)])).err(), Some(ZipError::Unsafe(_)))); + } + assert!(Archive::open(&zip(&[("./LIBRARY/a.xml", b"x", false)])).is_ok()); +} + +#[test] +fn invalid_eocd_comment_candidate_does_not_mask_archive_end() { + let mut z = zip(&[("a.xml", b"x", false)]); + let end = z.len() - 22; + z[end + 20..end + 22].copy_from_slice(&22u16.to_le_bytes()); + let fake = z.len(); + z.extend_from_slice(&END.to_le_bytes()); + z.extend_from_slice(&[0; 16]); + z.extend_from_slice(&u16::MAX.to_le_bytes()); + assert!(Archive::open(&z).is_ok()); + assert_eq!(fake + 22, z.len()); +} + +#[test] +fn central_directory_must_be_consumed_exactly() { + let mut z = zip(&[("a.xml", b"x", false)]); + let end = z.len() - 22; + let size = u32::from_le_bytes(z[end + 12..end + 16].try_into().unwrap()); + z.insert(end, 0); + z[end + 12 + 1..end + 16 + 1].copy_from_slice(&(size + 1).to_le_bytes()); + assert_eq!(Archive::open(&z).err(), Some(ZipError::Damaged("the central directory has trailing data"))); +} + #[test] fn deeply_nested_folders_are_refused() { let ok = format!("{}a.xml", "f/".repeat(MAX_DEPTH));