pyca / cryptography

cryptography is a package designed to expose cryptographic primitives and recipes to Python developers.
https://cryptography.io
Other
6.71k stars 1.54k forks source link

SMIME signatures with RSA-PSS #5471

Closed hughsie closed 1 year ago

hughsie commented 4 years ago

https://github.com/pyca/cryptography/commit/20c0388086b4eec91fdf1f9fd9535f4c741e4851 makes me happy as perhaps I can get rid of the custom gnutls command line scraping I use for the LVFS. These are the things the LVFS does now:

If cryptography could do those things I could probably drop a thousand lines of async python and tests from lvfs-website. It makes me a bit worried to convert to GnuTLS like this proposal https://gitlab.com/fwupd/lvfs-website/-/merge_requests/592/diffs does -- and that's not even possible unless the host OS is super new as it needs a very new libgnutls.

reaperhulk commented 4 years ago

@hughsie would it be possible to provide a sample sig for me to look at here?

hughsie commented 4 years ago

Sure thing; https://github.com/hughsie/libjcat/blob/master/data/tests/colorhug/firmware.bin.p7b

reaperhulk commented 4 years ago

You can get the cert (and the serial number from the cert) using load_pem_pkcs7_certificates. That looks roughly like this:

from cryptography.hazmat.primitives.serialization import pkcs7
with open("test.p7b", "rb") as f:
    data = f.read()
certs = pkcs7.load_pem_pkcs7_certificates(data)
print(certs[0].serial_number)

Does that get the cert info you expect?

Regarding verification...let's start with this straw man

def pkcs7_verify(encoding, sig, msg, additional_certs, trusted_certs, options)

Where additional_certs is certificates you want to pass but don't want to be explicitly trusted (e.g. the signing certs if they're not embedded in the PKCS7 structure) and trusted_certs is your set of anchors you expect to build a chain up to.

Unfortunately, we don't currently have an API for generating the PKCS7 PEM detached sig without wrapping it in additional S/MIME header nonsense. You can generate it by using the DER serialization and PEM encoding it yourself with proper line wrapping and leader/trailer, but that's not great. There should be a way to merge this with the existing S/MIME work for a slightly more abstracted PKCS7 API, but let's come back to that.

(We could also promote PKCS7 to a full-fledged object, but that doesn't really gain us much since other than certificates we will continue to largely treat PKCS7 as opaque)

That brings us to what's potentially problematic with our current API:

hughsie commented 4 years ago

Does that get the cert info you expect?

No, I get:

  File "/home/hughsie/Code/lvfs-website/lvfs/util.py", line 366, in _pkcs7_certificate_info
    certs = pkcs7.load_pem_pkcs7_certificates(text.encode())
  File "/home/hughsie/Code/lvfs-website/env/lib/python3.8/site-packages/cryptography/hazmat/primitives/serialization/pkcs7.py", line 12, in load_pem_pkcs7_certificates
    return backend.load_pem_pkcs7_certificates(data)
  File "/home/hughsie/Code/lvfs-website/env/lib/python3.8/site-packages/cryptography/hazmat/backends/openssl/backend.py", line 2652, in load_pem_pkcs7_certificates
    raise ValueError("Unable to parse PKCS7 data")
ValueError: Unable to parse PKCS7 data

let's start with this straw man

That certainly works, although I think the LVFS would only use trusted_certs

You can generate it by using the DER serialization and PEM encoding it yourself with proper line wrapping and leader/trailer, but that's not great

Agree, it seems quite a bit to get wrong.

The SMIMESignatureBuilder should potentially be a PKCS7SignatureBuilder that can also do S/MIME

Yes, that seems the "right way around" if you see what I mean.

reaperhulk commented 4 years ago

Do you get that error with the sample you gave? Because that loaded without issue for me (the integer serial number of the cert embedded is 27734639429187309363147334997).

hughsie commented 4 years ago

Do you get that error with the sample you gave

Ahh, no, sorry. I was testing with an actual PKCS certificate, not the detached signature.

reaperhulk commented 4 years ago

5496 and #5497 provide the PKCS7 PEM serialization now with the renamed class. Verification isn't going to make it into the next release since it ties us to some OpenSSL APIs we don't like much. Needs more thought.

hughsie commented 4 years ago

Needs more thought.

When you've pondered, yell if you have any verification test patches you want me to review.

zspasztori commented 3 years ago

hi, are there any updates on the pkcs7 verification? is this feature in development?

reaperhulk commented 3 years ago

No one has done the work so there's been no progress. The big challenge with verification is that it makes extensive use of OpenSSL's own X509 verification systems, which differ significantly between the forks we support and have various challenging behaviors (like user expectation of OS-level trusted root stores). These may be surmountable, but no one has stepped forward to take on the task. The fact that you can use non-public API surface to accomplish this (see our pkcs7 tests) probably contributes to this lack of forward movement, but as always be aware that we don't guarantee non-public APIs.

zspasztori commented 3 years ago

is there any similar code in the library, maybe for some verification? is verifying an X509 certificate implemented in the library?

reaperhulk commented 3 years ago

No, X509 verification is not supported in cryptography in public APIs at this time.

sscherfke commented 1 year ago

In Germany, there is a "Regelung zum Übertragungsweg" (roughly "transmission regulation") (Link) that requires S/MIME PKCS7 signatures with SHA512 and RSASSA-PSS.

You can create such signatures with openssl like this:

    cmd = [
        "openssl", "cms", "-sign",
        "-md", "sha512",
        "-in", "/dev/stdin",
        "-out", "/dev/stdout",
        "-outform", "SMIME",
        "-signer", str(our_cert),
        "-inkey", str(our_key),
        "-keyopt", "rsa_padding_mode:pss",
    ]
    result = subprocess.run(cmd, input=data, capture_output=True, check=True)

You can already build PKCS7 signatures with Cryptography, but there is no way to specify the padding (but Cryptography has support for PSS:

    signature = (
        pkcs7.PKCS7SignatureBuilder(
            data=data
        ).add_signer(
            x509.load_pem_x509_certificate(our_cert.read_bytes()),
            serialization.load_pem_private_key(our_key.read_bytes(), None),
            hashes.SHA512(),
        ).sign(
            encoding=serialization.Encoding.SMIME,
            options=[pkcs7.PKCS7Options.DetachedSignature],
        )
    )
alex commented 1 year ago

Ah Germany...

This is inter-connected with requests for PSS signatures in X.509, since they both have the same API structure.

sscherfke commented 1 year ago

Ah Germany...

I hope German regulation is a valid use case :sweat_smile:

I'm happy to help with testing, but I'm no expert when it comes to OpenSSL intricacies. :see_no_evil:

alex commented 1 year ago

Germany tends to implement cryptographic standards that require things that are not super well supported or well-designed. At least in this case the core algorithm (PSS) is fine.

The hard part here is not OpenSSL internals, but figuring out what the API should be, once we have an API, the implementation is kind of easy.

sscherfke commented 1 year ago

RSAPrivateKey.sign() already has a padding argument. Maybe you can add the same argument to PKCS7SignatureBuilder.add_signer() or PKCS7SignatureBuilder.sign() (depending on where it makes technically more sense).

reaperhulk commented 1 year ago

To properly support this we need to parse RSASSA-PSS keys appropriately too. cryptography currently strips PSS parameters (https://www.rfc-editor.org/rfc/rfc4055#section-3.1) and loads the key as a normal RSA key (we added this functionality in 37.0.0 and it's limited to OpenSSL 1.1.1e+ only), which means it can't verify that a PSS signature obeys constraints set in the RSASSA-PSS key itself. We could argue that RFC 4055 only states that following those parameters is RECOMMENDED I suppose, but I'm not happy with that.

ghost commented 1 year ago

bitmap

alex commented 1 year ago

Please limit yourself to productive contributions.

alfonsrv commented 1 year ago

Been watching this issue for quite a while and started using https://github.com/m32/endesive quite successfully in case anyone's looking for a temporary workaround other than calling the OpenSSL executable.