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

mbedtls_x509_crt_parse returns a chain, no way to "unchain" or get an unchained copy #9793

Open
Midar opened this issue Nov 20, 2024 · 4 comments

Comments

@Midar
Copy link

Midar commented Nov 20, 2024

Summary

mbedtls_x509_crt_parse returns mbedtls_x509_crt with the next pointer set. While this is fine for many use cases, it does not allow keeping a single certificate. Creating any kind of wrapper for X.509 certificates, like is often required with applications that support multiple TLS implementations, requires being able to parse a set of certificates and wrap every certificate separately.

System information

Mbed TLS version (number or commit id): 2.28.9
Operating system and version: Linux
Configuration (if not default, please attach mbedtls_config.h): Fedora default
Compiler and options (if you used a pre-built binary, please indicate how you obtained it): Fedora package
Additional environment information:

Expected behavior

There is either a way to get an array of certificates, a way to break the chain and get individual certificates, or a function that copies a single certificate with next not being set.

Actual behavior

There is no way to get the certificates in a way that they are not referencing each other. I have not tried what happens if I break the chain manually by setting next to NULL. Given it's not documented, I would expect nothing good would come from it and that even if it works now, it might cause memory corruption in the future.

Steps to reproduce

Call mbedtls_x509_crt_parse

Additional information

@mpg
Copy link
Contributor

mpg commented Nov 21, 2024

Hi @Midar and thanks for your report! I agree your request is reasonable and our API is unfortunately not making it easy.

I think there's a way to do it what you want, specifically copy a single certificate with next not being set, without relying on internals. mbedtls_x509_crt_parse(dst, src->raw.p, src->raw.len); should copy src to dst with the next pointer unset.

(To be clear: I'm not saying this is pretty, efficient or easy to discover - so a better API would be desirable. I'm just trying to offer a realistic solution that works now.)

Given it's not documented, I would expect nothing good would come from it and that even if it works now, it might cause memory corruption in the future.

Indeed, the documentation for the next field even says "Do not modify this field directly." so I fully agree this is not the way.

@gilles-peskine-arm
Copy link
Contributor

I have not tried what happens if I break the chain manually by setting next to NULL. Given it's not documented, I would expect nothing good would come from it and that even if it works now, it might cause memory corruption in the future.

That is indeed a very valid concern. An mbedtls_x509_crt object contains many embedded pointers, and those pointers are not necessarily owned by each individual mbedtls_x509_crt object. At least one common case that I'm aware of: in the verify callback from TLS, if MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is disabled, then the raw buffers in the whole chain point to a TLS buffer, and you need to copy any data that needs to remain available after the callback returns. That's just the situation as of Mbed TLS 3.6.2, and we can't guarantee that things will remain the same, e.g. if we have to fix a memory management bug in the X.509 code.

It would be nice to have an mbedtls_x509_crt_copy function, but it would actually be nontrivial to implement since it would need to ensure that it does a deep copy. Manuel's re-parse trick is a slick way to effectively do the copy at a minimal cost in complexity and code size.

@Midar
Copy link
Author

Midar commented Nov 22, 2024

mbedtls_x509_crt_parse(dst, src->raw.p, src->raw.len); should copy src to dst with the next pointer unset.

But if the buffer has multiple certs, they would all be set and chained, right?

It would be nice to have an mbedtls_x509_crt_copy function, but it would actually be nontrivial to implement since it would need to ensure that it does a deep copy.

That certainly would be an option, but also only half the solution: I would also need something to chain the certificates again. Ideally there would be a function to take a cert out of a chain and a function to insert it into a chain.

What I ended up with in the mean time is that I create a dummy chain object that wraps the entire chain. The certificate objects then reference the mbedlts_x509_crt and keep a reference to the dummy chain object (reference counted). On the other side, if I get wrapped certificates passed, I just get the dummy chain object from the certificate and unwrap that.

Code is here: https://objfw.nil.im/file?name=src/tls/OFMbedTLSX509Certificate.m&ci=tls-server

@mpg
Copy link
Contributor

mpg commented Nov 23, 2024

But if the buffer has multiple certs, they would all be set and chained, right?

I don't think so, the raw buffer should only contain one serialized cert, so parsing it should result in a single cert with with next set to NULL. (Not actually tested, but still quite confident.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants