From 89a0a221a3a1b4ff5818e7ebaadd916624b200c5 Mon Sep 17 00:00:00 2001 From: Quin Gillespie Date: Sat, 31 Oct 2026 09:16:22 -0600 Subject: [PATCH] Repair cross-linked chains and bad directory links under Permissive validation Permissive validation already repairs a FAT entry that points past the end of the file by ending the chain there, but several other spec violations that Word and LibreOffice tolerate were still fatal in both modes. Under Permissive validation they are now repaired the same way: - A FAT longer than the file is truncated to the file's sectors. - A FAT or MiniFAT sector pointed to twice ends the later chain there, so only that chain is truncated. - A MiniFAT entry pointing past the end of the MiniFAT ends its chain. - A directory entry's sibling or child that is out of range, or a child on a stream entry, is read as no link. Strict validation still rejects all of these. In a sample of about 2,300 real-world .doc files (Govdocs1 and the LibreOffice bug tracker), these were the reasons 6 genuine Word documents failed to open. --- src/internal/alloc.rs | 62 +++++++++++++++++++++++++++---- src/internal/direntry.rs | 73 ++++++++++++++++++++++++++++++++---- src/internal/minialloc.rs | 78 ++++++++++++++++++++++++++++++--------- 3 files changed, 181 insertions(+), 32 deletions(-) diff --git a/src/internal/alloc.rs b/src/internal/alloc.rs index 9fdd8cb..02d762d 100644 --- a/src/internal/alloc.rs +++ b/src/internal/alloc.rs @@ -118,11 +118,16 @@ impl Allocator { fn validate(&mut self, validation: Validation) -> io::Result<()> { if self.fat.len() > self.sectors.num_sectors() as usize { - malformed!( - "FAT has {} entries, but file has only {} sectors", - self.fat.len(), - self.sectors.num_sectors() - ); + if validation.is_strict() { + malformed!( + "FAT has {} entries, but file has only {} sectors", + self.fat.len(), + self.sectors.num_sectors() + ); + } + // Entries past the end of the file describe sectors that aren't + // there; any chain running into them gets ended below. + self.fat.truncate(self.sectors.num_sectors() as usize); } for &difat_sector in self.difat_sector_ids.iter() { let difat_sector_index = difat_sector as usize; @@ -178,7 +183,14 @@ impl Allocator { continue; } if pointees.contains(&to_sector) { - malformed!("sector {} pointed to twice", to_sector); + if validation.is_strict() { + malformed!("sector {} pointed to twice", to_sector); + } + // Two chains run into the same sector. End this one + // here, as above, so that the chain that got there first + // keeps its sectors and only this one is truncated. + self.fat[from_sector] = consts::END_OF_CHAIN; + continue; } pointees.insert(to_sector); } else if to_sector == consts::INVALID_SECTOR { @@ -581,6 +593,28 @@ mod tests { .unwrap(); } + #[test] + fn fat_longer_than_file_permissive() { + let difat = vec![0]; + let fat = vec![consts::FAT_SECTOR, 2, consts::END_OF_CHAIN]; + let sectors = make_sectors(Version::V3, 2); + // The entry past the end is dropped, and the chain that ran into it + // ends where the file does. + let mut allocator = Allocator::new( + sectors, + vec![], + difat, + fat, + Validation::Permissive, + ) + .unwrap(); + assert_eq!( + allocator.fat, + vec![consts::FAT_SECTOR, consts::END_OF_CHAIN] + ); + allocator.validate(Validation::Strict).unwrap(); + } + #[test] #[should_panic( expected = "Malformed FAT (FAT has 2 entries, but DIFAT lists 3 as \ @@ -744,10 +778,22 @@ mod tests { #[test] #[should_panic(expected = "Malformed FAT (sector 3 pointed to twice)")] - fn double_pointee() { + fn double_pointee_strict() { let difat = vec![0]; let fat = vec![consts::FAT_SECTOR, 3, 3, consts::END_OF_CHAIN]; - make_allocator(difat, fat, Validation::Permissive); + make_allocator(difat, fat, Validation::Strict); + } + + #[test] + fn double_pointee_permissive() { + let difat = vec![0]; + let fat = vec![consts::FAT_SECTOR, 3, 3, consts::END_OF_CHAIN]; + // The second chain into sector 3 is ended where it would have joined + // the first, and the result passes Strict validation. + let mut allocator = make_allocator(difat, fat, Validation::Permissive); + assert_eq!(allocator.fat[1], 3); + assert_eq!(allocator.fat[2], consts::END_OF_CHAIN); + allocator.validate(Validation::Strict).unwrap(); } #[test] diff --git a/src/internal/direntry.rs b/src/internal/direntry.rs index 0831d27..89407f6 100644 --- a/src/internal/direntry.rs +++ b/src/internal/direntry.rs @@ -185,21 +185,36 @@ impl DirEntry { None => malformed!("invalid color: {}", color_byte), } }; - let left_sibling = reader.read_le_u32()?; + // Some writers leave a reserved value such as END_OF_CHAIN, or a + // child on a stream entry, where there should be no link; under + // Permissive validation such a link is read as no link at all. + let mut left_sibling = reader.read_le_u32()?; if left_sibling != NO_STREAM && left_sibling > MAX_REGULAR_STREAM_ID { - malformed!("invalid left sibling: {}", left_sibling); + if validation.is_strict() { + malformed!("invalid left sibling: {}", left_sibling); + } + left_sibling = NO_STREAM; } - let right_sibling = reader.read_le_u32()?; + let mut right_sibling = reader.read_le_u32()?; if right_sibling != NO_STREAM && right_sibling > MAX_REGULAR_STREAM_ID { - malformed!("invalid right sibling: {}", right_sibling); + if validation.is_strict() { + malformed!("invalid right sibling: {}", right_sibling); + } + right_sibling = NO_STREAM; } - let child = reader.read_le_u32()?; + let mut child = reader.read_le_u32()?; if child != NO_STREAM { if obj_type == ObjType::Stream { - malformed!("non-empty stream child: {}", child); + if validation.is_strict() { + malformed!("non-empty stream child: {}", child); + } + child = NO_STREAM; } else if child > MAX_REGULAR_STREAM_ID { - malformed!("invalid child: {}", child); + if validation.is_strict() { + malformed!("invalid child: {}", child); + } + child = NO_STREAM; } } @@ -588,6 +603,50 @@ mod tests { assert_eq!(dir_entry.creation_time, Timestamp::zero()); } + const INVALID_LINKS_ON_STREAM: [u8; consts::DIR_ENTRY_LEN] = [ + 70, 0, 111, 0, 111, 0, 98, 0, 97, 0, 114, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, // name + 14, 0, // name length + 2, // obj type + 1, // color, + 0xfe, 0xff, 0xff, 0xff, // left sibling + 34, 0, 0, 0, // right sibling + 5, 0, 0, 0, // child + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // CLSID + 0, 0, 0, 0, // state bits + 0, 0, 0, 0, 0, 0, 0, 0, // created + 0, 0, 0, 0, 0, 0, 0, 0, // modified + 0, 0, 0, 0, // start sector + 0, 0, 0, 0, 0, 0, 0, 0, // stream length + ]; + + #[test] + #[should_panic( + expected = "Malformed directory entry (invalid left sibling: \ + 4294967294)" + )] + fn invalid_links_on_stream_strict() { + let mut input: &[u8] = &INVALID_LINKS_ON_STREAM; + DirEntry::read_from(&mut input, Version::V4, Validation::Strict) + .unwrap(); + } + + #[test] + fn invalid_links_on_stream_permissive() { + let mut input: &[u8] = &INVALID_LINKS_ON_STREAM; + let dir_entry = DirEntry::read_from( + &mut input, + Version::V4, + Validation::Permissive, + ) + .unwrap(); + assert_eq!(dir_entry.left_sibling, consts::NO_STREAM); + assert_eq!(dir_entry.right_sibling, 34); + assert_eq!(dir_entry.child, consts::NO_STREAM); + } + const NON_ZERO_MODIFIED_TIME_ON_STREAM: [u8; consts::DIR_ENTRY_LEN] = [ 70, 0, 111, 0, 111, 0, 98, 0, 97, 0, 114, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, diff --git a/src/internal/minialloc.rs b/src/internal/minialloc.rs index 52533e3..08f537e 100644 --- a/src/internal/minialloc.rs +++ b/src/internal/minialloc.rs @@ -138,24 +138,33 @@ impl MiniAllocator { } } let mut pointees = FnvHashSet::default(); - for (from_mini_sector, &to_mini_sector) in - self.minifat.iter().enumerate() - { + for from_mini_sector in 0..self.minifat.len() { + let to_mini_sector = self.minifat[from_mini_sector]; if to_mini_sector <= consts::MAX_REGULAR_SECTOR { if to_mini_sector as usize >= self.minifat.len() { - malformed!( - "MiniFAT has {} entries, but mini sector {} points to \ - {}", - self.minifat.len(), - from_mini_sector, - to_mini_sector - ); + if validation.is_strict() { + malformed!( + "MiniFAT has {} entries, but mini sector {} \ + points to {}", + self.minifat.len(), + from_mini_sector, + to_mini_sector + ); + } + // As in the FAT, end the chain here so that only a chain + // running through this mini sector is truncated. + self.minifat[from_mini_sector] = consts::END_OF_CHAIN; + continue; } if pointees.contains(&to_mini_sector) { - malformed!( - "mini sector {} pointed to twice", - to_mini_sector - ); + if validation.is_strict() { + malformed!( + "mini sector {} pointed to twice", + to_mini_sector + ); + } + self.minifat[from_mini_sector] = consts::END_OF_CHAIN; + continue; } pointees.insert(to_mini_sector); } @@ -558,15 +567,26 @@ mod tests { use super::MiniAllocator; fn make_minialloc(minifat: Vec) -> MiniAllocator>> { + make_minialloc_with(minifat, Validation::Strict) + } + + fn make_minialloc_with( + minifat: Vec, + validation: Validation, + ) -> MiniAllocator>> { let root_stream_len = (consts::MINI_SECTOR_LEN * minifat.len()) as u64; - make_minialloc_with_root_stream_len(minifat, root_stream_len) + make_minialloc_with_root_stream_len( + minifat, + root_stream_len, + validation, + ) } fn make_minialloc_with_root_stream_len( minifat: Vec, root_stream_len: u64, + validation: Validation, ) -> MiniAllocator>> { - let validation = Validation::Strict; let version = Version::V3; let num_sectors = 4; // FAT, Directory, MiniFAT, and mini chain let data_len = (1 + num_sectors) * version.sector_len(); @@ -676,7 +696,11 @@ mod tests { fn root_stream_too_short() { let minifat = vec![1, 2, consts::END_OF_CHAIN]; let root_stream_len = (2 * consts::MINI_SECTOR_LEN) as u64; - make_minialloc_with_root_stream_len(minifat, root_stream_len); + make_minialloc_with_root_stream_len( + minifat, + root_stream_len, + Validation::Strict, + ); } #[test] @@ -689,6 +713,26 @@ mod tests { make_minialloc(minifat); } + #[test] + fn pointee_out_of_range_permissive() { + let minifat = vec![1, 3]; + let mut minialloc = + make_minialloc_with(minifat, Validation::Permissive); + assert_eq!(minialloc.minifat, vec![1, consts::END_OF_CHAIN]); + minialloc.validate(Validation::Strict).unwrap(); + } + + #[test] + fn double_pointee_permissive() { + let minifat = vec![1, 2, 1]; + // The chain from mini sector 2 back into 1 is ended there, leaving + // 0 -> 1 -> 2 intact. + let mut minialloc = + make_minialloc_with(minifat, Validation::Permissive); + assert_eq!(minialloc.minifat, vec![1, 2, consts::END_OF_CHAIN]); + minialloc.validate(Validation::Strict).unwrap(); + } + #[test] #[should_panic( expected = "Malformed MiniFAT (mini sector 1 pointed to twice)"