From a1be18b5483c979f80f2111845dafbcac81ef02a Mon Sep 17 00:00:00 2001 From: "A. Molzer" <5550310+197g@users.noreply.github.com> Date: Thu, 10 Jul 2025 18:00:16 +0200 Subject: [PATCH 1/3] Add subsubifds test file --- tests/COPYRIGHT | 4 ++++ tests/subsubifds.rs | 33 +++++++++++++++++++++++++++++++++ tests/subsubifds.tif | Bin 0 -> 4774 bytes 3 files changed, 37 insertions(+) create mode 100644 tests/COPYRIGHT create mode 100644 tests/subsubifds.rs create mode 100644 tests/subsubifds.tif diff --git a/tests/COPYRIGHT b/tests/COPYRIGHT new file mode 100644 index 0000000..064bfe0 --- /dev/null +++ b/tests/COPYRIGHT @@ -0,0 +1,4 @@ +subsubifd.tif: + url: https://data.kitware.com/api/v1/file/hashsum/sha512/372ca32735c8a8fdbe2286dabead9e779da63f10ba81eda84625e5273f76d74ca1a47a978f67e9c00c12f7f72009c7b2c07a641e643bb0c463812f4ae7f15d6e/download + credit: https://github.com/DigitalSlideArchive/tifftools/commit/4d00c70dbce828262f86c1431f7c66b1748965ac + license: CC0 diff --git a/tests/subsubifds.rs b/tests/subsubifds.rs new file mode 100644 index 0000000..fc72e8c --- /dev/null +++ b/tests/subsubifds.rs @@ -0,0 +1,33 @@ +use tiff::{decoder::Decoder, tags::IfdPointer}; + +// This file has the following IFD structure: +// +// ``` +// IFD -> IFD -> IFD +// |-> IFD +// |-> IFD -> IFD +// | \-> IFD -> IFD +// \-> IFD +// ``` +// +// where the main chain of IFD has 3 images, the second of which has several sub-IFDs and some have +// more nested IFDs. +const TEST_IMAGE_SUBIFD: &str = "./tests/subsubifds.tif"; +use std::fs::File; + +#[test] +fn decode_seek_chain() { + let file = File::open(TEST_IMAGE_SUBIFD).expect("Cannot open test image"); + let mut decoder = Decoder::new(file).expect("Invalid format to create decoder"); + + decoder.next_image().unwrap(); + decoder.next_image().unwrap(); + assert!(!decoder.more_images()); + + // Ensure we can seek, to do this again. + decoder.seek_to_image(0) + .expect("Failed to seek to first image"); + decoder.next_image().unwrap(); + decoder.next_image().unwrap(); + assert!(!decoder.more_images()); +} diff --git a/tests/subsubifds.tif b/tests/subsubifds.tif new file mode 100644 index 0000000000000000000000000000000000000000..51c99d56e82a447e275e7a8d2d378260c3db279b GIT binary patch literal 4774 zcmd^Ddr(tn7XNNS5`;hy5JG89e2uQ60=`!P(h42!PT$1H7~qTIx) zsI(GsTHOuxF08VDgvQ{LXVE=?IpHs^N(om|xfx405+7F*Z`-igeto^<`C z>pEL5<+#-bb#PAo{^&5~V?Fbh(0)aIew&nyR&U*4aL!P!oI@DuMtsA@&H4Ku)-=Pv z0N^BLZ-rvvxT1XIBah0&HO52G36o(z7tYo^q_<0d zj8hQOoZKku-BIv|IzmRaZ~+5H!*T10XrG~-Ler4fh(A6N6e0NI{vC)zVecCyzNI{{ z0I>kydE+DOfA5WBaX)sg;!g?4u--X@45z$^7h*G7pvRmM{n)iuh{rmih+Xq*&|aWK zy?dxb9Nv%X5x?lM&IFHjUiEnYYY)5>vC9vI&oTu4!~2ts z72@J6yK7b9mKZ1aCKUcEA^&BTq^!Rj?;qtG`YhD58O**DCAmpIzm&7e!5poPZI~y zi0*DU3!lScXy{RGi&M>$9Lw4jraa}R8C`P9k@VqVi1^(DmZp{6F~$+X;?5pur>7VT zcKrrq5llVX>1B$YlOVA6bGtoD{`$7_A;B~IlJ$~pTq!i{z9A!97nQeoNX9JG6tXhG zRIVFByCK+;NA0=TubFjsBq1z-R$}U;){IlKFKCj)))Dg>X;vk-%<TRF}dyX#*d?oHoRt}bfLVX$y^{pzq`I_rUlsZN-guHY1@``~9892%o5hBIZ zvIp%T+76QS9QCpVz43ZEg5CzBm$NvZ*GmOrUN7Gu9*SN{uqN@TX6~bj-cv6VTE+1p zp?aJ82U*llx8Rb-R9=}H%%)eRHbJ-WW}V?ewO2b-9)=%jt+G38PsAoBCDIOSrYBmR z*{n)ioJ9^dSN&Wk03Ou>Lmj9VWhGz3#FC-RRNQ2eKqfZj?(ZXe^a^t|bbz*2mpuu$ zyu=zZkjjChG6~iT{IpeQo6u6oq9+nN^m^h-1Ye*xo>&^a4TjiUoX-<`05MN&A>wBt z)>Thjy~GCG$TKB{sK+YGV>Ann^wiD0XCQq!p`KGL$P*c`y6i2o4NMGW^7-&qm za@f~IJW(7hljJv{Bh2EE?8)zv300*w06NBaQ8vFL;NT8@m($ECq&mgIec#W$4$?no zd7WWlSbPU%Jktff$I`LH2y**UMUiC+$!oj2zQT7VG{9X#u8l8$uU9~;Kd6T?R?BW+ zR1GlP5DK|4UUVlTSb-GB1^=+FzAA8KeHdgFDx5cM<|9WTTAypEF)^EysP{46?t zns#e(V?QUT*#DWL{-3p*2x;839$0WTOB^{i2~vI9L7vPKZfWam_tA3o=RauoaRxEb z=Cxc{E+wP|Z9RtBh~^U5Xd-`30$Yrpc!3>7Z-Wt7InL(=R*9Gwm<91r1cthI3oP!j zj_I@lnxKiRXXs8yPq^us9bRO@Dg%mP_}|BRPiH_7@g+6ot7e$~TQ=t|lWEAX!D*Pv zYQ?7Wm8K~5-wIsUQjs<2lmn=`reVcX_04l4)~ z^Yx$H)Bz-kH7YLGN1fW&W^sz}E?msNcQL^ui#1VJ%(I-iRTX4$h_8G@5;Fl9l|rK( zTLe{fRAjiA>1E?oKLA`2Bai8S*I? z=5zjdrI8VWEg!jumPd9!vEU9pQDHtIC(s+Oqh|Cr7#$J3M)ErHM$GFd0P!!RqiFuZ zd;om8Ol}sF)D0nt%vx@m`p|cOp` literal 0 HcmV?d00001 From 43d7f66c9ef13da76f183dba4738b8f593f75bc1 Mon Sep 17 00:00:00 2001 From: "A. Molzer" <5550310+197g@users.noreply.github.com> Date: Thu, 10 Jul 2025 23:12:46 +0200 Subject: [PATCH 2/3] Refactor cycle and IFD tracking The previous strategy could not keep track of all cycles such as reflexive ones at the very start. Additionally, it would fail to be able to iterate after seeking since it was overly cautious and verified the next IFD position before allowing the read of a current IFD. The new implementation is furthermore proof against having multiple independent chains of IFD which may occur when iterating SubIFDs for thumbnails. (Note that we do not verify the tree-structure, only a freedom from cycles). --- src/decoder/cycles.rs | 169 ++++++++++++++++++++++++++++++++++++++++++ src/decoder/mod.rs | 49 +++++++----- tests/subsubifds.rs | 9 ++- 3 files changed, 208 insertions(+), 19 deletions(-) create mode 100644 src/decoder/cycles.rs diff --git a/src/decoder/cycles.rs b/src/decoder/cycles.rs new file mode 100644 index 0000000..b28880d --- /dev/null +++ b/src/decoder/cycles.rs @@ -0,0 +1,169 @@ +use crate::{tags::IfdPointer, TiffError, TiffFormatError}; +use std::collections::HashMap; + +/// The IFD structure of a TIFF file should be limited to a forest of entries, and a tree when we +/// only consider having a primary IFD and its children. There is up to one primary child of a node +/// that is the `next` entry in an IFD itself. Since we offer iteration over this chain we want to +/// detect when a new edge would introduce a cycle, such that callers can be guaranteed to +/// terminate iteration at some point even in malicious images. +/// +/// However, we are not necessarily visiting the IFDs in their topological order. +/// +/// Since we only consider one child, we run union find to assign each pointer to a known chain. +/// Then, when we insert a new edge we check if they belong to the same component. +#[derive(Default, Debug)] +pub struct IfdCycles { + /// The root of each component union. + component_union: HashMap, + links: HashMap, + chains: HashMap, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +struct ComponentId(u64); + +impl IfdCycles { + pub fn new() -> Self { + IfdCycles::default() + } + + pub fn insert_next( + &mut self, + from: IfdPointer, + to: Option, + ) -> Result { + let to_offset = to.map_or(0, |p| p.0); + + // For some reason we got + match self.links.get(&from) { + Some(existing) if *existing == to_offset => return Ok(false), + // We got here twice with two different reads of the same IFD. That is .. unusual and + // we interpret it as a cycle. + Some(_) => return Err(TiffError::FormatError(TiffFormatError::CycleInOffsets)), + None => self.links.insert(from, to_offset), + }; + + self.ensure_node(from); + + if let Some(to) = to { + self.ensure_node(to); + + let parent = self.nominal_component(from); + let child = self.nominal_component(to); + + if parent == child { + return Err(TiffError::FormatError(TiffFormatError::CycleInOffsets)); + } + + self.component_union.insert(child, parent); + } + + Ok(true) + } + + fn ensure_node(&mut self, ifd: IfdPointer) { + self.chains.entry(ifd).or_insert_with(|| { + let id = ComponentId(self.component_union.len() as u64); + self.component_union.insert(id, id); + id + }); + } + + fn nominal_component(&mut self, node: IfdPointer) -> ComponentId { + let id = self.chains[&node]; + + let nomimal = { + let mut iter = id; + + loop { + let parent = self.component_union[&iter]; + + if parent == iter { + break parent; + } + + iter = parent; + } + }; + + if nomimal != id { + // Compress. + let mut iter = id; + + loop { + let parent = self.component_union[&iter]; + + if parent == iter { + break; + } + + self.component_union.insert(iter, nomimal); + iter = parent; + } + } + + nomimal + } +} + +#[test] +fn cycles_are_detected() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x800))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x800), Some(IfdPointer(0x20))) + .expect_err("cycle must be detected"); +} + +#[test] +fn reflective_cycle() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x20))) + .expect_err("self-referential cycle must be detected"); +} + +#[test] +fn late_cycle() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x40))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x60), Some(IfdPointer(0x80))) + .expect("non-existing link is valid"); + cycles + .insert_next(IfdPointer(0x80), Some(IfdPointer(0x20))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x40), Some(IfdPointer(0x60))) + .expect_err("non-existing link is valid"); +} + +#[test] +fn odd_cycle() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x40))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x60), Some(IfdPointer(0x80))) + .expect("non-existing link is valid"); + cycles + .insert_next(IfdPointer(0x80), Some(IfdPointer(0x20))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x40), Some(IfdPointer(0x80))) + .expect_err("non-existing link is valid"); +} diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index f6f4fa3..66f4b9a 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashSet}; +use std::collections::BTreeMap; use std::io::{self, Read, Seek}; use crate::tags::{ @@ -14,6 +14,7 @@ use half::f16; use self::image::Image; use self::stream::{ByteOrder, EndianReader}; +mod cycles; pub mod ifd; mod image; mod stream; @@ -263,9 +264,12 @@ where reader: EndianReader, bigtiff: bool, limits: Limits, + current_ifd: Option, next_ifd: Option, + /// The IFDs we visited already in this chain of IFDs. ifd_offsets: Vec, - seen_ifds: HashSet, + /// Map from the ifd into the `ifd_offsets` ordered list. + seen_ifds: cycles::IfdCycles, image: Image, } @@ -495,9 +499,8 @@ impl Decoder { } .map(IfdPointer); - let mut seen_ifds = HashSet::new(); - seen_ifds.insert(*next_ifd.as_ref().unwrap()); - let ifd_offsets = vec![*next_ifd.as_ref().unwrap()]; + let current_ifd = *next_ifd.as_ref().unwrap(); + let ifd_offsets = vec![current_ifd]; let mut decoder = Decoder { reader, @@ -505,7 +508,8 @@ impl Decoder { limits: Default::default(), next_ifd, ifd_offsets, - seen_ifds, + current_ifd: None, + seen_ifds: cycles::IfdCycles::new(), image: Image { ifd: None, width: 0, @@ -542,6 +546,11 @@ impl Decoder { self.image().colortype() } + /// The offset of the directory representing the current image. + pub fn ifd_pointer(&mut self) -> Option { + self.current_ifd + } + fn image(&self) -> &Image { &self.image } @@ -552,6 +561,8 @@ impl Decoder { if ifd_index >= self.ifd_offsets.len() { // We possibly need to load in the next IFD if self.next_ifd.is_none() { + self.current_ifd = None; + return Err(TiffError::FormatError( TiffFormatError::ImageFileDirectoryNotFound, )); @@ -574,6 +585,8 @@ impl Decoder { // If the index is within the list of ifds then we can load the selected image/IFD if let Some(ifd_offset) = self.ifd_offsets.get(ifd_index) { let ifd = Self::read_ifd(&mut self.reader, self.bigtiff, *ifd_offset)?; + self.next_ifd = ifd.next(); + self.current_ifd = Some(*ifd_offset); self.image = Image::from_reader(&mut self.reader, ifd, &self.limits, self.bigtiff)?; Ok(()) @@ -591,20 +604,20 @@ impl Decoder { )); } - let ifd = Self::read_ifd( - &mut self.reader, - self.bigtiff, - self.next_ifd.take().unwrap(), - )?; + let ifd_offset = self.next_ifd.take().unwrap(); + let ifd = Self::read_ifd(&mut self.reader, self.bigtiff, ifd_offset)?; - if let Some(next) = ifd.next() { - if !self.seen_ifds.insert(next) { - return Err(TiffError::FormatError(TiffFormatError::CycleInOffsets)); - } - self.next_ifd = Some(next); - self.ifd_offsets.push(next); + // Ensure this walk does not get us into a cycle. + self.seen_ifds.insert_next(ifd_offset, ifd.next())?; + + // Extend the list of known IFD offsets in this chain, if needed. + if self.ifd_offsets.last().copied() == self.current_ifd { + self.ifd_offsets.push(ifd_offset); } + self.current_ifd = Some(ifd_offset); + self.next_ifd = ifd.next(); + Ok(ifd) } @@ -622,7 +635,7 @@ impl Decoder { self.next_ifd.is_some() } - /// Returns the byte_order + /// Returns the byte_order of the file. pub fn byte_order(&self) -> ByteOrder { self.reader.byte_order } diff --git a/tests/subsubifds.rs b/tests/subsubifds.rs index fc72e8c..7dfd2ca 100644 --- a/tests/subsubifds.rs +++ b/tests/subsubifds.rs @@ -1,4 +1,4 @@ -use tiff::{decoder::Decoder, tags::IfdPointer}; +use tiff::decoder::Decoder; // This file has the following IFD structure: // @@ -20,14 +20,21 @@ fn decode_seek_chain() { let file = File::open(TEST_IMAGE_SUBIFD).expect("Cannot open test image"); let mut decoder = Decoder::new(file).expect("Invalid format to create decoder"); + // Remember the offsets in the first iteration. + let offset0 = decoder.ifd_pointer().expect("First IFD pointer not found"); decoder.next_image().unwrap(); + let offset1 = decoder.ifd_pointer().expect("Second IFD pointer not found"); decoder.next_image().unwrap(); + let offset2 = decoder.ifd_pointer().expect("Third IFD pointer not found"); assert!(!decoder.more_images()); // Ensure we can seek, to do this again. decoder.seek_to_image(0) .expect("Failed to seek to first image"); + assert_eq!(Some(offset0), decoder.ifd_pointer()); decoder.next_image().unwrap(); + assert_eq!(Some(offset1), decoder.ifd_pointer()); decoder.next_image().unwrap(); + assert_eq!(Some(offset2), decoder.ifd_pointer()); assert!(!decoder.more_images()); } From 97e230584f8f0655beca7cf83dc470139a4bcc85 Mon Sep 17 00:00:00 2001 From: "A. Molzer" <5550310+197g@users.noreply.github.com> Date: Thu, 10 Jul 2025 23:31:47 +0200 Subject: [PATCH 3/3] Test ordering directory IFD --- src/directory.rs | 20 ++++++++++++++++++++ tests/subsubifds.rs | 3 ++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/directory.rs b/src/directory.rs index 1cfbcc8..5cb2c52 100644 --- a/src/directory.rs +++ b/src/directory.rs @@ -158,4 +158,24 @@ mod tests { u16::MAX.into() ); } + + #[test] + fn iteration_order() { + let mut dir = Directory::empty(IfdPointer(0)); + assert_eq!(dir.len(), 0); + + let fake_entry = Entry::new_u64(crate::tags::Type::BYTE, 0, [0; 8]); + dir.extend((0..32).map(|i| { + let tag = Tag::Unknown(i); + let entry = fake_entry.clone(); + (tag, entry) + })); + + let iter_order: Vec = dir.iter().map(|(tag, _e)| tag.to_u16()).collect(); + assert_eq!( + iter_order, + (0..32).collect::>(), + "Tags must be in ascending order according to the specification" + ); + } } diff --git a/tests/subsubifds.rs b/tests/subsubifds.rs index 7dfd2ca..3fde959 100644 --- a/tests/subsubifds.rs +++ b/tests/subsubifds.rs @@ -29,7 +29,8 @@ fn decode_seek_chain() { assert!(!decoder.more_images()); // Ensure we can seek, to do this again. - decoder.seek_to_image(0) + decoder + .seek_to_image(0) .expect("Failed to seek to first image"); assert_eq!(Some(offset0), decoder.ifd_pointer()); decoder.next_image().unwrap();