cryptography icon indicating copy to clipboard operation
cryptography copied to clipboard

More SMIME please!

Open hughsie opened this issue 3 years ago • 12 comments

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:

  • generate a PKCS#7 detached signature for a binary blob for a specific certificate. I think the new functionality might allow me to do this.

  • verify that a specific PKCS#7 detached signature verifies a binary blob, for a list of given PKCS#7 certificates.

  • get the serial_number from a random PKCS#7 certificate

  • get the signer_serial from the detached signature.

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.

hughsie avatar Sep 21 '20 14:09 hughsie

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

reaperhulk avatar Sep 21 '20 14:09 reaperhulk

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

hughsie avatar Sep 21 '20 15:09 hughsie

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:

  • PKCS7 PEM is distinct from SMIME PEM but the current API just assumes PEM/DER. We may need to use a different Encoding enum to handle this ambiguity or add a new serialization type.
  • The SMIMESignatureBuilder should potentially be a PKCS7SignatureBuilder that can also do S/MIME.

reaperhulk avatar Sep 21 '20 15:09 reaperhulk

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.

hughsie avatar Sep 22 '20 11:09 hughsie

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).

reaperhulk avatar Sep 23 '20 02:09 reaperhulk

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.

hughsie avatar Sep 23 '20 08:09 hughsie

#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.

reaperhulk avatar Oct 25 '20 00:10 reaperhulk

Needs more thought.

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

hughsie avatar Oct 26 '20 09:10 hughsie

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

zspasztori avatar Nov 24 '21 17:11 zspasztori

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.

reaperhulk avatar Nov 28 '21 21:11 reaperhulk

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

zspasztori avatar Dec 01 '21 16:12 zspasztori

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

reaperhulk avatar Dec 01 '21 20:12 reaperhulk

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],
        )
    )

sscherfke avatar Feb 02 '23 15:02 sscherfke

Ah Germany...

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

alex avatar Feb 02 '23 15:02 alex

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:

sscherfke avatar Feb 02 '23 15:02 sscherfke

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.

alex avatar Feb 02 '23 15:02 alex

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).

sscherfke avatar Feb 02 '23 16:02 sscherfke

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.

reaperhulk avatar Feb 10 '23 15:02 reaperhulk

bitmap

ghost avatar Jul 04 '23 16:07 ghost

Please limit yourself to productive contributions.

alex avatar Jul 04 '23 16:07 alex

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.

alfonsrv avatar Aug 20 '23 17:08 alfonsrv