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

X509StoreRef::objects violates Rust safety guarantees #2096

Closed
davidben opened this issue Nov 19, 2023 · 1 comment
Closed

X509StoreRef::objects violates Rust safety guarantees #2096

davidben opened this issue Nov 19, 2023 · 1 comment

Comments

@davidben
Copy link
Contributor

davidben commented Nov 19, 2023

Haven't had time to put together a full TSan PoC here, but I'll just file the bug because it's fairly self-explanatory. X509StoreRef::objects wraps X509_STORE_get0_objects. As OpenSSL documents:

X509_STORE_get0_objects() retrieves an internal pointer to the store's X509 object cache. The cache contains X509 and X509_CRL objects. The returned pointer must not be freed by the calling application.

https://www.openssl.org/docs/man3.0/man3/X509_STORE_get0_objects.html

As it's an internal cache, this is of course inappropriate for rust-openssl to export in a public API. In particular, other operations in OpenSS update that cache. The directory-based X509_LOOKUP adds things to the cache as it finds them:
https://github.com/openssl/openssl/blob/master/crypto/x509/by_dir.c#L332
https://github.com/openssl/openssl/blob/master/crypto/x509/by_file.c#L127
https://github.com/openssl/openssl/blob/master/crypto/x509/x509_lu.c#L430
https://github.com/openssl/openssl/blob/master/crypto/x509/x509_lu.c#L419

Although OpenSSL internally locks this, it cannot synchronize this with someone poking around with X509_STORE_get0_objects's return value. As a result, rust-openssl's API here is not safe: it takes a shared reference to the X509_STORE, but it races with APIs like X509StoreContextRef::init which also take a shared reference to the X509_STORE but may trigger the above code.

(Really, X509_STORE_get0_objects shouldn't have been part of OpenSSL public API either, but it's a consequence of, before opaquification, legacy code reaching into the library internals. Looks like it was added to appease CPython per openssl/openssl@f0c58c3. Rust, however, has much more stringent threading expectations than CPython, so it should not have been used here.)

@alex
Copy link
Collaborator

alex commented Nov 19, 2023

I think we just want to deprecate (or entirely remove?) this and expose an API based on X509_STORE_get1_all_certs, which I imagine is what everyone actually wants in the first place.

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

No branches or pull requests

2 participants