From c31294e18f10f09192374a1b9266f626e5371927 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Fri, 9 Feb 2024 18:28:23 +0100 Subject: [PATCH 1/5] 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. --- src/rust/src/pkcs7.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rust/src/pkcs7.rs b/src/rust/src/pkcs7.rs index 28edd016b863..011f191c3625 100644 --- a/src/rust/src/pkcs7.rs +++ b/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<( From 02fca8a7b3d319211ee146f31ffaf0afe9976f98 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Mon, 12 Feb 2024 21:06:29 +0100 Subject: [PATCH 2/5] 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. --- src/rust/src/pkcs7.rs | 22 +++++++++++++++++++++- src/rust/src/x509/sign.rs | 5 ++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/rust/src/pkcs7.rs b/src/rust/src/pkcs7.rs index 011f191c3625..77dd06f13c8a 100644 --- a/src/rust/src/pkcs7.rs +++ b/src/rust/src/pkcs7.rs @@ -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> { + let key_type = x509::sign::identify_key_type(py, private_key)?; + let has_pss_padding = !rsa_padding.is_none() && 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(None), + }) + } 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![]; diff --git a/src/rust/src/x509/sign.rs b/src/rust/src/x509/sign.rs index 099032210e8b..638bbbe909af 100644 --- a/src/rust/src/x509/sign.rs +++ b/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 { +pub(crate) fn identify_key_type( + py: pyo3::Python<'_>, + private_key: &pyo3::PyAny, +) -> pyo3::PyResult { 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)?)? { From b18f7b1c47d802002301ebd20c3c8ddc48c41f35 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Mon, 12 Feb 2024 21:17:20 +0100 Subject: [PATCH 3/5] Add tests for the changes in PKCS7 signing --- tests/hazmat/primitives/test_pkcs7.py | 54 ++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/tests/hazmat/primitives/test_pkcs7.py b/tests/hazmat/primitives/test_pkcs7.py index 837ad261941c..a929a9e83ae3 100644 --- a/tests/hazmat/primitives/test_pkcs7.py +++ b/tests/hazmat/primitives/test_pkcs7.py @@ -558,6 +558,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() @@ -678,9 +722,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, From c719313c4ac19903b3585dfe8986c29d996295b7 Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Tue, 20 Feb 2024 06:58:27 +0100 Subject: [PATCH 4/5] PKCS7 fixes from code review --- src/rust/src/pkcs7.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rust/src/pkcs7.rs b/src/rust/src/pkcs7.rs index 77dd06f13c8a..9732b6b93b9b 100644 --- a/src/rust/src/pkcs7.rs +++ b/src/rust/src/pkcs7.rs @@ -269,13 +269,13 @@ fn compute_pkcs7_signature_algorithm<'p>( rsa_padding: &'p pyo3::PyAny, ) -> pyo3::PyResult> { let key_type = x509::sign::identify_key_type(py, private_key)?; - let has_pss_padding = !rsa_padding.is_none() && rsa_padding.is_instance(types::PSS.get(py)?)?; + 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(None), + params: common::AlgorithmParameters::Rsa(Some(())), }) } else { x509::sign::compute_signature_algorithm(py, private_key, hash_algorithm, rsa_padding) From 99f6aba9f2f45603b735b372d8e18fec781098bb Mon Sep 17 00:00:00 2001 From: Facundo Tuesca Date: Tue, 20 Feb 2024 07:13:14 +0100 Subject: [PATCH 5/5] Update CHANGELOG --- CHANGELOG.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2a529c2d7b80..348a7770a316 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -26,6 +26,9 @@ Changelog and :class:`~cryptography.hazmat.primitives.ciphers.algorithms.ARC4` into :doc:`/hazmat/decrepit/index` and deprecated them in the ``cipher`` module. They will be removed from the ``cipher`` module in 48.0.0. +* 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: