From 5856c12eb3748ff1e1be936695aa8014896a08c2 Mon Sep 17 00:00:00 2001 From: ffishman <20211677+ffishman@users.noreply.github.com> Date: Thu, 22 Oct 2026 14:55:44 -0300 Subject: [PATCH 1/2] fix(sign): judge a page's indirect /Annots array by the annotations it gains A page whose /Annots is an object of its own keeps its dictionary when a later revision adds a signature widget or a comment: only the array changes. That array fell through to "other changes", so countersigning such a document (as many signing services do) turned every earlier signature invalid, including PdfCraft's own second signature. A changed array that was a signed page's /Annots is now classified like an inline /Annots: by the annotations it gained (signature, form fill or comments), under the same DocMDP rules. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/sign/src/pdf.rs | 12 +++++++++++ crates/sign/tests/it/pdf.rs | 41 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/crates/sign/src/pdf.rs b/crates/sign/src/pdf.rs index 82a801dee..cd555a989 100644 --- a/crates/sign/src/pdf.rs +++ b/crates/sign/src/pdf.rs @@ -1265,6 +1265,11 @@ fn classify_changes(doc: &Document, old: Option, p: Option) -> Mod // attacker-written version carries (relabelling a signed page /Type /Sig, or dropping its // /Type and adding /ModDate, must not reclassify it away from page content). let signed_pages: HashSet = pdfcraft_annot::page_refs(&old).unwrap_or_default().into_iter().collect(); + // The signed pages' /Annots arrays that are objects of their own. Adding an annotation updates + // such an array in place (the page dictionary does not change), so it is judged like a + // page's inline /Annots: by the annotations it gained. + let signed_annots: HashSet = + signed_pages.iter().filter_map(|pg| old.get(*pg).as_dict().and_then(|d| d.get(b"Annots").and_then(Object::as_ref))).collect(); // What each signed page draws from, by the role it held when signed (#1787). let signed_resources = signed_page_resources(&old, &signed_pages); let old_info = old.trailer().get(b"Info").and_then(Object::as_ref); @@ -1333,6 +1338,13 @@ fn classify_changes(doc: &Document, old: Option, p: Option) -> Mod (Some(k), _) => k, // The Document Security Store's own arrays and dictionaries, when they only grew. (_, _) if dss.contains(&num) => kind::SECURITY_STORE, + // A signed page's indirect /Annots array, still an array: what it gained. + (_, Object::Array(now)) if signed_annots.contains(&r) && before.as_deref().is_some_and(|b| b.as_array().is_some()) => { + let was: HashSet = + before.as_deref().and_then(Object::as_array).map(|a| a.iter().filter_map(Object::as_ref).collect()).unwrap_or_default(); + let added: Vec = now.iter().filter_map(Object::as_ref).filter(|r| !was.contains(r)).collect(); + added_annots(doc, &added) + } (_, Object::Dict(d)) => { let was = before.as_deref().and_then(Object::as_dict); if signed_pages.contains(&r) { diff --git a/crates/sign/tests/it/pdf.rs b/crates/sign/tests/it/pdf.rs index 61517b53d..cd0b713a8 100644 --- a/crates/sign/tests/it/pdf.rs +++ b/crates/sign/tests/it/pdf.rs @@ -139,6 +139,47 @@ fn signing_an_existing_field_and_counter_signing() { )); } +/// A page whose /Annots is an object of its own: signing and commenting update that array in +/// place, not the page. A second signature is a signature change for the first, as with an +/// inline /Annots, and a comment is still a comment (refused under a form-fill certification). +#[test] +fn an_indirect_annots_array_is_judged_by_the_annotations_it_gains() { + let page = pdfcraft_cos::ObjRef { num: 3, generation: 0 }; + let unsigned = edit_after(&fixture(), |doc| { + let annots = doc.add(Object::Array(vec![Object::Ref(pdfcraft_cos::ObjRef { num: 6, generation: 0 })])); + doc.update_dict(page, |p| p.set(b"Annots".to_vec(), Object::Ref(annots))).unwrap(); + }); + let annots_ref = |bytes: &[u8]| open(bytes).get(page).as_dict().and_then(|d| d.get(b"Annots").and_then(Object::as_ref)); + let annots = annots_ref(&unsigned).unwrap(); + let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap(); + let trust = TrustStore { certs: vec![id.certificate.clone()] }; + let first = pdfcraft_sign::sign(&open(&unsigned), &id, &opts()).unwrap(); + let second = pdfcraft_sign::sign(&open(&first), &id, &SignOptions { rect: Some([20.0, 160.0, 220.0, 210.0]), ..opts() }).unwrap(); + assert_eq!(annots_ref(&second), Some(annots), "the signer kept the page's array indirect"); + let doc = open(&second); + let sigs = signatures(&doc, &second, &trust); + let s = sigs.iter().find(|s| s.field == "Signature1").unwrap(); + assert_eq!(s.modification, Modification::Allowed(vec![kind::SIGNATURE]), "{:?}", s.details); + assert_eq!(s.status, Status::Valid); + // A comment appended to the indirect array. + let comment = |doc: &mut Document| { + let mut d = pdfcraft_cos::Dict::new(); + d.set(b"Type".to_vec(), Object::name("Annot")); + d.set(b"Subtype".to_vec(), Object::name("Text")); + d.set(b"Rect".to_vec(), Object::Array(vec![Object::Int(10), Object::Int(10), Object::Int(30), Object::Int(30)])); + let r = doc.add(Object::Dict(d)); + let mut a = doc.get(annots).as_array().cloned().unwrap(); + a.push(Object::Ref(r)); + doc.set(annots, Object::Array(a)); + }; + let check = |bytes: &[u8]| signatures(&open(bytes), bytes, &trust).into_iter().find(|s| s.signed).unwrap(); + let s = check(&edit_after(&first, comment)); + assert_eq!((s.status, s.modification), (Status::Valid, Modification::Allowed(vec![kind::COMMENTS]))); + let certified = pdfcraft_sign::sign(&open(&unsigned), &id, &SignOptions { certify: Some(2), ..opts() }).unwrap(); + let s = check(&edit_after(&certified, comment)); + assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::COMMENTS]))); +} + /// Issue #1328: a signature field whose /V is null, directly or through a reference, is unsigned: /// it is listed as unsigned and can be signed by name. #[test] From c829a1ad44cb4128d68b71895238ae1e8941219d Mon Sep 17 00:00:00 2001 From: ffishman <20211677+ffishman@users.noreply.github.com> Date: Thu, 22 Oct 2026 17:03:37 -0300 Subject: [PATCH 2/2] fix(sign): a removed annotation is not hidden behind an added signature Review follow-up: an indirect /Annots array that drops a signed annotation while gaining a signature widget was judged by the widget alone, so a form-fill certification let a signed comment be deleted. A removed comment is now a comment change, and a removed widget is a change no permission covers. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/sign/src/pdf.rs | 17 ++++++++++++++--- crates/sign/tests/it/pdf.rs | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/crates/sign/src/pdf.rs b/crates/sign/src/pdf.rs index cd555a989..0cd58d7f9 100644 --- a/crates/sign/src/pdf.rs +++ b/crates/sign/src/pdf.rs @@ -1338,12 +1338,23 @@ fn classify_changes(doc: &Document, old: Option, p: Option) -> Mod (Some(k), _) => k, // The Document Security Store's own arrays and dictionaries, when they only grew. (_, _) if dss.contains(&num) => kind::SECURITY_STORE, - // A signed page's indirect /Annots array, still an array: what it gained. + // A signed page's indirect /Annots array, still an array: what it lost, then what it + // gained. Dropping a signed annotation is never hidden behind an added signature. (_, Object::Array(now)) if signed_annots.contains(&r) && before.as_deref().is_some_and(|b| b.as_array().is_some()) => { let was: HashSet = before.as_deref().and_then(Object::as_array).map(|a| a.iter().filter_map(Object::as_ref).collect()).unwrap_or_default(); - let added: Vec = now.iter().filter_map(Object::as_ref).filter(|r| !was.contains(r)).collect(); - added_annots(doc, &added) + let kept: HashSet = now.iter().filter_map(Object::as_ref).collect(); + let removed: Vec = was.iter().filter(|r| !kept.contains(r)).copied().collect(); + if removed.is_empty() { + let added: Vec = now.iter().filter_map(Object::as_ref).filter(|r| !was.contains(r)).collect(); + added_annots(doc, &added) + } else if removed.iter().any(|r| old.get(*r).as_dict().is_some_and(|d| d.name(b"Subtype") == Some(b"Widget"))) { + // A form field or signature widget taken off the page: no permission covers it. + kind::OTHER + } else { + // Deleting comments is a comment change (permitted at P 3 only). + kind::COMMENTS + } } (_, Object::Dict(d)) => { let was = before.as_deref().and_then(Object::as_dict); diff --git a/crates/sign/tests/it/pdf.rs b/crates/sign/tests/it/pdf.rs index cd0b713a8..b3a025fc2 100644 --- a/crates/sign/tests/it/pdf.rs +++ b/crates/sign/tests/it/pdf.rs @@ -178,6 +178,30 @@ fn an_indirect_annots_array_is_judged_by_the_annotations_it_gains() { let certified = pdfcraft_sign::sign(&open(&unsigned), &id, &SignOptions { certify: Some(2), ..opts() }).unwrap(); let s = check(&edit_after(&certified, comment)); assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::COMMENTS]))); + // Under that certification, a signed comment swapped for a new signature widget is still a + // deleted comment, and a form field taken off the page is permitted by no level. + let commented = pdfcraft_sign::sign(&open(&edit_after(&unsigned, comment)), &id, &SignOptions { certify: Some(2), ..opts() }).unwrap(); + let swap = |drop: &dyn Fn(&Document, &Object) -> bool| { + edit_after(&commented, |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("Later")); + w.set(b"Rect".to_vec(), Object::Array(vec![Object::Int(0); 4])); + let w = doc.add(Object::Dict(w)); + let mut a = doc.get(annots).as_array().cloned().unwrap(); + a.retain(|o| !drop(doc, o)); + a.push(Object::Ref(w)); + doc.set(annots, Object::Array(a)); + }) + }; + let subtype = + |doc: &Document, o: &Object, t: &[u8]| o.as_ref().is_some_and(|r| doc.get(r).as_dict().is_some_and(|d| d.name(b"Subtype") == Some(t))); + let s = check(&swap(&|doc, o| subtype(doc, o, b"Text"))); + assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::COMMENTS]))); + let s = check(&swap(&|doc, o| subtype(doc, o, b"Widget") && o.as_ref().is_some_and(|r| r.num == 6))); + assert_eq!((s.status, s.modification), (Status::Invalid, Modification::Disallowed(vec![kind::OTHER]))); } /// Issue #1328: a signature field whose /V is null, directly or through a reference, is unsigned: