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

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,