Add X509_STORE_get1_objects #23224
Add X509_STORE_get1_objects #23224
Conversation
davidben
force-pushed
the
get1-objects
branch
2 times, most recently
from
bbadb5d to
4e19155
Compare
X509_STORE_get0_objects returns a pointer to the X509_STORE's storage, but this function is a bit deceptive. It is practically unusable in a multi-threaded program. See, for example, RUSTSEC-2023-0072, a security vulnerability caused by this OpenSSL API. One might think that, if no other threads are mutating the X509_STORE, it is safe to read the resulting list. However, the documention does not mention that other logically-const operations on the X509_STORE, notably certifcate verifications when a hash_dir is installed, will, under a lock, write to the X509_STORE. The X509_STORE also internally re-sorts the list on the first query. If the caller knows to call X509_STORE_lock and X509_STORE_unlock, it can work around this. But this is not obvious, and the documentation does not discuss how X509_STORE_lock is very rarely safe to use. E.g. one cannot call any APIs like X509_STORE_add_cert or X509_STORE_CTX_get1_issuer while holding the lock because those functions internally expect to take the lock. (X509_STORE_lock is another such API which is not safe to export as public API.) Rather than leave all this to the caller to figure out, the API should have returned a shallow copy of the list, refcounting the values. Then it could be internally locked and the caller can freely inspect the result without synchronization with the X509_STORE.
github-actions
bot
added
the
severity: ABI change
|
CIFuzz failure seems to be unrelated. I probably should mention, given the issues with Beyond the thread-safety and double-locking flaws listed above, |
t8m
added
branch: master
nhorman
added
approval: done
|
I would also tend to make this a function that takes a parameter that indicates which objects you want included (certs or crls or both) and then the X509_STORE_get1_all_certs becomes a trivial convenience function and we don't have two different ways of implementing things - X509_STORE_get1_all_certs sorts, X509_STORE_get1_all_objects doesn't - and their internal paths are different and these sorts of differences I think will be unexpected for the user. When cleaning up a mess (which is from usage of X509_STORE rather than the intended X509_STORE_CTX functions I think we need to look around the other APIs and see if we can clean things up as well so we don't end up with slightly different ways of doing this which is only "obvious" when looking at the source. |
|
Taking a type parameter would make it harder for OpenSSL to introduce new Sorting is interesting because these values do not inherently have a sort order. The sorting is actually an internal implementation detail of how OpenSSL optimizes lookups. Except that OpenSSL does not optimize lookup very well because, as discussed above, a hash table would actually be the better structure. (You all need both insertions and deletions.) So while returning some sorted order is certainly technically possible, and you could always take your hash table order and sort afterwards, it seems something we should not be encouraging applications to rely on and instead document that the order is unspecified. That would be the most future-proof, presuming OpenSSL wishes to address
This mess doesn't have anything to do with |
|
24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually. |
|
I agree that a get0 on the entire content of the store is a poor API and should be deprecated promptly. But, I'd go further, and would suggest that even a |
|
There already are APIs to search by key (namely name). There are projects that specifically want the whole list. Now, X509_LOOKUP's rather weird caching semantics make this use case slightly odd, but they're part of the public APIs of projects like CPython, so it's a bit late to do something else now. CPython needs to know all the certs and CRLs in the store. I think a CRL counterpart of What sort of iterator did you have in mind? A pull-based iterator (i.e. one where the caller gets an object and iteratively calls "next") would still need to worry about locking. Locking around each call to "next" would be tricky because the store may be modified in between calls. If it internally saved a snapshot of the contents, it just becomes the get1 function under the hood. Also keep in mind that get1_objects doesn't preclude writing an iterator API later. You can always implement get1_objects in terms of the iterator by collecting the iterator into a list. If there is already a preferred iterator pattern, happy to evaluate it against the open source projects that need it and, if suitable, adopt it in this PR. But, as you say, OpenSSL needs to provide a replacement for get0_objects promptly. Unless you all have a concrete preferred pattern already, we should start with get1_objects and expand to other patterns later as the needs arise. |
t8m
added
approval: ready to merge
|
Merged to the master branch. Thank you for your contribution. |
X509_STORE_get0_objects returns a pointer to the X509_STORE's storage, but this function is a bit deceptive. It is practically unusable in a multi-threaded program. See, for example, RUSTSEC-2023-0072, a security vulnerability caused by this OpenSSL API. One might think that, if no other threads are mutating the X509_STORE, it is safe to read the resulting list. However, the documention does not mention that other logically-const operations on the X509_STORE, notably certifcate verifications when a hash_dir is installed, will, under a lock, write to the X509_STORE. The X509_STORE also internally re-sorts the list on the first query. If the caller knows to call X509_STORE_lock and X509_STORE_unlock, it can work around this. But this is not obvious, and the documentation does not discuss how X509_STORE_lock is very rarely safe to use. E.g. one cannot call any APIs like X509_STORE_add_cert or X509_STORE_CTX_get1_issuer while holding the lock because those functions internally expect to take the lock. (X509_STORE_lock is another such API which is not safe to export as public API.) Rather than leave all this to the caller to figure out, the API should have returned a shallow copy of the list, refcounting the values. Then it could be internally locked and the caller can freely inspect the result without synchronization with the X509_STORE. Reviewed-by: Neil Horman <nhorman@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #23224)
cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock.
cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock.
* gh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb
…thonGH-114573) * pythongh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
…thonGH-114573) * pythongh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
…thonGH-114573) * pythongh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
…H-114573) (#115549) gh-114572: Fix locking in cert_store_stats and get_ca_certs (GH-114573) * gh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
…H-114573) (#115548) gh-114572: Fix locking in cert_store_stats and get_ca_certs (GH-114573) * gh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
…H-114573) (GH-115547) gh-114572: Fix locking in cert_store_stats and get_ca_certs (GH-114573) * gh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
…H-114573) (GH-115547) gh-114572: Fix locking in cert_store_stats and get_ca_certs (GH-114573) * gh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce693111bff906ccf9281c22371331aaff766ab) Co-authored-by: David Benjamin <davidben@google.com> CPython-sync-commit-latest: 542f3272f56f31ed04e74c40635a913fbc12d286
…thon#114573) * pythongh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb
…rts (pythonGH-114573) (python#115548) pythongh-114572: Fix locking in cert_store_stats and get_ca_certs (pythonGH-114573) * pythongh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
…rts (pythonGH-114573) (python#115548) pythongh-114572: Fix locking in cert_store_stats and get_ca_certs (pythonGH-114573) * pythongh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb (cherry picked from commit bce6931) Co-authored-by: David Benjamin <davidben@google.com>
cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See gh#openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. From-PR: gh#python/cpython!114573 Fixes: gh#python#114572 Fixes: bsc#1226447 (CVE-2024-0397) Patch: CVE-2024-0397-memrace_ssl.SSLContext_cert_store.patch
|
There is nothing wrong with the This is the whole ref counting API of OpenSSL which is confusing. I don't know what is the current state of memory leaks of OpenSSL, but last time I checked initializing the library just made some memory leaks at shutdown. Before trying to rewrite the API or deprecate anything which is used everywhere, we could first prioritize to make the library itself without leak. And don't forget that OpenSSL is used outside of plain C, with other languages, and we need access to the raw information via low-level helpers, since direct access to the C structs have been deprecated. Linking to several versions of OpenSSL is perhaps not possible in plain C with its .h headers, but it is manageable to link and adapt to OpenSSL 1.1 or 3.x at runtime, from other languages. |
…thon#114573) * pythongh-114572: Fix locking in cert_store_stats and get_ca_certs cert_store_stats and get_ca_certs query the SSLContext's X509_STORE with X509_STORE_get0_objects, but reading the result requires a lock. See openssl/openssl#23224 for details. Instead, use X509_STORE_get1_objects, newly added in that PR. X509_STORE_get1_objects does not exist in current OpenSSLs, but we can polyfill it with X509_STORE_lock and X509_STORE_unlock. * Work around const-correctness problem * Add missing X509_STORE_get1_objects failure check * Add blurb
X509_STORE_get0_objects is not thread-safe and also constrains X509_STORE's internals in undesirable ways. See openssl/openssl#23224 TEST=ci Change-Id: I0d501d92208e4af9de944b38c07cbcb66acaec66 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/407840 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Brian Quinlan <bquinlan@google.com> Reviewed-by: Brian Quinlan <bquinlan@google.com>
7 participants
X509_STORE_get0_objectsreturns a pointer to theX509_STORE's storage, but this function is a bit deceptive. It is practically unusable in a multi-threaded program. See, for example, RUSTSEC-2023-0072, a security vulnerability caused by this OpenSSL API.One might think that, if no other threads are mutating the
X509_STORE, it is safe to read the resulting list. However, the documention does not mention that other logically-const operations on theX509_STORE, notably certifcate verifications when a hash_dir is installed, will, under a lock, write to theX509_STORE. TheX509_STOREalso internally re-sorts the list on the first query.If the caller knows to call
X509_STORE_lockandX509_STORE_unlock, it can work around this. But this is not obvious, and the documentation does not discuss howX509_STORE_lockis very rarely safe to use. E.g. one cannot call any APIs likeX509_STORE_add_certorX509_STORE_CTX_get1_issuerwhile holding the lock because those functions internally expect to take the lock. Conversely, if a function happens not to take the lock, but then later it does, this becomes a breaking change for anyone using it withX509_STORE_lock!X509_STORE_lockis another such API which is not safe to export as public API.Rather than leave all this to the caller to figure out, the API should have returned a shallow copy of the list, refcounting the values. Then it could be internally locked and the caller can freely inspect the result without synchronization with the
X509_STORE.Checklist