Fix ASN.1 issues in PKCS#7 and S/MIME signing (#10373) (#10442)

* Fix ASN.1 for S/MIME capabilities.

The current implementation defines the SMIMECapabilities attribute
so that its value is a SEQUENCE of all the algorithm OIDs that are
supported.
However, the S/MIME v3 spec (RFC 2633) specifies that each algorithm
should be specified in its own SEQUENCE:

SMIMECapabilities ::= SEQUENCE OF SMIMECapability

SMIMECapability ::= SEQUENCE {
   capabilityID OBJECT IDENTIFIER,
   parameters ANY DEFINED BY capabilityID OPTIONAL }

(RFC 2633, Appendix A)

This commit changes the implementation so that each algorithm
is inside its own SEQUENCE. This also matches the OpenSSL
implementation.

* Fix the RSA OID used for signing PKCS#7/SMIME

The current implementation computes the algorithm identifier used
in the `digest_encryption_algorithm` PKCS#7 field
(or `SignatureAlgorithmIdentifier` in S/MIME) based on both the
algorithm used to sign (e.g. RSA) and the digest algorithm (e.g. SHA512).

This is correct for ECDSA signatures, where the OIDs used include the
digest algorithm (e.g: ecdsa-with-SHA512). However, due to historical
reasons, when signing with RSA the OID specified should be the one
corresponding to just RSA ("1.2.840.113549.1.1.1" rsaEncryption),
rather than OIDs which also include the digest algorithm (such as
"1.2.840.113549.1.1.13", sha512WithRSAEncryption).

This means that the logic to compute the algorithm identifier is the
same except when signing with RSA, in which case the OID will always
be `rsaEncryption`. This is consistent with the OpenSSL implementation,
and the RFCs that define PKCS#7 and S/MIME.

See RFC 3851 (section 2.2), and RFC 3370 (section 3.2) for more details.

* Add tests for the changes in PKCS7 signing

* PKCS7 fixes from code review

* Update CHANGELOG

Co-authored-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

CHANGELOG.rst

@@ -1,6 +1,15 @@
 Changelog
 =========
 
+.. _v42-0-4:
+
+42.0.4 - 2024-02-20
+~~~~~~~~~~~~~~~~~~~
+
+* Fixed ASN.1 encoding for PKCS7/SMIME signed messages. The fields ``SMIMECapabilities``
+  and ``SignatureAlgorithmIdentifier`` should now be correctly encoded according to the
+  definitions in :rfc:`2633` :rfc:`3370`.
+
 .. _v42-0-3:
 
 42.0.3 - 2024-02-15

src/rust/src/pkcs7.rs

@@ -104,9 +104,9 @@ fn sign_and_serialize<'p>(
         // Subset of values OpenSSL provides:
         // https://github.com/openssl/openssl/blob/667a8501f0b6e5705fd611d5bb3ca24848b07154/crypto/pkcs7/pk7_smime.c#L150
         // removing all the ones that are bad cryptography
-        AES_256_CBC_OID,
-        AES_192_CBC_OID,
-        AES_128_CBC_OID,
+        &asn1::SequenceOfWriter::new([AES_256_CBC_OID]),
+        &asn1::SequenceOfWriter::new([AES_192_CBC_OID]),
+        &asn1::SequenceOfWriter::new([AES_128_CBC_OID]),
     ]))?;
 
     let py_signers: Vec<(
@@ -205,7 +205,7 @@ fn sign_and_serialize<'p>(
             },
             digest_algorithm: digest_alg,
             authenticated_attributes: authenticated_attrs,
-            digest_encryption_algorithm: x509::sign::compute_signature_algorithm(
+            digest_encryption_algorithm: compute_pkcs7_signature_algorithm(
                 py,
                 py_private_key,
                 py_hash_alg,
@@ -262,6 +262,26 @@ fn sign_and_serialize<'p>(
     }
 }
 
+fn compute_pkcs7_signature_algorithm<'p>(
+    py: pyo3::Python<'p>,
+    private_key: &'p pyo3::PyAny,
+    hash_algorithm: &'p pyo3::PyAny,
+    rsa_padding: &'p pyo3::PyAny,
+) -> pyo3::PyResult<common::AlgorithmIdentifier<'static>> {
+    let key_type = x509::sign::identify_key_type(py, private_key)?;
+    let has_pss_padding = rsa_padding.is_instance(types::PSS.get(py)?)?;
+    // For RSA signatures (with no PSS padding), the OID is always the same no matter the
+    // digest algorithm. See RFC 3370 (section 3.2).
+    if key_type == x509::sign::KeyType::Rsa && !has_pss_padding {
+        Ok(common::AlgorithmIdentifier {
+            oid: asn1::DefinedByMarker::marker(),
+            params: common::AlgorithmParameters::Rsa(Some(())),
+        })
+    } else {
+        x509::sign::compute_signature_algorithm(py, private_key, hash_algorithm, rsa_padding)
+    }
+}
+
 fn smime_canonicalize(data: &[u8], text_mode: bool) -> (Cow<'_, [u8]>, Cow<'_, [u8]>) {
     let mut new_data_with_header = vec![];
     let mut new_data_without_header = vec![];

src/rust/src/x509/sign.rs

@@ -48,7 +48,10 @@ enum HashType {
     Sha3_512,
 }
 
-fn identify_key_type(py: pyo3::Python<'_>, private_key: &pyo3::PyAny) -> pyo3::PyResult<KeyType> {
+pub(crate) fn identify_key_type(
+    py: pyo3::Python<'_>,
+    private_key: &pyo3::PyAny,
+) -> pyo3::PyResult<KeyType> {
     if private_key.is_instance(types::RSA_PRIVATE_KEY.get(py)?)? {
         Ok(KeyType::Rsa)
     } else if private_key.is_instance(types::DSA_PRIVATE_KEY.get(py)?)? {

tests/hazmat/primitives/test_pkcs7.py

@@ -557,6 +557,50 @@ def test_sign_text(self, backend):
             backend,
         )
 
+    def test_smime_capabilities(self, backend):
+        data = b"hello world"
+        cert, key = _load_cert_key()
+        builder = (
+            pkcs7.PKCS7SignatureBuilder()
+            .set_data(data)
+            .add_signer(cert, key, hashes.SHA256())
+        )
+
+        sig_binary = builder.sign(serialization.Encoding.DER, [])
+
+        # 1.2.840.113549.1.9.15 (SMIMECapabilities) as an ASN.1 DER encoded OID
+        assert b"\x06\t*\x86H\x86\xf7\r\x01\t\x0f" in sig_binary
+
+        # 2.16.840.1.101.3.4.1.42 (aes256-CBC-PAD) as an ASN.1 DER encoded OID
+        aes256_cbc_pad_oid = b"\x06\x09\x60\x86\x48\x01\x65\x03\x04\x01\x2A"
+        # 2.16.840.1.101.3.4.1.22 (aes192-CBC-PAD) as an ASN.1 DER encoded OID
+        aes192_cbc_pad_oid = b"\x06\x09\x60\x86\x48\x01\x65\x03\x04\x01\x16"
+        # 2.16.840.1.101.3.4.1.2 (aes128-CBC-PAD) as an ASN.1 DER encoded OID
+        aes128_cbc_pad_oid = b"\x06\x09\x60\x86\x48\x01\x65\x03\x04\x01\x02"
+
+        # Each algorithm in SMIMECapabilities should be inside its own
+        # SEQUENCE.
+        # This is encoded as SEQUENCE_IDENTIFIER + LENGTH + ALGORITHM_OID.
+        # This tests that each algorithm is indeed encoded inside its own
+        # sequence. See RFC 2633, Appendix A for more details.
+        sequence_identifier = b"\x30"
+        for oid in [
+            aes256_cbc_pad_oid,
+            aes192_cbc_pad_oid,
+            aes128_cbc_pad_oid,
+        ]:
+            len_oid = len(oid).to_bytes(length=1, byteorder="big")
+            assert sequence_identifier + len_oid + oid in sig_binary
+
+        _pkcs7_verify(
+            serialization.Encoding.DER,
+            sig_binary,
+            None,
+            [cert],
+            [],
+            backend,
+        )
+
     def test_sign_no_capabilities(self, backend):
         data = b"hello world"
         cert, key = _load_cert_key()
@@ -677,9 +721,15 @@ def test_rsa_pkcs_padding_options(self, pad, backend):
                 sig.count(b"\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x08") == 1
             )
         else:
-            # This should be a pkcs1 sha512 signature
+            # This should be a pkcs1 RSA signature, which uses the
+            # `rsaEncryption` OID (1.2.840.113549.1.1.1) no matter which
+            # digest algorithm is used.
+            # See RFC 3370 section 3.2 for more details.
+            # This OID appears twice, once in the certificate itself and
+            # another in the SignerInfo data structure in the
+            # `digest_encryption_algorithm` field.
             assert (
-                sig.count(b"\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x01\x0D") == 1
+                sig.count(b"\x06\x09\x2A\x86\x48\x86\xF7\x0D\x01\x01\x01") == 2
             )
             _pkcs7_verify(
                 serialization.Encoding.DER,