From 2c4645de414e0ac07cb27bf0d6ed11812765a8c0 Mon Sep 17 00:00:00 2001 From: Tiago Kiel <186439951+TiagoCK@users.noreply.github.com> Date: Tue, 13 Oct 2026 08:52:03 -0400 Subject: [PATCH] fix(cos): check object-stream member numbers and offsets (#1815) Object-stream members were indexed with unchecked casts: `/First` and each offset went through `as usize` and an unchecked addition, which truncate or wrap on 32-bit targets such as the wasm32 build, and the member number went through `as u32` on every target, so a member numbered 2^32 + 3 was read as object 3. A negative offset was clamped to zero and read the stream's first member instead. Each member is now converted and added with checks, and kept as an unusable slot (preserving positions, which cross-reference entries refer to) when its number is not an object number or its start falls outside the decoded data. Looking such a member up is a MissingObject error, as for any other absent object. Co-Authored-By: Claude Opus 5.5 --- crates/cos/src/document.rs | 113 +++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 29 deletions(-) diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index a0b1f56ee..97c832eb2 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -311,11 +311,22 @@ pub struct Revision { #[derive(Clone, Debug)] struct ObjStm { - /// (object number, byte offset within `data`). - index: Vec<(u32, usize)>, + /// One slot per member in stream order, since cross-reference entries name a member by its + /// position: (object number, byte offset within `data`), or `None` for a member whose number + /// or offset is unusable ([`objstm_member`]). + index: Vec>, data: Arc>, } +/// Where member `num` of an object stream starts: its `offset` past `/First` (`first`), when the +/// number is an object number and the start lies inside the decoded `len` bytes. Checked on every +/// target, so a 32-bit build neither truncates nor wraps a large `/First` or offset (#1815). +fn objstm_member(num: i64, offset: i64, first: Option, len: usize) -> Option<(u32, usize)> { + let num = u32::try_from(num).ok()?; + let start = first?.checked_add(usize::try_from(offset).ok()?)?; + (start < len).then_some((num, start)) +} + /// Where an overlay slot came from. #[derive(Clone, Debug, PartialEq)] enum Slot { @@ -395,7 +406,7 @@ impl ObjectReader { let bytes = streams.values().fold(0usize, |bytes, stream| { bytes .saturating_add(stream.data.capacity()) - .saturating_add(stream.index.capacity().saturating_mul(std::mem::size_of::<(u32, usize)>())) + .saturating_add(stream.index.capacity().saturating_mul(std::mem::size_of::>())) }); if bytes > Self::STREAM_BYTES || streams.len() >= Self::OBJECTS { // Retain one oversized active stream so its next object does not require @@ -890,9 +901,9 @@ impl Document { // Offsets may count from a header that does not start the file. usize::try_from(offset).is_ok_and(|off| at(off) || (self.header_offset != 0 && at(off.saturating_add(self.header_offset)))) } - Some(XrefEntry::InStream { stream, index }) => { - self.objstm(stream).is_ok_and(|stm| usize::try_from(index).ok().and_then(|i| stm.index.get(i)).is_some_and(|(n, _)| *n == num)) - } + Some(XrefEntry::InStream { stream, index }) => self + .objstm(stream) + .is_ok_and(|stm| usize::try_from(index).ok().and_then(|i| stm.index.get(i)).is_some_and(|m| m.is_some_and(|(n, _)| n == num))), } } @@ -1005,7 +1016,7 @@ impl Document { let bytes = streams.values().fold(0usize, |bytes, stream| { bytes .saturating_add(stream.data.capacity()) - .saturating_add(stream.index.capacity().saturating_mul(std::mem::size_of::<(u32, usize)>())) + .saturating_add(stream.index.capacity().saturating_mul(std::mem::size_of::>())) }); bytes > Self::EDITOR_OBJSTM_BYTES || streams.len() > Self::EDITOR_OBJSTM_OBJECTS }; @@ -1096,14 +1107,9 @@ impl Document { } Some(XrefEntry::InStream { stream, index }) => { let stm = self.objstm(stream)?; - let (n, off) = stm - .index - .get(index as usize) - .copied() - .or_else(|| stm.index.iter().find(|(n, _)| *n == num).copied()) - .ok_or(CosError::MissingObject(num))?; - let off = - if n == num { off } else { stm.index.iter().find(|(n, _)| *n == num).map(|(_, o)| *o).ok_or(CosError::MissingObject(num))? }; + // The member at the entry's position, else wherever the stream lists `num`. + let at = usize::try_from(index).ok().and_then(|i| stm.index.get(i)).copied().flatten().filter(|(n, _)| *n == num); + let (_, off) = at.or_else(|| stm.index.iter().flatten().find(|(n, _)| *n == num).copied()).ok_or(CosError::MissingObject(num))?; Lexer::new(&stm.data, off).object() } } @@ -1115,13 +1121,13 @@ impl Document { } let Object::Stream(s) = &*self.try_get(num)? else { return Err(CosError::MissingObject(num)) }; let n = s.dict.int(b"N").unwrap_or(0).clamp(0, 1_000_000) as usize; - let first = s.dict.int(b"First").unwrap_or(0).max(0) as usize; + let first = usize::try_from(s.dict.int(b"First").unwrap_or(0).max(0)).ok(); let data = s.decoded_within(self.stream_limit)?; let mut lx = Lexer::new(&data, 0); let mut index = Vec::with_capacity(n); for _ in 0..n { let (Some(Object::Int(onum)), Some(Object::Int(off))) = (lx.object().ok(), lx.object().ok()) else { break }; - index.push((onum.max(0) as u32, first + off.max(0) as usize)); + index.push(objstm_member(onum, off, first, data.len())); } let stm = Arc::new(ObjStm { index, data: Arc::new(data) }); // Same bound as the object cache: decoded object streams can be megabytes each. @@ -1848,8 +1854,10 @@ impl Document { && s.dict.name(b"Type") == Some(b"ObjStm") && let Ok(stm) = self.objstm(num) { - for (idx, (inner, _)) in stm.index.iter().enumerate() { - entries.entry(*inner).or_insert(XrefEntry::InStream { stream: num, index: idx as u32 }); + for (idx, member) in stm.index.iter().enumerate() { + if let Some((inner, _)) = member { + entries.entry(*inner).or_insert(XrefEntry::InStream { stream: num, index: idx as u32 }); + } } } } @@ -2362,7 +2370,7 @@ mod tests { assert!(reader.document.cache.lock().unwrap().len() < ObjectReader::OBJECTS); let decoded = reader.document.objstms.lock().unwrap(); assert!(decoded.len() < ObjectReader::OBJECTS); - let bytes: usize = decoded.values().map(|s| s.data.capacity() + s.index.capacity() * std::mem::size_of::<(u32, usize)>()).sum(); + let bytes: usize = decoded.values().map(|s| s.data.capacity() + s.index.capacity() * std::mem::size_of::>()).sum(); assert!(bytes <= ObjectReader::STREAM_BYTES || decoded.len() == 1, "only one oversized stream may remain"); } drop(reader); @@ -2483,6 +2491,16 @@ mod tests { /// says it lives in object stream 4. #[test] fn hybrid_reference_files_read_the_xref_stream_too() { + let doc = Document::open(Arc::new(object_stream_file("3 0 ", "<< /Hidden (yes) >>", 0))).unwrap(); + assert!(doc.repair_log().is_empty(), "{:?}", doc.repair_log()); + assert_compact_matches_materialized(&doc); + assert_eq!(text_of(&doc, 3, b"Hidden").as_deref(), Some("yes")); + } + + /// A hybrid-reference file whose classic table leaves object 3 out, and whose `/XRefStm` + /// stream puts it at position `member` of object stream 4, which holds `header` (the number + /// and offset pairs) and then `members`. + fn object_stream_file(header: &str, members: &str, member: u8) -> Vec { let mut out = b"%PDF-1.5\n".to_vec(); let mut offs = [0usize; 6]; let mut obj = |out: &mut Vec, n: usize, body: &[u8]| { @@ -2493,12 +2511,13 @@ mod tests { }; obj(&mut out, 1, b"<< /Type /Catalog /Pages 2 0 R >>"); obj(&mut out, 2, b"<< /Type /Pages /Kids [] /Count 0 /Extra 3 0 R >>"); - let inner = b"3 0 << /Hidden (yes) >>"; - let mut stm = format!("<< /Type /ObjStm /N 1 /First 4 /Length {} >>\nstream\n", inner.len()).into_bytes(); - stm.extend_from_slice(inner); + let count = header.split_whitespace().count() / 2; + let inner = format!("{header}{members}"); + let mut stm = format!("<< /Type /ObjStm /N {count} /First {} /Length {} >>\nstream\n", header.len(), inner.len()).into_bytes(); + stm.extend_from_slice(inner.as_bytes()); stm.extend_from_slice(b"\nendstream"); obj(&mut out, 4, &stm); - let data = [2u8, 0, 4, 0]; + let data = [2u8, 0, 4, member]; let mut xs = format!("<< /Type /XRef /Size 6 /W [1 2 1] /Index [3 1] /Length {} >>\nstream\n", data.len()).into_bytes(); xs.extend_from_slice(&data); xs.extend_from_slice(b"\nendstream"); @@ -2510,11 +2529,47 @@ mod tests { out.extend_from_slice(line.as_bytes()); } out.extend_from_slice(format!("trailer << /Size 6 /Root 1 0 R /XRefStm {} >>\nstartxref\n{table}\n%%EOF\n", offs[5]).as_bytes()); - let doc = Document::open(Arc::new(out)).unwrap(); - assert!(doc.repair_log().is_empty(), "{:?}", doc.repair_log()); - assert_compact_matches_materialized(&doc); - let hidden = doc.get(ObjRef::new(3, 0)); - assert_eq!(hidden.as_dict().and_then(|d| d.get(b"Hidden").and_then(|h| h.as_string().map(|s| s.to_text()))), Some("yes".into())); + out + } + + fn text_of(doc: &Document, num: u32, key: &[u8]) -> Option { + doc.get(ObjRef::new(num, 0)).as_dict().and_then(|d| d.get(key).and_then(|v| v.as_string().map(|s| s.to_text()))) + } + + /// An object stream member numbered 2^32 + 3 is not object 3 (#1815): the number used to be + /// cast to `u32`, which dropped the high bits. + #[test] + fn object_stream_numbers_beyond_u32_are_not_truncated() { + let control = Document::open(Arc::new(object_stream_file("3 0 ", "<< /Member (yes) >>", 0))).unwrap(); + assert_eq!(text_of(&control, 3, b"Member").as_deref(), Some("yes")); + let doc = Document::open(Arc::new(object_stream_file("4294967299 0 ", "<< /Member (yes) >>", 0))).unwrap(); + assert!(matches!(*doc.get(ObjRef::new(3, 0)), Object::Null), "{:?}", doc.get(ObjRef::new(3, 0))); + assert!(!doc.xref_entry_is_exact(3)); + } + + /// Member starts are checked rather than cast, so they cannot wrap on any target (#1815). + #[test] + fn object_stream_member_starts_are_checked() { + assert_eq!(objstm_member(3, 4, Some(10), 20), Some((3, 14))); + assert_eq!(objstm_member(3, 9, Some(10), 20), Some((3, 19)), "the last byte"); + assert_eq!(objstm_member(3, 10, Some(10), 20), None, "at the end of the data"); + assert_eq!(objstm_member(3, 1, Some(usize::MAX), 20), None, "/First plus the offset overflows"); + assert_eq!(objstm_member(3, i64::MAX, Some(10), 20), None); + assert_eq!(objstm_member(3, -1, Some(10), 20), None); + assert_eq!(objstm_member(3, 0, None, 20), None, "/First does not fit in usize"); + assert_eq!(objstm_member(i64::from(u32::MAX), 0, Some(0), 1), Some((u32::MAX, 0))); + assert_eq!(objstm_member(i64::from(u32::MAX) + 1, 0, Some(0), 1), None); + assert_eq!(objstm_member(-1, 0, Some(0), 1), None); + } + + /// A member at a negative offset is unusable, not the stream's first member (#1815): the + /// offset used to be clamped to zero, which read the object at `/First`. + #[test] + fn object_stream_negative_offsets_are_refused() { + let doc = Document::open(Arc::new(object_stream_file("7 0 3 -1 ", "<< /First (yes) >>", 1))).unwrap(); + assert!(matches!(*doc.get(ObjRef::new(3, 0)), Object::Null), "{:?}", doc.get(ObjRef::new(3, 0))); + let control = Document::open(Arc::new(object_stream_file("7 0 3 19 ", "<< /First (yes) >> << /Second (yes) >>", 1))).unwrap(); + assert_eq!(text_of(&control, 3, b"Second").as_deref(), Some("yes")); } #[test]