From daec898d7c56fc9f283777c99e2f1ca1f52d53c2 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:49:35 -0500 Subject: [PATCH] fix(sign): judge a shared signature value once per listing; dedupe DSS evidence by object number (#6041) * fix(sign): judge a shared signature value once per listing; dedupe DSS evidence by object number * address review: shared signature values are judged by output index without copying the dictionary; a dead evidence reference does not hide a live one * address review: repeated evidence references are skipped before they are resolved * address review: each full evidence reference is tried once --- crates/sign/src/pdf/listing.rs | 16 ++++++++++++- crates/sign/src/pdf/validate.rs | 20 +++++++++++++--- crates/sign/src/pdf_discovery_tests.rs | 13 +++++++++++ crates/sign/tests/it/pdf.rs | 32 ++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 4 deletions(-) diff --git a/crates/sign/src/pdf/listing.rs b/crates/sign/src/pdf/listing.rs index c2a7183db1..2d41477127 100644 --- a/crates/sign/src/pdf/listing.rs +++ b/crates/sign/src/pdf/listing.rs @@ -431,8 +431,12 @@ pub fn list(doc: &Document, bytes: &[u8], trust: &TrustStore) -> Vec Vec { let pages = annot_pages(doc); - let mut out = Vec::new(); + let mut out: Vec = Vec::new(); let mut field_values: HashSet = HashSet::new(); + // Fields that name one signature value are judged once: validation does not depend on the + // field, and repeating it per field would repeat the digest, chain and evidence work. + // The row of `out` that holds each value's verdict: a later field copies it, not a map of copies. + let mut judged: HashMap = HashMap::new(); let (fields, whole) = walk_sig_fields(doc); for f in fields { if let Some(v_ref) = f.dict.get(b"V").and_then(Object::as_ref) { @@ -445,6 +449,13 @@ pub fn list_cached(doc: &Document, bytes: &[u8], trust: &TrustStore, cache: &Dig .filter(|r| r.len() == 4) .map(|r| [r[0].min(r[2]), r[1].min(r[3]), r[0].max(r[2]), r[1].max(r[3])]); let visible = widget.is_some_and(|(_, w)| widget_shown(doc, w)); + // A repeat of a judged value reads nothing of it: the signature dictionary is not copied. + let shared = f.dict.get(b"V").and_then(Object::as_ref).filter(|_| f.dict.get(b"V").is_some_and(|v| doc.resolve(v).as_dict().is_some())); + if let Some(first) = shared.and_then(|r| judged.get(&r)).and_then(|&i| out.get(i)) { + let own = SignatureInfo { field: f.name.clone(), signed: true, page, rect, visible, ..first.clone() }; + out.push(own); + continue; + } let v = f.signature(doc); let mut info = SignatureInfo { field: f.name.clone(), @@ -477,6 +488,9 @@ pub fn list_cached(doc: &Document, bytes: &[u8], trust: &TrustStore, cache: &Dig if let Some(v) = v { validate_into(doc, bytes, trust, &v, &mut info, cache); } + if let Some(r) = shared { + judged.insert(r, out.len()); + } out.push(info); } // Standalone document timestamps (ISO 32000-2 ยง12.8.2.2) live outside AcroForm fields. diff --git a/crates/sign/src/pdf/validate.rs b/crates/sign/src/pdf/validate.rs index 52bf79351f..d3ad411a62 100644 --- a/crates/sign/src/pdf/validate.rs +++ b/crates/sign/src/pdf/validate.rs @@ -371,7 +371,11 @@ pub(crate) const MAX_DSS_BYTES: usize = 64 << 20; pub(crate) struct DssEvidence<'a> { doc: &'a Document, items: std::vec::IntoIter, - seen: HashSet, + /// Object numbers read: `Document::get` resolves a reference by number, so `5 0 R` and `5 1 R` + /// are one stream. + seen: HashSet, + /// Full references already tried, resolved or not. + tried: HashSet, taken: usize, left: usize, /// A cap stopped the array being read in full. @@ -381,7 +385,7 @@ pub(crate) struct DssEvidence<'a> { impl<'a> DssEvidence<'a> { pub(crate) fn new(doc: &'a Document, dss: &Dict, key: &[u8]) -> Self { let items = dss.get(key).map(|o| doc.resolve(o)).and_then(|o| o.as_array().cloned()).unwrap_or_default(); - DssEvidence { doc, items: items.into_iter(), seen: HashSet::new(), taken: 0, left: MAX_DSS_BYTES, cut: false } + DssEvidence { doc, items: items.into_iter(), seen: HashSet::new(), tried: HashSet::new(), taken: 0, left: MAX_DSS_BYTES, cut: false } } } @@ -395,11 +399,21 @@ impl Iterator for DssEvidence<'_> { // Repeats are skipped before the caps count anything, so repeating one stream cannot use // them up; the scan of the array itself is bounded by the parser's cap on array length. for item in self.items.by_ref() { - if item.as_ref().is_some_and(|r| !self.seen.insert(r)) { + // Resolved first: a reference to a generation the slot doesn't hold reads as null and + // must not make a later, valid reference to the same object number a repeat. + // A repeat is skipped before anything is resolved: a long chain of repeats costs no + // lookups. The number is recorded only once the reference gave a stream. + // Each full reference is tried once, so one that never reaches a stream (a cycle) costs + // its lookups once, however often it is listed; `seen` keeps object numbers apart so a + // dead generation can't hide a live one. + if item.as_ref().is_some_and(|r| self.seen.contains(&r.num) || !self.tried.insert(r)) { continue; } let o = self.doc.resolve(&item); let Object::Stream(s) = o.as_ref() else { continue }; + if let Some(r) = item.as_ref() { + self.seen.insert(r.num); + } if self.taken >= MAX_DSS_ENTRIES { self.cut = true; return None; diff --git a/crates/sign/src/pdf_discovery_tests.rs b/crates/sign/src/pdf_discovery_tests.rs index 29f115cb07..d3250d62ed 100644 --- a/crates/sign/src/pdf_discovery_tests.rs +++ b/crates/sign/src/pdf_discovery_tests.rs @@ -373,6 +373,19 @@ fn only_grew_compares_huge_arrays_without_quadratic_work() { assert!(start.elapsed() < std::time::Duration::from_secs(10), "{:?}", start.elapsed()); } +/// A reference to a generation the object doesn't have reads as null: it is no repeat of the valid +/// reference to the same object number that follows it. +#[test] +fn dss_evidence_skips_a_dead_reference_without_hiding_the_live_one() { + let mut doc = fixture(&[]); + let live = doc.add(Object::Stream(Stream::flate(Dict::new(), b"evidence"))); + let dead = ObjRef::new(live.num, live.generation + 1); + let mut dss = Dict::new(); + dss.set(b"CRLs".to_vec(), Object::Array(vec![Object::Ref(dead), Object::Ref(live)])); + let read: Vec> = DssEvidence::new(&doc, &dss, b"CRLs").collect(); + assert_eq!(read, vec![b"evidence".to_vec()]); +} + #[test] fn dss_evidence_decodes_each_stream_once_within_its_caps() { // One small stream listed 1000 times, then streams that inflate past the evidence cap. diff --git a/crates/sign/tests/it/pdf.rs b/crates/sign/tests/it/pdf.rs index fa45ab1ecb..5d24d05dce 100644 --- a/crates/sign/tests/it/pdf.rs +++ b/crates/sign/tests/it/pdf.rs @@ -3369,6 +3369,38 @@ fn signing_a_field_with_a_null_value_after_another_signature_keeps_the_first_val ); } +/// Fields that name one signature value are listed each, under their own name, with the verdict +/// the value earned once. +#[test] +fn fields_sharing_a_signature_value_are_listed_with_one_verdict() { + let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap(); + let trust = TrustStore { certs: vec![id.certificate.clone()] }; + let signed = pdfcraft_sign::sign(&open(&fixture()), &id, &SignOptions { field: Some("Approval".into()), ..opts() }).unwrap(); + let v = open(&signed).get(pdfcraft_cos::ObjRef { num: 6, generation: 0 }).as_dict().and_then(|d| d.get(b"V").cloned()).unwrap(); + let shared = edit_after(&signed, |doc| { + let mut w = pdfcraft_cos::Dict::new(); + w.set(b"Type".to_vec(), Object::name("Annot")); + w.set(b"Subtype".to_vec(), Object::name("Widget")); + w.set(b"FT".to_vec(), Object::name("Sig")); + w.set(b"T".to_vec(), PdfString::text("Twin")); + w.set(b"V".to_vec(), v.clone()); + w.set(b"Rect".to_vec(), Object::Array(vec![Object::Int(0); 4])); + let w = doc.add(Object::Dict(w)); + let root = doc.root().unwrap(); + let af = doc.get(root).as_dict().and_then(|c| c.get(b"AcroForm").cloned()).unwrap(); + let af = doc.resolve(&af).as_dict().cloned().unwrap(); + let mut fields = af.get(b"Fields").map(|f| doc.resolve(f)).and_then(|f| f.as_array().cloned()).unwrap(); + fields.push(Object::Ref(w)); + let mut af = af; + af.set(b"Fields".to_vec(), Object::Array(fields)); + doc.update_dict(root, |c| c.set(b"AcroForm".to_vec(), Object::Dict(af))).unwrap(); + }); + let list = signatures(&open(&shared), &shared, &trust); + let (a, b) = (list.iter().find(|s| s.field == "Approval").unwrap(), list.iter().find(|s| s.field == "Twin").unwrap()); + assert!(a.signed && b.signed); + assert_eq!((a.status, &a.modification, &a.signer, a.signed_len), (b.status, &b.modification, &b.signer, b.signed_len)); +} + #[test] fn the_catalogs_perms_do_not_change_under_no_changes() { // /Perms names the certification signature (DocMDP). Under P=1 nothing may change, so a later