fix(sign): a signed revision of a password-protected file opens with the document's key (#1808) (#1945)

To classify what later revisions changed, the signed bytes were reopened
with Document::open, without a password, so for a file opened with a
nonempty user password the earlier revision was unreadable and the earlier
signature became Invalid as soon as it was counter-signed. Document::
open_with_same_credentials opens such bytes with the current document's own
security handler when their /Encrypt and /ID are the same (no password is
needed again or kept), and the signature validator uses it.


Claude-Session: https://claude.ai/code/session_01RUNQSPVXx3dDBabRZHAnjz

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Brandon Thomas <echelon@gmail.com>

crates/cos/src/document.rs

@@ -441,6 +441,13 @@ impl std::fmt::Debug for Document {
     }
 }
 
+/// How an encrypted document is opened: with a password, or with the file key of a document
+/// that has the same encryption (an earlier revision of the same file).
+enum Credentials<'a> {
+    Password(Option<&'a str>),
+    Handler { handler: &'a Arc<pdfcraft_crypt::SecurityHandler>, id0: Vec<u8> },
+}
+
 impl Document {
     /// A new document with an empty page tree (for split / extract / combine). It has no
     /// original bytes, so saving always writes a full file.
@@ -497,6 +504,23 @@ impl Document {
     /// document is read-only: objects in a larger object stream read as missing, so the
     /// writers refuse to save it ([`CosError::ReadOnlyLimit`]).
     pub fn open_with_stream_limit(data: Arc<Vec<u8>>, password: Option<&str>, stream_limit: usize) -> Result<Self, CosError> {
+        Self::open_as(data, Credentials::Password(password), stream_limit)
+    }
+
+    /// Open `data`, an earlier revision of this file (the bytes a signature covers) or another
+    /// save of it, with this document's credentials: an encrypted one must have the same
+    /// `/Encrypt` dictionary and `/ID` as this document, whose file key is then reused, so no
+    /// password is needed or kept; unencrypted data opens as [`open`](Self::open) does. Data
+    /// encrypted differently fails with `NeedsPassword`.
+    pub fn open_with_same_credentials(&self, data: Arc<Vec<u8>>) -> Result<Self, CosError> {
+        let credentials = match &self.security {
+            Some(handler) => Credentials::Handler { handler, id0: self.first_id() },
+            None => Credentials::Password(None),
+        };
+        Self::open_as(data, credentials, self.stream_limit)
+    }
+
+    fn open_as(data: Arc<Vec<u8>>, credentials: Credentials<'_>, stream_limit: usize) -> Result<Self, CosError> {
         // Viewers accept files whose header is missing or damaged as long as the body looks
         // like PDF; so do we (a note goes to the repair log).
         let header = find(&data, b"%PDF-", 0, 1024);
@@ -546,7 +570,7 @@ impl Document {
                 // Authenticate before checking the catalog: it may sit in an encrypted object
                 // stream, which reads as garbage until the key is known.
                 if let Some(enc) = doc.trailer.get(b"Encrypt").cloned() {
-                    doc.authenticate(&enc, password)?;
+                    doc.authenticate(&enc, &credentials)?;
                     authenticated = true;
                 }
                 doc.root_is_catalog()
@@ -567,7 +591,7 @@ impl Document {
         if (!ok || !authenticated)
             && let Some(enc) = doc.trailer.get(b"Encrypt").cloned()
         {
-            doc.authenticate(&enc, password)?;
+            doc.authenticate(&enc, &credentials)?;
         }
         let max = doc.entries.max_num();
         let size = doc.trailer.int(b"Size").unwrap_or(0).max(0) as u32;
@@ -575,7 +599,7 @@ impl Document {
         Ok(doc)
     }
 
-    fn authenticate(&mut self, enc: &Object, password: Option<&str>) -> Result<(), CosError> {
+    fn authenticate(&mut self, enc: &Object, credentials: &Credentials<'_>) -> Result<(), CosError> {
         // Authentication can change how the same input is decoded.
         self.source_identity = Arc::new(());
         let (num, dict) = match enc {
@@ -593,12 +617,17 @@ impl Document {
         };
         let params = crate::security::encrypt_dict(self, &dict);
         let id0 = self.first_id();
-        let handler = pdfcraft_crypt::SecurityHandler::open(params, &id0, password).map_err(|e| match e {
-            pdfcraft_crypt::CryptError::WrongPassword if password.is_none() => CosError::NeedsPassword,
-            pdfcraft_crypt::CryptError::WrongPassword => CosError::WrongPassword,
-            other => CosError::Security(other.to_string()),
-        })?;
-        self.security = Some(Arc::new(handler));
+        let handler = match credentials {
+            Credentials::Password(password) => Arc::new(pdfcraft_crypt::SecurityHandler::open(params, &id0, *password).map_err(|e| match e {
+                pdfcraft_crypt::CryptError::WrongPassword if password.is_none() => CosError::NeedsPassword,
+                pdfcraft_crypt::CryptError::WrongPassword => CosError::WrongPassword,
+                other => CosError::Security(other.to_string()),
+            })?),
+            // The same encryption as the document whose key this is: that key opens it.
+            Credentials::Handler { handler, id0: known } if *handler.dict() == params && *known == id0 => Arc::clone(handler),
+            Credentials::Handler { .. } => return Err(CosError::NeedsPassword),
+        };
+        self.security = Some(handler);
         self.encrypt_num = num;
         // Objects read while locating the catalog were read before decryption was possible.
         self.cache = Arc::default();

crates/cos/tests/encryption.rs

@@ -301,3 +301,38 @@ fn a_nonpositive_crypt_filter_length_reads_as_the_default_key_length() {
         assert_eq!(content(&again), SECRET, "/Length {length}");
     }
 }
+
+#[test]
+fn an_earlier_revision_opens_with_the_documents_own_credentials() {
+    // The bytes a signature covers are an earlier revision of the file: the document that is
+    // open can read them with its own key, so no password has to be kept or asked for again.
+    for alg in ALL {
+        let original = protect(alg, "user", "owner", -1);
+        let mut doc = Document::open_with_password(Arc::new(original.clone()), Some("user")).unwrap();
+        let info = doc.trailer().reference(b"Info").unwrap();
+        doc.update_dict(info, |d| d.set(b"Title".to_vec(), Object::String(PdfString::text("Revised")))).unwrap();
+        let saved = write_incremental(&doc, &SaveOptions::default()).unwrap();
+        let current = Document::open_with_password(Arc::new(saved), Some("user")).unwrap();
+        assert!(matches!(Document::open(Arc::new(original.clone())), Err(CosError::NeedsPassword)), "{alg:?}: not without a password");
+        let earlier = current.open_with_same_credentials(Arc::new(original.clone())).unwrap();
+        assert_eq!(content(&earlier), SECRET, "{alg:?}: the earlier revision is readable");
+        assert_eq!(title(&earlier).as_deref(), Some("Quarterly numbers"), "{alg:?}: and it is the earlier one");
+        // Another file, encrypted with other keys: the document's key is no use, and it says so.
+        let other = {
+            let mut doc = Document::open(Arc::new(fixture())).unwrap();
+            doc.set_encryption(&NewEncryption {
+                algorithm: alg,
+                user_password: "user",
+                owner_password: "owner",
+                permissions: -1,
+                encrypt_metadata: true,
+                seed: [3; 32],
+            })
+            .unwrap();
+            write_full(&doc, &SaveOptions::default()).unwrap()
+        };
+        assert!(matches!(current.open_with_same_credentials(Arc::new(other)), Err(CosError::NeedsPassword)), "{alg:?}");
+        // An unencrypted file opens as it always does.
+        assert_eq!(content(&current.open_with_same_credentials(Arc::new(fixture())).unwrap()), SECRET, "{alg:?}");
+    }
+}

crates/sign/src/pdf.rs

@@ -156,13 +156,15 @@ impl DigestCache {
         candidates
     }
 
-    /// The signed revision `bytes[..end]`, parsed (and kept).
-    fn revision(&self, bytes: &[u8], end: usize) -> Option<Document> {
+    /// The signed revision `bytes[..end]`, parsed (and kept). An encrypted one is opened with
+    /// `doc`'s own key (the same `/Encrypt` and `/ID`), so a user password the document was
+    /// opened with is neither needed again nor kept here (#1808).
+    fn revision(&self, doc: &Document, bytes: &[u8], end: usize) -> Option<Document> {
         let key = (end, fingerprint(&bytes[..end]));
         if let Some(d) = self.revisions.lock().ok().and_then(|m| m.get(&key).cloned()) {
             return Some(d);
         }
-        let d = Document::open(Arc::new(bytes[..end].to_vec())).ok()?;
+        let d = doc.open_with_same_credentials(Arc::new(bytes[..end].to_vec())).ok()?;
         if let Ok(mut m) = self.revisions.lock() {
             if m.len() > 8 {
                 m.clear();
@@ -726,7 +728,7 @@ fn validate_into(doc: &Document, bytes: &[u8], trust: &TrustStore, v: &Dict, inf
     let covered = o1 + l1;
     info.signed_len = covered;
     // The signed revision's number: its cross-reference sections (1 for a reconstructed file).
-    info.revision = cache.revision(bytes, covered).map_or(1, |d| d.revisions().len().max(1));
+    info.revision = cache.revision(doc, bytes, covered).map_or(1, |d| d.revisions().len().max(1));
     // What can't be checked is unknown, not invalid: only data that is wrong or fails its check
     // is "invalid".
     let unsupported = |info: &mut SignatureInfo, why: &str| {
@@ -853,7 +855,7 @@ fn finish_validation(
     info.modification = if covered == bytes.len() || bytes[covered..].iter().all(|b| b.is_ascii_whitespace() || *b == 0) {
         Modification::None
     } else {
-        classify_changes(doc, cache.revision(bytes, covered), info.certify)
+        classify_changes(doc, cache.revision(doc, bytes, covered), info.certify)
     };
     let mut problems = false;
     // The changes are handed over as the words they are, not stitched into the sentence, so an
@@ -1015,7 +1017,7 @@ fn validate_doc_timestamp(doc: &Document, bytes: &[u8], v: &Dict, info: &mut Sig
     };
     let covered = o1 + l1;
     info.signed_len = covered;
-    info.revision = cache.revision(bytes, covered).map_or(1, |d| d.revisions().len().max(1));
+    info.revision = cache.revision(doc, bytes, covered).map_or(1, |d| d.revisions().len().max(1));
     let token = match crate::timestamp::parse_token(&contents) {
         Ok(t) => t,
         Err(e) => return invalid(info, Detail::of("The timestamp token could not be read ({}).", Fill::Reason(e.to_string()))),
@@ -1038,7 +1040,7 @@ fn validate_doc_timestamp(doc: &Document, bytes: &[u8], v: &Dict, info: &mut Sig
     info.modification = if covered == bytes.len() || bytes[covered..].iter().all(|b| b.is_ascii_whitespace() || *b == 0) {
         Modification::None
     } else {
-        classify_changes(doc, cache.revision(bytes, covered), None)
+        classify_changes(doc, cache.revision(doc, bytes, covered), None)
     };
     match &info.modification {
         Modification::None => info.details.push(Detail::plain("This document has not been modified since this timestamp was applied.")),

crates/sign/tests/pdf.rs

@@ -1638,10 +1638,15 @@ fn validates_the_legacy_adbe_x509_rsa_sha1_format() {
 /// `fixture()` protected with an empty user password (opens without asking, like most
 /// "secured" forms) and an owner password, then saved.
 fn protected(alg: pdfcraft_cos::Algorithm, permissions: i32) -> Vec<u8> {
+    protected_with(alg, "", permissions)
+}
+
+/// `fixture()` protected with the user password `user` and an owner password, then saved.
+fn protected_with(alg: pdfcraft_cos::Algorithm, user: &str, permissions: i32) -> Vec<u8> {
     let mut doc = open(&fixture());
     doc.set_encryption(&pdfcraft_cos::NewEncryption {
         algorithm: alg,
-        user_password: "",
+        user_password: user,
         owner_password: "owner",
         permissions,
         encrypt_metadata: true,
@@ -1651,6 +1656,38 @@ fn protected(alg: pdfcraft_cos::Algorithm, permissions: i32) -> Vec<u8> {
     pdfcraft_cos::write_full(&doc, &SaveOptions::default()).unwrap()
 }
 
+/// #1808: a document that asks for its user password, signed and then counter-signed. The
+/// first signature no longer covers the whole file, so its revision is reopened to classify the
+/// change; that revision is encrypted too, and used to be unreadable without the password,
+/// which made the first signature Invalid. It opens with the document's own key.
+#[test]
+fn a_user_password_document_keeps_its_earlier_signature_valid_when_counter_signed() {
+    let id = pkcs12::open(&data("ec-p256.p12"), "test").unwrap();
+    let id2 = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap();
+    let trust = TrustStore { certs: vec![id.certificate.clone(), id2.certificate.clone()] };
+    for alg in ALL_ALGORITHMS {
+        let base = protected_with(alg, "secret", -1);
+        assert!(Document::open_with_password(Arc::new(base.clone()), None).is_err(), "{alg:?}: the file asks for its password");
+        let signed = pdfcraft_sign::sign(&open_pw(&base, Some("secret")), &id, &opts()).unwrap();
+        let both = pdfcraft_sign::sign(&open_pw(&signed, Some("secret")), &id2, &SignOptions { field: Some("Approval".into()), ..opts() }).unwrap();
+        let sigs = signatures(&open_pw(&both, Some("secret")), &both, &trust);
+        let first = sigs.iter().find(|s| s.field == "Signature1").unwrap();
+        assert_eq!(first.modification, Modification::Allowed(vec![kind::SIGNATURE]), "{alg:?}: {:?}", first.details);
+        assert_eq!(first.status, Status::Valid, "{alg:?}: {:?}", first.details);
+        let second = sigs.iter().find(|s| s.field == "Approval").unwrap();
+        assert_eq!((second.status, second.revision), (Status::Valid, 3), "{alg:?}: {:?}", second.details);
+        // Opened as owner, the same.
+        let sigs = signatures(&open_pw(&both, Some("owner")), &both, &trust);
+        assert!(sigs.iter().filter(|s| s.signed).all(|s| s.status == Status::Valid), "{alg:?}");
+        // Tampering inside the first signature's range still breaks both.
+        let mut tampered = both.clone();
+        let at = base.len() / 2;
+        tampered[at] ^= 1;
+        let tampered_sigs = signatures(&open_pw(&tampered, Some("secret")), &tampered, &trust);
+        assert!(tampered_sigs.iter().filter(|s| s.signed).all(|s| s.status == Status::Invalid), "{alg:?}");
+    }
+}
+
 fn open_pw(b: &[u8], pw: Option<&str>) -> Document {
     Document::open_with_password(Arc::new(b.to_vec()), pw).unwrap()
 }