fix(sign): signed attributes must carry the message digest and the data content type

With signed attributes, the signature value is made over the attributes,
and their messageDigest is what ties it to the document. The digest was
compared only when the attribute was present, and the content-type
attribute was never checked. RFC 5652 section 5.3 requires both.

A signature whose signed attributes have no messageDigest, or name a
content type other than id-data, is now invalid. A missing content type
is reported and the signature is read anyway. A signature that still
validates with an RSA key under 2048 bits now gets a note, as SHA-1 and
RIPEMD-160 do. The four new sentences are in WORDINGS and in Spanish.

crates/sign/src/keys.rs

@@ -278,6 +278,12 @@ fn pss_params(params: Option<&Tlv<'_>>) -> Result<(DigestAlg, PssParams), SignEr
     Ok((hash, PssParams { mgf, salt_len: Some(salt_len) }))
 }
 
+/// The length in bits of the RSA modulus `n` (big-endian; leading zero bytes don't count).
+fn rsa_bits(n: &[u8]) -> usize {
+    let n = n.iter().position(|b| *b != 0).and_then(|i| n.get(i..)).unwrap_or_default();
+    n.first().map_or(0, |b| n.len() * 8 - b.leading_zeros() as usize)
+}
+
 /// A public key from a `SubjectPublicKeyInfo`.
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum PublicKey {
@@ -441,10 +447,31 @@ impl PublicKey {
         self.verify_noting(scheme, alg, digest, sig, &mut Vec::new())
     }
 
-    /// [`PublicKey::verify`], noting in `notes` each encoding irregularity it tolerated. Only
-    /// meaningful when it returns `Ok(true)`: a signature that doesn't verify may leave notes
-    /// about how far it got.
+    /// [`PublicKey::verify`], noting in `notes` each encoding irregularity it tolerated, and a key
+    /// too small to rely on ([`PublicKey::weakness`]). Only meaningful when it returns `Ok(true)`:
+    /// a signature that doesn't verify may leave notes about how far it got.
     pub fn verify_noting(&self, scheme: Scheme, alg: DigestAlg, digest: &[u8], sig: &[u8], notes: &mut Vec<&'static str>) -> Result<bool, SignError> {
+        let verified = self.verify_scheme(scheme, alg, digest, sig, notes);
+        if let Ok(true) = verified
+            && let Some(weak) = self.weakness()
+        {
+            notes.push(weak);
+        }
+        verified
+    }
+
+    /// What to tell the user when a signature that still validates was made with this key, or
+    /// `None` for one that is fine, as [`DigestAlg::weakness`] does for hashes.
+    pub fn weakness(&self) -> Option<&'static str> {
+        match self {
+            PublicKey::Rsa { n, .. } if rsa_bits(n) < 2048 => Some(
+                "This signature uses an RSA key shorter than 2048 bits, which is no longer considered strong enough for signatures. It still validates, but it should not be relied on.",
+            ),
+            _ => None,
+        }
+    }
+
+    fn verify_scheme(&self, scheme: Scheme, alg: DigestAlg, digest: &[u8], sig: &[u8], notes: &mut Vec<&'static str>) -> Result<bool, SignError> {
         match (self, scheme) {
             (PublicKey::Unsupported { what, .. }, _) => Err(SignError::Unsupported(what.clone())),
             (PublicKey::Rsa { n, e }, Scheme::RsaPkcs1) => crate::rsa_pad::pkcs1_verify(n, e, alg, digest, sig, notes),

crates/sign/src/pdf.rs

@@ -354,10 +354,14 @@ pub const WORDINGS: &[&str] = &[
     "The ECDSA signature is not in canonical DER (trailing bytes, padding or a missing sign byte); it was read anyway.",
     "The ECDSA signature is the raw r and s values instead of a DER sequence; it was read anyway.",
     "The signature algorithm and the SignerInfo's digest algorithm name different hashes; the signature was made with the SignerInfo's.",
-    // What a signature that validates with an old hash still does not prove.
+    "The signature's signed attributes have no content type, which CMS requires; it was read anyway.",
+    // What a signature that validates with an old hash or a small key still does not prove.
     "This signature uses SHA-1, a weak hash algorithm: collisions in it have been demonstrated. It still validates because many documents signed years ago use it, but it should not be relied on.",
     "This signature uses RIPEMD-160, a legacy 160-bit hash algorithm that is no longer recommended for signatures. It still validates, but it should not be relied on.",
+    "This signature uses an RSA key shorter than 2048 bits, which is no longer considered strong enough for signatures. It still validates, but it should not be relied on.",
     // Whether the signed bytes and the signature still agree.
+    "The signature's signed attributes have no message digest, so the signature does not cover the document.",
+    "The signature's signed attributes name a content type other than data, so it is not a signature of the document.",
     "The document has been altered or corrupted since the signature was applied.",
     "The signer's certificate is not in the signature.",
     "The signature value does not match the signer's certificate: the signature is corrupt.",
@@ -787,6 +791,29 @@ fn validate_into(doc: &Document, bytes: &[u8], trust: &TrustStore, v: &Dict, inf
         }
         None => cache.digest(s.digest, bytes, l0, o1, covered),
     };
+    // With signed attributes the signature is made over them, not over the document: their
+    // message digest is what ties it to the signed bytes, and their content type says what those
+    // bytes are (data, for a document). RFC 5652 §5.3 requires both.
+    if s.signed_attrs.is_some() {
+        match s.content_type.as_deref() {
+            Some(crate::cms::DATA) => {}
+            Some(_) => {
+                return invalid(
+                    info,
+                    Detail::plain("The signature's signed attributes name a content type other than data, so it is not a signature of the document."),
+                );
+            }
+            None => {
+                info.details.push(Detail::plain("The signature's signed attributes have no content type, which CMS requires; it was read anyway."))
+            }
+        }
+        if s.message_digest.is_none() {
+            return invalid(
+                info,
+                Detail::plain("The signature's signed attributes have no message digest, so the signature does not cover the document."),
+            );
+        }
+    }
     if let Some(md) = &s.message_digest
         && *md != content_digest
     {

crates/sign/tests/crypto.rs

@@ -546,6 +546,25 @@ fn weak_digests_say_so_in_words_that_are_pinned() {
     }
 }
 
+#[test]
+fn small_rsa_keys_say_so_and_others_do_not() {
+    let rsa = |n: Vec<u8>| PublicKey::Rsa { n, e: vec![1, 0, 1] };
+    let note = Some(
+        "This signature uses an RSA key shorter than 2048 bits, which is no longer considered strong enough for signatures. It still validates, but it should not be relied on.",
+    );
+    // 1024 bits, with and without the leading zero byte of a DER INTEGER.
+    assert_eq!(rsa(vec![0xC0; 128]).weakness(), note);
+    assert_eq!(rsa([vec![0], vec![0xC0; 128]].concat()).weakness(), note);
+    // 2047 bits is still short; 2048 bits and more are not.
+    assert_eq!(rsa([vec![0x7F], vec![0xFF; 255]].concat()).weakness(), note);
+    assert_eq!(rsa([vec![0, 0x80], vec![0; 255]].concat()).weakness(), None);
+    assert_eq!(rsa(vec![0xFF; 512]).weakness(), None);
+    assert_eq!(rsa(Vec::new()).weakness(), note, "an empty modulus is not a strong key");
+    assert_eq!(PublicKey::P256(vec![4; 65]).weakness(), None);
+    let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap();
+    assert_eq!(id.certificate.public_key.weakness(), None, "the 2048-bit test identity");
+}
+
 #[test]
 fn an_rsa_signature_value_not_below_the_modulus_is_refused() {
     use pdfcraft_sign::keys::{DigestAlg, PssParams, Scheme};

crates/sign/tests/pdf.rs

@@ -798,6 +798,81 @@ fn validates_signatures_written_with_ber_indefinite_lengths() {
     }
 }
 
+/// A signed attribute: the attribute type and a SET holding its one value.
+fn attribute(oid: &str, value: &[u8]) -> Vec<u8> {
+    der::seq(&[&der::oid(oid), &der::set_of(&[value])])
+}
+
+/// `signed` with its signature made again by `id` over the signed attributes `attrs`, the rest of
+/// the CMS built as the signer builds it, so only the attributes differ.
+fn resigned_with_attributes(signed: &[u8], id: &pkcs12::DigitalId, attrs: &[Vec<u8>]) -> Vec<u8> {
+    use pdfcraft_sign::DigestAlg;
+    use pdfcraft_sign::der::tag;
+    let alg = DigestAlg::Sha256;
+    let refs: Vec<&[u8]> = attrs.iter().map(Vec::as_slice).collect();
+    let set = der::set_of(&refs);
+    let signature = id.key.sign(alg, &set).unwrap();
+    let mut signed_attrs = set.clone();
+    signed_attrs[0] = tag::ctx(0);
+    let cert = &id.certificate;
+    let sid = der::seq(&[&cert.issuer.raw, &der::uint(&cert.serial)]);
+    let signer = der::seq(&[&der::int(1), &sid, &alg.algorithm(), &signed_attrs, &id.key.signature_algorithm(alg), &der::octets(&signature)]);
+    let encap = der::seq(&[&der::oid(DATA)]);
+    let signed_data =
+        der::seq(&[&der::int(1), &der::set_of(&[&alg.algorithm()]), &encap, &der::tlv(tag::ctx(0), &cert.raw), &der::set_of(&[&signer])]);
+    let cms = der::seq(&[&der::oid("1.2.840.113549.1.7.2"), &der::explicit(0, &signed_data)]);
+    let (a, b) = contents_span(signed);
+    assert!(cms.len() * 2 <= b - a, "the CMS fits the /Contents placeholder");
+    let hex: String = cms.iter().chain(std::iter::repeat(&0)).take((b - a) / 2).map(|x| format!("{x:02x}")).collect();
+    let mut out = signed.to_vec();
+    out[a..b].copy_from_slice(hex.as_bytes());
+    out
+}
+
+const DATA: &str = "1.2.840.113549.1.7.1";
+const CONTENT_TYPE: &str = "1.2.840.113549.1.9.3";
+const MESSAGE_DIGEST: &str = "1.2.840.113549.1.9.4";
+
+/// RFC 5652 §5.3: signed attributes carry the message digest, which ties the signature to the
+/// document, and the content type (data, for a document). A signature over attributes without
+/// the digest covers nothing of the document and is invalid; another content type is not a
+/// document signature; a missing content type is read anyway, with a note.
+#[test]
+fn signed_attributes_must_bind_the_document() {
+    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, &opts()).unwrap();
+    let (a, b) = contents_span(&signed);
+    let digest = pdfcraft_sign::DigestAlg::Sha256.digest(&[&signed[..a - 1], &signed[b + 1..]]);
+    let data_type = attribute(CONTENT_TYPE, &der::oid(DATA));
+    let message_digest = attribute(MESSAGE_DIGEST, &der::octets(&digest));
+    let check = |attrs: &[Vec<u8>]| {
+        let pdf = resigned_with_attributes(&signed, &id, attrs);
+        signatures(&open(&pdf), &pdf, &trust).into_iter().find(|s| s.signed).unwrap()
+    };
+    let said = |s: &pdfcraft_sign::SignatureInfo, sentence: &str| s.details.iter().any(|d| d.message == sentence);
+
+    // Both attributes, as a signer writes them: valid, so the cases below differ only in them.
+    let s = check(&[data_type.clone(), message_digest.clone()]);
+    assert_eq!(s.status, Status::Valid, "{:?}", s.details);
+
+    let s = check(std::slice::from_ref(&data_type));
+    assert_eq!(s.status, Status::Invalid, "{:?}", s.details);
+    assert!(said(&s, "The signature's signed attributes have no message digest, so the signature does not cover the document."), "{:?}", s.details);
+
+    let s = check(&[attribute(CONTENT_TYPE, &der::oid("1.2.840.113549.1.9.16.1.4")), message_digest.clone()]);
+    assert_eq!(s.status, Status::Invalid, "{:?}", s.details);
+    assert!(
+        said(&s, "The signature's signed attributes name a content type other than data, so it is not a signature of the document."),
+        "{:?}",
+        s.details
+    );
+
+    let s = check(std::slice::from_ref(&message_digest));
+    assert_eq!(s.status, Status::Valid, "{:?}", s.details);
+    assert!(said(&s, "The signature's signed attributes have no content type, which CMS requires; it was read anyway."), "{:?}", s.details);
+}
+
 #[test]
 fn validation_data_and_xmp_added_after_signing_are_not_tampering() {
     let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap();

crates/ui-egui/src/i18n/es.tsv

@@ -2298,9 +2298,13 @@ action wizard	Start	Iniciar
 	The ECDSA signature is not in canonical DER (trailing bytes, padding or a missing sign byte); it was read anyway.	La firma ECDSA no está en DER canónico (bytes de sobra, relleno o falta el byte de signo); se ha leído igualmente.
 	The ECDSA signature is the raw r and s values instead of a DER sequence; it was read anyway.	La firma ECDSA son los valores r y s en bruto en vez de una secuencia DER; se ha leído igualmente.
 	The signature algorithm and the SignerInfo's digest algorithm name different hashes; the signature was made with the SignerInfo's.	El algoritmo de la firma y el algoritmo de resumen del SignerInfo nombran hashes distintos; la firma se hizo con el del SignerInfo.
+	The signature's signed attributes have no content type, which CMS requires; it was read anyway.	Los atributos firmados de la firma no tienen tipo de contenido, que CMS exige; se ha leído igualmente.
 # What a signature that validates with an old hash still does not prove.
 	This signature uses SHA-1, a weak hash algorithm: collisions in it have been demonstrated. It still validates because many documents signed years ago use it, but it should not be relied on.	Esta firma usa SHA-1, un algoritmo de hash débil: se han demostrado colisiones en él. Sigue siendo válida porque muchos documentos firmados hace años lo usan, pero no conviene fiarse de ella.
 	This signature uses RIPEMD-160, a legacy 160-bit hash algorithm that is no longer recommended for signatures. It still validates, but it should not be relied on.	Esta firma usa RIPEMD-160, un algoritmo de hash heredado de 160 bits que ya no se recomienda para firmas. Sigue siendo válida, pero no conviene fiarse de ella.
+	This signature uses an RSA key shorter than 2048 bits, which is no longer considered strong enough for signatures. It still validates, but it should not be relied on.	Esta firma usa una clave RSA de menos de 2048 bits, que ya no se considera lo bastante fuerte para firmas. Sigue siendo válida, pero no conviene fiarse de ella.
+	The signature's signed attributes have no message digest, so the signature does not cover the document.	Los atributos firmados de la firma no tienen resumen del mensaje, así que la firma no cubre el documento.
+	The signature's signed attributes name a content type other than data, so it is not a signature of the document.	Los atributos firmados de la firma indican un tipo de contenido distinto de datos, así que no es una firma del documento.
 	The document has been altered or corrupted since the signature was applied.	El documento se ha alterado o se ha dañado desde que se aplicó la firma.
 	The signer's certificate is not in the signature.	El certificado del firmante no está en la firma.
 	The signature value does not match the signer's certificate: the signature is corrupt.	El valor de la firma no se corresponde con el certificado del firmante: la firma está dañada.