fix(sign): eContentType must be data, and both content types must be OIDs

A content-type attribute value or an eContentType that was not an OID
was read as missing, which skipped the check that the two match. Such a
CMS is now not read, and neither is one without an eContentType.

A PDF signature's eContentType must also be data. With a content-type
attribute this already follows from the checks on the attribute.
Without one, a signature whose eContentType named another type could
still validate as a signature of the document. It is now invalid.

The PDF test now covers a wrong eContentType and a repeated
content-type attribute, and the README says that a missing
content-type attribute is read anyway, with a note.

crates/sign/README.md

@@ -86,7 +86,10 @@ needs a prompt to open is listed and asks at signing (issue #179).
 - **One content type, one message digest.** A CMS whose signed attributes repeat the content type
   or the message digest, or give either of them more or fewer than one value, is not read (RFC
   5652 §11.1, §11.2). Neither is one whose content-type attribute differs from `eContentType`:
-  the signature covers the attribute but not `eContentType`.
+  the signature covers the attribute but not `eContentType`. Both content types must be object
+  identifiers, and a PDF signature's must be data. A signature with signed attributes but no
+  content-type attribute is read anyway, with a note, although §11.1 requires one: then
+  `eContentType` alone says that the content is data.
 - **Later changes are classified narrowly.** Only the catalog's own `/Metadata` stream is the
   document's XMP, and only objects reached through the `/DSS` keys above are the security store. A
   `/Type /Metadata` or `/Type /DSS` *label* on any other dictionary or stream (a page's Form

crates/sign/src/cms.rs

@@ -50,6 +50,9 @@ pub struct SignedData {
     pub signer: SignerInfo,
     /// Encapsulated content (`adbe.pkcs7.sha1` carries the document digest here).
     pub content: Option<Vec<u8>>,
+    /// `eContentType`, the type of the encapsulated content: data for a PDF signature, TSTInfo
+    /// for an RFC 3161 token. The signature does not cover it.
+    pub econtent_type: String,
     /// Encoding irregularities that were tolerated (BER instead of DER, …), for the details.
     pub quirks: Vec<&'static str>,
 }
@@ -80,7 +83,7 @@ impl SignedData {
         let _version = it.next();
         let _digest_algs = it.next();
         let encap = it.next().ok_or_else(|| bad("encapContentInfo"))?.children()?;
-        let encap_type = encap.first().and_then(|t| t.oid().ok());
+        let econtent_type = encap.first().ok_or_else(|| bad("eContentType"))?.oid()?;
         let content = match encap.get(1) {
             Some(c) if c.tag == tag::ctx(0) => {
                 // BER signers may split the content into a constructed OCTET STRING.
@@ -116,12 +119,12 @@ impl SignedData {
         // RFC 5652 §11.1: the content-type attribute names the type of the encapsulated content.
         // The signature covers the attribute but not `eContentType`, so when they differ, one of
         // them was written wrong or changed after signing.
-        if let (Some(signed), Some(encap)) = (&signer.content_type, &encap_type)
-            && signed != encap
+        if let Some(signed) = &signer.content_type
+            && *signed != econtent_type
         {
             return Err(bad("the signed content type is not the encapsulated content type"));
         }
-        Ok(SignedData { certificates, signer, content, quirks })
+        Ok(SignedData { certificates, signer, content, econtent_type, quirks })
     }
 
     /// The signer's certificate among those carried.
@@ -234,7 +237,8 @@ fn parse_signer(si: &Tlv<'_>) -> Result<SignerInfo, SignError> {
             let value = values.into_iter().next();
             match (o.as_str(), value) {
                 (MESSAGE_DIGEST, Some(v)) => message_digest = Some(v.value.to_vec()),
-                (CONTENT_TYPE, Some(v)) => content_type = v.oid().ok(),
+                // A value that is not an OID would skip every content-type check after this.
+                (CONTENT_TYPE, Some(v)) => content_type = Some(v.oid()?),
                 (SIGNING_TIME, Some(v)) => signing_time = v.time().ok(),
                 (SIGNING_CERT_V2 | SIGNING_CERT, _) => signing_certificate = true,
                 _ => {}

crates/sign/src/pdf.rs

@@ -814,6 +814,13 @@ fn validate_into(doc: &Document, bytes: &[u8], trust: &TrustStore, v: &Dict, inf
             );
         }
     }
+    // A PDF signature signs data: the document, or (adbe.pkcs7.sha1) its digest. A content-type
+    // attribute was checked above, and `SignedData::parse` refuses one that differs from
+    // `eContentType`; without that attribute, `eContentType` is the only place that says so.
+    if sd.econtent_type != crate::cms::DATA {
+        let why = SignError::Malformed("CMS: the encapsulated content is not data".into());
+        return invalid(info, Detail::of("The signature could not be read ({}).", Fill::Reason(why.to_string())));
+    }
     if let Some(md) = &s.message_digest
         && *md != content_digest
     {

crates/sign/tests/cms.rs

@@ -1,6 +1,6 @@
 //! What a CMS's signed attributes may hold. RFC 5652 allows one content-type and one
 //! message-digest attribute, each with one value, and the content type must be the type of the
-//! encapsulated content.
+//! encapsulated content. Both content types are object identifiers.
 //!
 //! These go through `SignedData::parse`, the way a PDF's signature reaches it. Each CMS is built
 //! here as the signer builds it, so a case differs from a valid one only where it says.
@@ -29,6 +29,12 @@ fn attribute(oid: &str, values: &[&[u8]]) -> Vec<u8> {
 /// A SignedData whose encapsulated content has the type `encap_type` and whose one signer
 /// signed the attributes `attrs`.
 fn signed_data(id: &pkcs12::DigitalId, encap_type: &str, attrs: &[Vec<u8>]) -> Vec<u8> {
+    signed_data_with_encap(id, &der::seq(&[&der::oid(encap_type)]), attrs)
+}
+
+/// A SignedData with the encapContentInfo `encap`, as it stands, and whose one signer signed the
+/// attributes `attrs`.
+fn signed_data_with_encap(id: &pkcs12::DigitalId, encap: &[u8], attrs: &[Vec<u8>]) -> Vec<u8> {
     let alg = DigestAlg::Sha256;
     let refs: Vec<&[u8]> = attrs.iter().map(Vec::as_slice).collect();
     let set = der::set_of(&refs);
@@ -38,9 +44,8 @@ fn signed_data(id: &pkcs12::DigitalId, encap_type: &str, attrs: &[Vec<u8>]) -> V
     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(encap_type)]);
     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])]);
+        der::seq(&[&der::int(1), &der::set_of(&[&alg.algorithm()]), encap, &der::tlv(tag::ctx(0), &cert.raw), &der::set_of(&[&signer])]);
     der::seq(&[&der::oid(SIGNED_DATA), &der::explicit(0, &signed_data)])
 }
 
@@ -99,6 +104,27 @@ fn the_signed_content_type_is_the_type_of_the_encapsulated_content() {
     let why = refused(&signed_data(&id, SIGNED_DATA, &[signed_as(DATA), message_digest.clone()]));
     assert!(why.contains("not the encapsulated content type"), "{why}");
 
-    // Without a content-type attribute there is nothing to compare here.
-    SignedData::parse(&signed_data(&id, SIGNED_DATA, std::slice::from_ref(&message_digest))).unwrap();
+    // Without a content-type attribute there is nothing to compare here. The type is kept, and a
+    // PDF signature's validation refuses one that is not data (tests/pdf.rs).
+    let sd = SignedData::parse(&signed_data(&id, SIGNED_DATA, std::slice::from_ref(&message_digest))).unwrap();
+    assert_eq!(sd.econtent_type, SIGNED_DATA);
+}
+
+#[test]
+fn content_types_are_object_identifiers() {
+    let id = identity();
+    let digest = DigestAlg::Sha256.digest(&[b"document"]);
+    let message_digest = attribute(MESSAGE_DIGEST, &[&der::octets(&digest)]);
+    let content_type = attribute(CONTENT_TYPE, &[&der::oid(DATA)]);
+    let not_an_oid = der::octets(DATA.as_bytes());
+
+    // A content-type value that is not an OID would leave nothing to compare.
+    let why = refused(&signed_data(&id, DATA, &[attribute(CONTENT_TYPE, &[&not_an_oid]), message_digest.clone()]));
+    assert!(why.contains("expected an OID"), "{why}");
+
+    // eContentType is required (RFC 5652 §5.2) and is an OID.
+    let why = refused(&signed_data_with_encap(&id, &der::seq(&[&not_an_oid]), &[content_type.clone(), message_digest.clone()]));
+    assert!(why.contains("expected an OID"), "{why}");
+    let why = refused(&signed_data_with_encap(&id, &der::seq(&[]), &[content_type, message_digest]));
+    assert!(why.contains("eContentType"), "{why}");
 }

crates/sign/tests/pdf.rs

@@ -803,9 +803,10 @@ 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> {
+/// `signed` with its signature made again by `id` over the signed attributes `attrs` and its
+/// encapsulated content typed `encap_type`, the rest of the CMS built as the signer builds it, so
+/// only those differ.
+fn resigned_with_attributes(signed: &[u8], id: &pkcs12::DigitalId, encap_type: &str, attrs: &[Vec<u8>]) -> Vec<u8> {
     use pdfcraft_sign::DigestAlg;
     use pdfcraft_sign::der::tag;
     let alg = DigestAlg::Sha256;
@@ -817,7 +818,7 @@ fn resigned_with_attributes(signed: &[u8], id: &pkcs12::DigitalId, attrs: &[Vec<
     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 encap = der::seq(&[&der::oid(encap_type)]);
     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)]);
@@ -830,13 +831,15 @@ fn resigned_with_attributes(signed: &[u8], id: &pkcs12::DigitalId, attrs: &[Vec<
 }
 
 const DATA: &str = "1.2.840.113549.1.7.1";
+const TST_INFO: &str = "1.2.840.113549.1.9.16.1.4";
 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.
+/// document signature; a missing content type is read anyway, with a note. `eContentType` must
+/// be data too, and the same type as the attribute, which the signature covers and it does not.
 #[test]
 fn signed_attributes_must_bind_the_document() {
     let id = pkcs12::open(&data("rsa-aes.p12"), "test").unwrap();
@@ -845,12 +848,17 @@ fn signed_attributes_must_bind_the_document() {
     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 tst_type = attribute(CONTENT_TYPE, &der::oid(TST_INFO));
     let message_digest = attribute(MESSAGE_DIGEST, &der::octets(&digest));
-    let check = |attrs: &[Vec<u8>]| {
-        let pdf = resigned_with_attributes(&signed, &id, attrs);
+    let check_as = |encap_type: &str, attrs: &[Vec<u8>]| {
+        let pdf = resigned_with_attributes(&signed, &id, encap_type, attrs);
         signatures(&open(&pdf), &pdf, &trust).into_iter().find(|s| s.signed).unwrap()
     };
+    let check = |attrs: &[Vec<u8>]| check_as(DATA, attrs);
     let said = |s: &pdfcraft_sign::SignatureInfo, sentence: &str| s.details.iter().any(|d| d.message == sentence);
+    let unreadable_because = |s: &pdfcraft_sign::SignatureInfo, why: &str| {
+        s.details.iter().any(|d| d.pattern == "The signature could not be read ({})." && d.message.contains(why))
+    };
 
     // 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()]);
@@ -860,7 +868,8 @@ fn signed_attributes_must_bind_the_document() {
     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()]);
+    // An RFC 3161 token's types, TSTInfo in the attribute and in eContentType.
+    let s = check_as(TST_INFO, &[tst_type.clone(), 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."),
@@ -871,6 +880,21 @@ fn signed_attributes_must_bind_the_document() {
     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);
+
+    // A content type the attribute does not name, and one that only eContentType names.
+    let s = check_as(TST_INFO, &[data_type.clone(), message_digest.clone()]);
+    assert_eq!(s.status, Status::Invalid, "{:?}", s.details);
+    assert!(unreadable_because(&s, "the signed content type is not the encapsulated content type"), "{:?}", s.details);
+    let s = check_as(TST_INFO, std::slice::from_ref(&message_digest));
+    assert_eq!(s.status, Status::Invalid, "{:?}", s.details);
+    assert!(unreadable_because(&s, "the encapsulated content is not data"), "{:?}", s.details);
+
+    // A second content-type attribute, with the same value or another one.
+    for second in [&data_type, &tst_type] {
+        let s = check(&[data_type.clone(), second.clone(), message_digest.clone()]);
+        assert_eq!(s.status, Status::Invalid, "{:?}", s.details);
+        assert!(unreadable_because(&s, "one content type with one value"), "{:?}", s.details);
+    }
 }
 
 #[test]