From 6dcaabb6d23bf3c150c33fde2b48eb422d307cda Mon Sep 17 00:00:00 2001 From: Quentin Retourne <32574188+nitneuqr@users.noreply.github.com> Date: Fri, 6 Jun 2025 18:52:27 +0200 Subject: [PATCH 1/3] feat: PKCS#7 extension policies added tests accordingly adapted the pkcs7 certificate adapted EE policy do not know if a CA policy is needed! added SAN checking --- .../hazmat/primitives/serialization/pkcs7.py | 112 +++++++++++++ tests/hazmat/primitives/test_pkcs7.py | 147 +++++++++++++++++- 2 files changed, 257 insertions(+), 2 deletions(-) diff --git a/src/cryptography/hazmat/primitives/serialization/pkcs7.py b/src/cryptography/hazmat/primitives/serialization/pkcs7.py index 456dc5b0831c..d9a066e87291 100644 --- a/src/cryptography/hazmat/primitives/serialization/pkcs7.py +++ b/src/cryptography/hazmat/primitives/serialization/pkcs7.py @@ -21,6 +21,13 @@ algorithms, ) from cryptography.utils import _check_byteslike +from cryptography.x509 import Certificate +from cryptography.x509.oid import ExtendedKeyUsageOID +from cryptography.x509.verification import ( + Criticality, + ExtensionPolicy, + Policy, +) load_pem_pkcs7_certificates = rust_pkcs7.load_pem_pkcs7_certificates @@ -53,6 +60,111 @@ class PKCS7Options(utils.Enum): NoCerts = "Don't embed signer certificate" +def pkcs7_x509_extension_policies() -> tuple[ExtensionPolicy, ExtensionPolicy]: + """ + Gets the default X.509 extension policy for S/MIME. Some specifications + that differ from the standard ones: + - Certificates used as end entities (i.e., the cert used to sign + a PKCS#7/SMIME message) should not have ca=true in their basic + constraints extension. + - EKU_CLIENT_AUTH_OID is not required + - EKU_EMAIL_PROTECTION_OID is required + """ + + # CA policy - TODO: is default CA policy sufficient? Too much? + ca_policy = ExtensionPolicy.webpki_defaults_ca() + + # EE policy + def _validate_basic_constraints( + policy: Policy, cert: Certificate, bc: x509.BasicConstraints | None + ) -> None: + if bc is not None and bc.ca: + raise ValueError("Basic Constraints CA must be False.") + + def _validate_key_usage( + policy: Policy, cert: Certificate, ku: x509.KeyUsage | None + ) -> None: + if ( + ku is not None + and not ku.digital_signature + and not ku.content_commitment + ): + raise ValueError( + "Key Usage, if specified, must have at least one of the " + "digital signature or content commitment (formerly non " + "repudiation) bits set." + ) + + def _validate_subject_alternative_name( + policy: Policy, + cert: Certificate, + san: x509.SubjectAlternativeName, + ) -> None: + """ + For each general name in the SAN, for those which are email addresses: + - If it is an RFC822Name, general part must be ascii. + - If it is an OtherName, general part must be non-ascii. + """ + for general_name in san: + if ( + isinstance(general_name, x509.RFC822Name) + and "@" in general_name.value + and not general_name.value.split("@")[0].isascii() + ): + raise ValueError( + f"RFC822Name {general_name.value} contains non-ASCII " + "characters." + ) + if ( + isinstance(general_name, x509.OtherName) + and "@" in general_name.value.decode() + and general_name.value.decode().split("@")[0].isascii() + ): + raise ValueError( + f"OtherName {general_name.value.decode()} is ASCII, " + "so must be stored in RFC822Name." + ) + + def _validate_extended_key_usage( + policy: Policy, cert: Certificate, eku: x509.ExtendedKeyUsage | None + ) -> None: + if ( + eku is not None + and ExtendedKeyUsageOID.EMAIL_PROTECTION not in eku + and ExtendedKeyUsageOID.ANY_EXTENDED_KEY_USAGE not in eku + ): + raise ValueError( + "Extended Key Usage, if specified, must include " + "emailProtection or anyExtendedKeyUsage." + ) + + ee_policy = ( + ExtensionPolicy.webpki_defaults_ee() + .may_be_present( + x509.BasicConstraints, + Criticality.AGNOSTIC, + _validate_basic_constraints, + ) + .may_be_present( + x509.KeyUsage, + Criticality.CRITICAL, + _validate_key_usage, + ) + .require_present( + x509.SubjectAlternativeName, + Criticality.AGNOSTIC, + _validate_subject_alternative_name, + ) + .may_be_present( + x509.ExtendedKeyUsage, + Criticality.AGNOSTIC, + _validate_extended_key_usage, + ) + ) + + return ca_policy, ee_policy + + class PKCS7SignatureBuilder: def __init__( self, diff --git a/tests/hazmat/primitives/test_pkcs7.py b/tests/hazmat/primitives/test_pkcs7.py index 1496a23e1b2e..8d04a5419810 100644 --- a/tests/hazmat/primitives/test_pkcs7.py +++ b/tests/hazmat/primitives/test_pkcs7.py @@ -18,6 +18,16 @@ from cryptography.hazmat.primitives.asymmetric import ed25519, padding, rsa from cryptography.hazmat.primitives.ciphers import algorithms from cryptography.hazmat.primitives.serialization import pkcs7 +from cryptography.x509.oid import ( + ExtendedKeyUsageOID, + ExtensionOID, + ObjectIdentifier, +) +from cryptography.x509.verification import ( + PolicyBuilder, + Store, + VerificationError, +) from tests.x509.test_x509 import _generate_ca_and_leaf from ...hazmat.primitives.fixtures_rsa import ( @@ -125,20 +135,153 @@ def test_load_pkcs7_empty_certificates(self): def _load_cert_key(): key = load_vectors_from_file( - os.path.join("x509", "custom", "ca", "ca_key.pem"), + os.path.join("pkcs7", "ca_key.pem"), lambda pemfile: serialization.load_pem_private_key( pemfile.read(), None, unsafe_skip_rsa_key_validation=True ), mode="rb", ) cert = load_vectors_from_file( - os.path.join("x509", "custom", "ca", "ca.pem"), + os.path.join("pkcs7", "ca.pem"), loader=lambda pemfile: x509.load_pem_x509_certificate(pemfile.read()), mode="rb", ) return cert, key +class TestPKCS7VerifyCertificate: + @staticmethod + def build_pkcs7_certificate( + ca: bool = False, + digital_signature: bool = True, + usages: typing.Optional[typing.List[ObjectIdentifier]] = None, + ) -> x509.Certificate: + """ + This static method is a helper to build certificates allowing us + to test all cases in PKCS#7 certificate verification. + """ + # Load the standard certificate and private key + certificate, private_key = _load_cert_key() + + # Basic certificate builder + certificate_builder = ( + x509.CertificateBuilder() + .serial_number(certificate.serial_number) + .subject_name(certificate.subject) + .issuer_name(certificate.issuer) + .public_key(private_key.public_key()) + .not_valid_before(certificate.not_valid_before) + .not_valid_after(certificate.not_valid_after) + ) + + # Add AuthorityKeyIdentifier extension + aki = certificate.extensions.get_extension_for_oid( + ExtensionOID.AUTHORITY_KEY_IDENTIFIER + ) + certificate_builder = certificate_builder.add_extension( + aki.value, critical=False + ) + + # Add SubjectAlternativeName extension + san = certificate.extensions.get_extension_for_oid( + ExtensionOID.SUBJECT_ALTERNATIVE_NAME + ) + certificate_builder = certificate_builder.add_extension( + san.value, critical=True + ) + + # Add BasicConstraints extension + bc_extension = x509.BasicConstraints(ca=ca, path_length=None) + certificate_builder = certificate_builder.add_extension( + bc_extension, False + ) + + # Add KeyUsage extension + ku_extension = x509.KeyUsage( + digital_signature=digital_signature, + content_commitment=False, + key_encipherment=True, + data_encipherment=True, + key_agreement=True, + key_cert_sign=True, + crl_sign=True, + encipher_only=False, + decipher_only=False, + ) + certificate_builder = certificate_builder.add_extension( + ku_extension, True + ) + + # Add valid ExtendedKeyUsage extension + usages = usages or [ExtendedKeyUsageOID.EMAIL_PROTECTION] + certificate_builder = certificate_builder.add_extension( + x509.ExtendedKeyUsage(usages), True + ) + + # Build the certificate + return certificate_builder.sign( + private_key, certificate.signature_hash_algorithm, None + ) + + def test_verify_pkcs7_certificate(self): + # Prepare the parameters + certificate = self.build_pkcs7_certificate() + ca_policy, ee_policy = pkcs7.pkcs7_x509_extension_policies() + + # Verify the certificate + verifier = ( + PolicyBuilder() + .store(Store([certificate])) + .extension_policies(ca_policy=ca_policy, ee_policy=ee_policy) + .build_client_verifier() + ) + verifier.verify(certificate, []) + + @pytest.mark.parametrize( + "arguments", + [ + {"ca": True}, + {"digital_signature": False}, + {"usages": [ExtendedKeyUsageOID.CLIENT_AUTH]}, + ], + ) + def test_verify_invalid_pkcs7_certificate(self, arguments: dict): + # Prepare the parameters + certificate = self.build_pkcs7_certificate(**arguments) + + # Verify the certificate + self.verify_invalid_pkcs7_certificate(certificate) + + @staticmethod + def verify_invalid_pkcs7_certificate(certificate: x509.Certificate): + ca_policy, ee_policy = pkcs7.pkcs7_x509_extension_policies() + verifier = ( + PolicyBuilder() + .store(Store([certificate])) + .extension_policies(ca_policy=ca_policy, ee_policy=ee_policy) + .build_client_verifier() + ) + + with pytest.raises(VerificationError): + verifier.verify(certificate, []) + + @pytest.mark.parametrize( + "filename", ["ca_non_ascii_san.pem", "ca_ascii_san.pem"] + ) + def test_verify_pkcs7_certificate_wrong_san(self, filename): + # Read a certificate with an invalid SAN + pkcs7_certificate = load_vectors_from_file( + os.path.join("pkcs7", filename), + loader=lambda pemfile: x509.load_pem_x509_certificate( + pemfile.read() + ), + mode="rb", + ) + + # Verify the certificate + self.verify_invalid_pkcs7_certificate(pkcs7_certificate) + + @pytest.mark.supported( only_if=lambda backend: backend.pkcs7_supported(), skip_message="Requires OpenSSL with PKCS7 support", From 339da50de78b86dc6fb5732a40d3c00e8d8b7add Mon Sep 17 00:00:00 2001 From: Quentin Retourne <32574188+nitneuqr@users.noreply.github.com> Date: Sun, 15 Jun 2025 11:56:46 +0200 Subject: [PATCH 2/3] minor changes based on comments --- .../hazmat/primitives/serialization/pkcs7.py | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/cryptography/hazmat/primitives/serialization/pkcs7.py b/src/cryptography/hazmat/primitives/serialization/pkcs7.py index d9a066e87291..dc3c290b73e6 100644 --- a/src/cryptography/hazmat/primitives/serialization/pkcs7.py +++ b/src/cryptography/hazmat/primitives/serialization/pkcs7.py @@ -62,28 +62,33 @@ class PKCS7Options(utils.Enum): def pkcs7_x509_extension_policies() -> tuple[ExtensionPolicy, ExtensionPolicy]: """ - Gets the default X.509 extension policy for S/MIME. Some specifications - that differ from the standard ones: - - Certificates used as end entities (i.e., the cert used to sign - a PKCS#7/SMIME message) should not have ca=true in their basic - constraints extension. - - EKU_CLIENT_AUTH_OID is not required - - EKU_EMAIL_PROTECTION_OID is required + Gets the default X.509 extension policy for S/MIME, based on RFC 8550. + Visit https://www.rfc-editor.org/rfc/rfc8550#section-4.4 for more info. """ - - # CA policy - TODO: is default CA policy sufficient? Too much? + # CA policy ca_policy = ExtensionPolicy.webpki_defaults_ca() # EE policy def _validate_basic_constraints( policy: Policy, cert: Certificate, bc: x509.BasicConstraints | None ) -> None: + """ + We check that Certificates used as EE (i.e., the cert used to sign + a PKCS#7/SMIME message) must not have ca=true in their basic + constraints extension. RFC 5280 doesn't impose this requirement, but we + firmly agree about it being best practice. + """ if bc is not None and bc.ca: raise ValueError("Basic Constraints CA must be False.") def _validate_key_usage( policy: Policy, cert: Certificate, ku: x509.KeyUsage | None ) -> None: + """ + Checks that the Key Usage extension, if present, has at least one of + the digital signature or content commitment (formerly non-repudiation) + bits set. + """ if ( ku is not None and not ku.digital_signature @@ -128,6 +133,10 @@ def _validate_subject_alternative_name( def _validate_extended_key_usage( policy: Policy, cert: Certificate, eku: x509.ExtendedKeyUsage | None ) -> None: + """ + Checks that the Extended Key Usage extension, if present, + includes either emailProtection or anyExtendedKeyUsage bits. + """ if ( eku is not None and ExtendedKeyUsageOID.EMAIL_PROTECTION not in eku From 39723052074d69a9c30799f77caf45f8518bbaad Mon Sep 17 00:00:00 2001 From: Patrick Rauscher Date: Sat, 6 Sep 2025 00:58:41 +0200 Subject: [PATCH 3/3] apply patch to prepare after #13376 was merged --- tests/hazmat/primitives/test_pkcs7.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/hazmat/primitives/test_pkcs7.py b/tests/hazmat/primitives/test_pkcs7.py index 8d04a5419810..ec840d9ef015 100644 --- a/tests/hazmat/primitives/test_pkcs7.py +++ b/tests/hazmat/primitives/test_pkcs7.py @@ -20,7 +20,6 @@ from cryptography.hazmat.primitives.serialization import pkcs7 from cryptography.x509.oid import ( ExtendedKeyUsageOID, - ExtensionOID, ObjectIdentifier, ) from cryptography.x509.verification import ( @@ -135,14 +134,14 @@ def test_load_pkcs7_empty_certificates(self): def _load_cert_key(): key = load_vectors_from_file( - os.path.join("pkcs7", "ca_key.pem"), + os.path.join("x509", "custom", "ca", "ca_key.pem"), lambda pemfile: serialization.load_pem_private_key( pemfile.read(), None, unsafe_skip_rsa_key_validation=True ), mode="rb", ) cert = load_vectors_from_file( - os.path.join("pkcs7", "ca.pem"), + os.path.join("x509", "custom", "ca", "ca.pem"), loader=lambda pemfile: x509.load_pem_x509_certificate(pemfile.read()), mode="rb", ) @@ -175,19 +174,25 @@ def build_pkcs7_certificate( ) # Add AuthorityKeyIdentifier extension - aki = certificate.extensions.get_extension_for_oid( - ExtensionOID.AUTHORITY_KEY_IDENTIFIER + aki = x509.AuthorityKeyIdentifier( + b"\xfc\xeb\xb4\xd8\x12\xf2\xc9=\x99\xc3