Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

S/MIME signature broken with CA chain in v39.0.0 #8127

Closed
dlouzan opened this issue Jan 23, 2023 · 15 comments · Fixed by #8389
Closed

S/MIME signature broken with CA chain in v39.0.0 #8127

dlouzan opened this issue Jan 23, 2023 · 15 comments · Fixed by #8389

Comments

@dlouzan
Copy link

dlouzan commented Jan 23, 2023

Hello dear maintainers,

We have just experienced a regression when updating from v38.0.4 to v39.0.0 when generating S/MIME signatures. We have a setup with multiple intermediate CAs, and the generated signatures are broken, they do not pass smime validation.

We have tracked this to a chain sorting issue, in the generated signature the root CA ends up as the first entry in the signature chain, instead of the expected leaf > intermediate1 > intermediate2 > root order.

We are wondering if this could have to do with the recent bump of the pem library at #8043.

A sample of the code:

def load_certificates(cert_path: str) -> list[Certificate]:
    with open(cert_path, "r") as f:
        ...
        return [
            cryptography.x509.load_pem_x509_certificate(
                bytes(cert + delimiter, "utf-8")
            )
            for cert in all_certs
        ]


def load_key(key_path: str) -> CERTIFICATE_PRIVATE_KEY_TYPES:
    with open(key_path, "rb") as f:
        return cryptography.hazmat.primitives.serialization.load_pem_private_key(
            f.read(),
            None,
        )


def get_smime_attachment_content(
    data: bytes,
    key: CERTIFICATE_PRIVATE_KEY_TYPES,
    cert: Certificate,
    ca: list[Certificate],
) -> bytes:
    return PKCS7SignatureBuilder(
        data=data,
        signers=[
            (cert, key, cryptography.hazmat.primitives.hashes.SHA512()),
        ],
        additional_certs=ca,
    ).sign(
        Encoding.SMIME,
        options=[PKCS7Options.DetachedSignature],
    )

We have debugged and the ca: list[Certificate] param is pased in the right order, but the generated signature messes this up (and also breaks the signature itself). After generating the message and analyzing it with openssl, we see:

cat smime.msg | openssl smime -pk7out | openssl pkcs7 -print_certs

<root CA>
<leaf>
<intermediate1>
<intermediate2>

in a proper email we should see:

<leaf>
<intermediate1>
<intermediate2>
<root CA>

Reverting to v38.0.4 fixes the problem.

/cc @max-wittig

@reaperhulk
Copy link
Member

reaperhulk commented Jan 24, 2023

Thanks for the comprehensive report here, it's extremely helpful. We rewrote the PKCS7 signer in rust for 39 so it's likely that is the culprit (we have no tests for ordering on this, something we'll be rectifying). I'll take a closer look in a day or two when I have some time and can prepare a fix if appropriate at that point.

One side note: the PKCS7SignatureBuilder is meant to be a builder pattern -- we do not guarantee that the undocumented __init__ args will be stable, so you should consider using the public methods instead. That would look something more like:

return PKCS7SignatureBuilder()
    .set_data(data)
    .add_signer(cert, key, SHA512())
    .add_certificate(ca).sign(
        Encoding.SMIME,
        options=[PKCS7Options.DetachedSignature],
    )

@dlouzan
Copy link
Author

dlouzan commented Jan 24, 2023

@reaperhulk Thanks for the quick answer and the very helpful suggestion, we'll indeed adapt our code 💯

@alex
Copy link
Member

alex commented Jan 24, 2023

Hmm, so looking at the raw ASN.1 structure: certificate a SET OF. This means that DER requires them to be in lexicographic order -- it'd be a spec violation for us to order them a particular way.

Signature verification shouldn't rely on having the certificates in a particular order, can you explain more about how you verification works?

@dlouzan
Copy link
Author

dlouzan commented Jan 24, 2023

@alex That's a very good question that got me thinking. I'm definitely not that familiar with the insides of the DER format.

We are generating S/MIME signed emails with detached signatures, so basically passing the input email and generating the final one with the attached signature in a multipart message, the smime part including the full chain of certs involved for allowing offline verification.

In theory only the signing cert + intermediates should be needed, with the root CA being trusted on the validating client system. We are also including the root CA in the signature part, I have not seen it forbidden but maybe that's a mistake.

Then e.g. verified on the shell with:

 openssl smime -verify -in message.msg

Interestingly the S/MIME RFC actually states that the order of certs should not matter https://www.rfc-editor.org/rfc/rfc2632#section-4.2. In all systems I've seen until today we've always used the ordering as I stated above, I don't know if this is a soft-standard, or some tooling/clients have issues when certs are out of order, even though they should work (this wouldn't be the first time...). This could also of course a time for me to TIL 😅

@alex
Copy link
Member

alex commented Jan 24, 2023

Yes, including the root CA is not disallowed, but there are definitely some x.509 validators that choke on them -- this is their bug, but working around them is understandable.

You're definitely right that many systems (of all kinds, not just smime/pkcs7) treat certs as ordered [EE, intermediate1, intermediate2, etc.]. But I don't see a way to accommodate that, consistent with DER.

If openssl smime -verify assumes this, I think a bug should be filed with them.

@max-wittig
Copy link

max-wittig commented Jan 25, 2023

If openssl smime -verify assumes this, I think a bug should be filed with them.

The problem here is that also every email program on the planet will shown invalid signatures with the new version. Outlook, Evolution etc., so there has to be a standard of ordering and everything worked fine with 38 and earlier.

image

@dlouzan
Copy link
Author

dlouzan commented Jan 25, 2023

@alex Basically just what Max said: it's not only openssl which seems to depend on the ordering, Outlook at the very least shows the same problem. And the previous version 38 did indeed work, so clearly the non-RFC-ordering was ok til now 😅

Another option is that the issue is not the ordering of the certificates in the generated email, but that the actual signature is incorrectly generated in the new version? Though I think I tested with the -noverify option, so we'd do openssl smime -verify -noverify (weird naming in openssl I know), and that passed, so the signature matches the message, just that the chain cannot be properly validated / trusted.

Usage: smime [options] cert.pem...
  cert.pem... recipient certs for encryption
Valid options are:
 ...
 -verify               Verify signed message
 -nointern             Don't search certificates in message for signer
 -nosigs               Don't verify message signature
 -noverify             Don't verify signers certificate

@dlouzan
Copy link
Author

dlouzan commented Jan 25, 2023

@alex Yes, I just checked on a test email; I can properly verify the email with openssl when using -noverify to ignore the signers chain, this passes so the email signature is correct. To be 100% sure I then modified the clear-text content of the message and this failed verification even with -noverify.

So the signature generation seems to be ok, just not the chain included in the email.

@dlouzan
Copy link
Author

dlouzan commented Jan 25, 2023

@alex @max-wittig Ok sorry no! (I had picked the wrong email for testing) It actually looks like what is broken is the signature itself, the digest fails!

$ openssl smime -verify -noverify -in test.msg
Content-Type: text/plain

Content-Type: text/html;
 charset=UTF-8


Verification failure
4329764352:error:21071065:PKCS7 routines:PKCS7_signatureVerify:digest failure:crypto/pkcs7/pk7_doit.c:1011:
4329764352:error:21075069:PKCS7 routines:PKCS7_verify:signature failure:crypto/pkcs7/pk7_smime.c:353:

This email was generated:

  • Using the "hidden" constructor above (could this be related?). We have not yet migrated the code.
  • Otherwise, perhaps the issue is indeed that the signature is broken

@dlouzan dlouzan changed the title Incorrect order of CAs inside generated S/MIME signature with v39.0.0 S/MIME signature broken with CA chain in v39.0.0 Feb 3, 2023
@alex alex added this to the Fortieth release milestone Feb 3, 2023
@alex
Copy link
Member

alex commented Feb 17, 2023

Sorry I've kind of lost the thread here. Can you provide an entirely self-contained reproducer (i.e., single python file that generates the smime sig and shells out to openssl to verify it) that we can use to test this?

@max-wittig
Copy link

max-wittig commented Feb 19, 2023

@alex Thanks for picking up the thread. Yes. We've actually made a library with unit tests (https://pypi.org/project/smime-email/) and we're planning to publish the source as well on Github, just need to go to our processes. For now, here is a dump. Just put the code in a python file and run it and you will see that it succeeds with 38.0.4 and fails with 39.0.1

image

# mypy: ignore-errors

import cryptography
import cryptography.hazmat
import cryptography.hazmat.primitives.serialization
import cryptography.hazmat.primitives.serialization.pkcs7
import cryptography.x509
from cryptography.hazmat.primitives.serialization import Encoding
from cryptography.hazmat.primitives.serialization.pkcs7 import (
    PKCS7Options,
    PKCS7SignatureBuilder,
)
from cryptography.x509.base import CERTIFICATE_PRIVATE_KEY_TYPES, Certificate


def load_certificates(cert_path: str) -> list[Certificate]:
    with open(cert_path, "r") as f:
        delimiter = "-----END CERTIFICATE-----"
        split_content = f.read().split(delimiter)
        split_content = split_content[: len(split_content) - 1]
        return [
            cryptography.x509.load_pem_x509_certificate(
                bytes(cert + delimiter, "utf-8")
            )
            for cert in split_content
        ]


def load_key(key_path: str) -> CERTIFICATE_PRIVATE_KEY_TYPES:
    with open(key_path, "rb") as f:
        return cryptography.hazmat.primitives.serialization.load_pem_private_key(
            f.read(),
            None,
        )


def add_headers(headers: dict[str, str], message: bytes) -> bytes:
    content = message.decode("utf-8")
    for key, value in headers.items():
        content = f"{key}: {value}\n{content}"
    return content.encode("utf-8")


def get_smime_attachment_content(
    data: bytes,
    key: CERTIFICATE_PRIVATE_KEY_TYPES,
    cert: Certificate,
    ca: list[Certificate],
) -> bytes:
    build = (
        PKCS7SignatureBuilder()
        .set_data(data)
        .add_signer(cert, key, cryptography.hazmat.primitives.hashes.SHA512())
    )
    for c in ca:
        build = build.add_certificate(c)
    return build.sign(
        Encoding.SMIME,
        options=[PKCS7Options.DetachedSignature],
    )


import datetime
import subprocess
import tempfile
from pathlib import Path

from cryptography import x509
from cryptography.hazmat.primitives import hashes, serialization
from cryptography.hazmat.primitives.asymmetric import rsa


def generate_rsa_keypair() -> tuple[rsa.RSAPrivateKey, rsa.RSAPublicKey]:
    ca_private_key = rsa.generate_private_key(public_exponent=65537, key_size=4096)
    ca_public_key = ca_private_key.public_key()
    return ca_private_key, ca_public_key


def generate_ca_certificate(
    ca_private_key: rsa.RSAPrivateKey, ca_public_key: rsa.RSAPublicKey
) -> tuple[x509.Certificate, x509.Name]:
    ca_name = x509.Name(
        [
            x509.NameAttribute(x509.NameOID.COMMON_NAME, "Test Root CA"),
        ]
    )
    ca_cert = (
        x509.CertificateBuilder()
        .subject_name(ca_name)
        .issuer_name(ca_name)
        .public_key(ca_public_key)
        .serial_number(x509.random_serial_number())
        .not_valid_before(datetime.datetime.utcnow())
        .not_valid_after(datetime.datetime.utcnow() + datetime.timedelta(days=365))
        .add_extension(
            x509.BasicConstraints(ca=True, path_length=None),
            critical=True,
        )
        .add_extension(
            x509.SubjectKeyIdentifier.from_public_key(ca_public_key),
            critical=False,
        )
        .add_extension(
            x509.AuthorityKeyIdentifier.from_issuer_public_key(ca_public_key),
            critical=False,
        )
        .add_extension(
            x509.KeyUsage(
                digital_signature=False,
                content_commitment=False,
                key_encipherment=False,
                data_encipherment=False,
                key_agreement=False,
                key_cert_sign=True,
                crl_sign=True,
                encipher_only=False,
                decipher_only=False,
            ),
            critical=True,
        )
        .sign(ca_private_key, hashes.SHA256())
    )
    return ca_cert, ca_name


def generate_intermediate_certificate(
    intermediate_public_key: rsa.RSAPublicKey,
    ca_private_key: rsa.RSAPrivateKey,
    ca_public_key: rsa.RSAPublicKey,
    ca_name: x509.Name,
) -> tuple[x509.Certificate, x509.Name]:
    # Generate a certificate for the intermediate authority
    intermediate_name = x509.Name(
        [
            x509.NameAttribute(x509.NameOID.COMMON_NAME, "Test Intermediate CA"),
        ]
    )
    intermediate_cert = (
        x509.CertificateBuilder()
        .subject_name(intermediate_name)
        .issuer_name(ca_name)
        .public_key(intermediate_public_key)
        .serial_number(x509.random_serial_number())
        .not_valid_before(datetime.datetime.utcnow())
        .not_valid_after(datetime.datetime.utcnow() + datetime.timedelta(days=365))
        .add_extension(
            x509.BasicConstraints(ca=True, path_length=None),
            critical=True,
        )
        .add_extension(
            x509.SubjectKeyIdentifier.from_public_key(intermediate_public_key),
            critical=False,
        )
        .add_extension(
            x509.AuthorityKeyIdentifier.from_issuer_subject_key_identifier(
                x509.SubjectKeyIdentifier.from_public_key(ca_public_key)
            ),
            critical=False,
        )
        .add_extension(
            x509.KeyUsage(
                digital_signature=False,
                content_commitment=False,
                key_encipherment=False,
                data_encipherment=False,
                key_agreement=False,
                key_cert_sign=True,
                crl_sign=True,
                encipher_only=False,
                decipher_only=False,
            ),
            critical=True,
        )
        .sign(ca_private_key, hashes.SHA256())
    )
    return intermediate_cert, intermediate_name


def generate_email_certificate(
    email_public_key: rsa.RSAPublicKey,
    intermediate_private_key: rsa.RSAPrivateKey,
    intermediate_public_key: rsa.RSAPublicKey,
    intermediate_name: x509.Name,
) -> x509.Certificate:
    # Generate a certificate for the email address
    email_address = "myemail@example.com"
    name = x509.Name(
        [
            x509.NameAttribute(x509.NameOID.COMMON_NAME, "TestMailer"),
            x509.NameAttribute(x509.NameOID.EMAIL_ADDRESS, email_address),
        ]
    )
    email_cert = (
        x509.CertificateBuilder()
        .subject_name(name)
        .issuer_name(intermediate_name)
        .public_key(email_public_key)
        .serial_number(x509.random_serial_number())
        .not_valid_before(datetime.datetime.utcnow())
        .not_valid_after(datetime.datetime.utcnow() + datetime.timedelta(days=365))
        .add_extension(
            x509.BasicConstraints(ca=False, path_length=None),
            critical=True,
        )
        .add_extension(
            x509.SubjectKeyIdentifier.from_public_key(email_public_key),
            critical=False,
        )
        .add_extension(
            x509.AuthorityKeyIdentifier.from_issuer_subject_key_identifier(
                x509.SubjectKeyIdentifier.from_public_key(intermediate_public_key)
            ),
            critical=False,
        )
        .add_extension(
            x509.SubjectAlternativeName([x509.RFC822Name(email_address)]),
            critical=False,
        )
        .add_extension(
            x509.KeyUsage(
                digital_signature=True,
                content_commitment=False,
                key_encipherment=True,
                data_encipherment=False,
                key_agreement=False,
                key_cert_sign=False,
                crl_sign=False,
                encipher_only=False,
                decipher_only=False,
            ),
            critical=True,
        )
        .add_extension(
            x509.ExtendedKeyUsage(
                [
                    x509.ExtendedKeyUsageOID.CLIENT_AUTH,
                    x509.ExtendedKeyUsageOID.EMAIL_PROTECTION,
                ]
            ),
            critical=False,
        )
        .sign(intermediate_private_key, hashes.SHA256())
    )
    return email_cert


def generate_certificates() -> (
    tuple[rsa.RSAPrivateKey, x509.Certificate, x509.Certificate, x509.Certificate]
):
    ca_private_key, ca_public_key = generate_rsa_keypair()
    intermediate_private_key, intermediate_public_key = generate_rsa_keypair()
    email_private_key, email_public_key = generate_rsa_keypair()
    ca_certificate, ca_name = generate_ca_certificate(ca_private_key, ca_public_key)
    intermediate_certificate, intermediate_name = generate_intermediate_certificate(
        intermediate_public_key, ca_private_key, ca_public_key, ca_name
    )
    email_certificate = generate_email_certificate(
        email_public_key,
        intermediate_private_key,
        intermediate_public_key,
        intermediate_name,
    )
    return (
        email_private_key,
        email_certificate,
        ca_certificate,
        intermediate_certificate,
    )


def test_sign_smime_produces_valid_payload() -> None:
    (
        email_private_key,
        email_certificate,
        ca_certificate,
        intermediate_certificate,
    ) = generate_certificates()
    message_str = "test message"
    smime_content = get_smime_attachment_content(
        bytes(message_str, "utf-8"),
        email_private_key,
        email_certificate,
        [intermediate_certificate, ca_certificate],
    )
    # Combine the intermediate certificate, and root certificate into a list
    cert_chain = [intermediate_certificate, ca_certificate]

    # Concatenate the PEM-encoded certificates into a single PEM file
    full_chain_pem = b"".join(
        [cert.public_bytes(serialization.Encoding.PEM) for cert in cert_chain]
    )

    # Write the full chain to disk
    with tempfile.TemporaryDirectory() as tmpdir:
        tmpdir_path = Path(tmpdir)
        full_chain_path = tmpdir_path / "full_chain.pem"
        email_path = tmpdir_path / "email.eml"
        cert_path = tmpdir_path / "cert.pem"
        out_path = tmpdir_path / "output.txt"
        with open(full_chain_path, "wb") as f:
            f.write(full_chain_pem)
        with open(cert_path, "wb") as f:
            f.write(email_certificate.public_bytes(serialization.Encoding.PEM))
        with open(email_path, "wb") as f:
            f.write(smime_content)
        # use raw openssl to verify as cryptography can't do it, yet: https://github.com/pyca/cryptography/issues/2381
        subprocess.check_call(
            [
                "openssl",
                "smime",
                "-verify",
                "-in",
                str(email_path),
                "-CAfile",
                str(full_chain_path),
                "-out",
                str(out_path),
            ]
        )
        with open(out_path, "r") as f:
            assert f.read() == message_str


if __name__ == "__main__":
    test_sign_smime_produces_valid_payload()

@alex
Copy link
Member

alex commented Feb 19, 2023 via email

@alex
Copy link
Member

alex commented Feb 26, 2023

So I have good news (bad news? regular news?), the bug here is not cert chain related, it's actually the same bug as #8298 and therefore fixed by #8389.

@dlouzan
Copy link
Author

dlouzan commented Feb 28, 2023

@alex Thanks, we'll give this a try as soon as there's a release and report back 🙇

@alex
Copy link
Member

alex commented Feb 28, 2023 via email

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants