-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
gh-114572: Fix locking in cert_store_stats and get_ca_certs #114573
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this David!
This change feels newsworthy, can we get a news entry for this change using blurb?
I've reviewed the approach and I'm happy with it but will need a core developers' review before it can be merged.
return ret; | ||
} | ||
|
||
static STACK_OF(X509_OBJECT) * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting this is a faithful backport of openssl/openssl@08cecb4
@@ -4553,6 +4553,48 @@ set_sni_callback(PySSLContext *self, PyObject *arg, void *c) | |||
return 0; | |||
} | |||
|
|||
#if OPENSSL_VERSION_NUMBER < 0x30300000L | |||
static X509_OBJECT *x509_object_dup(const X509_OBJECT *obj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From looking at all the OpenSSL docs this appears to be correct? I'm less versed with the OpenSSL API than the submitter :)
Also noting that this PR can't be directly backported, some of the functions it relies on are only in OpenSSL 1.1.0+ (specifically the locking functions). |
Done. |
This will be needed for python/cpython#114573. Along the way, document the various functions that expose "query from X509_STORE". Most of them unfortunately leak the weird caching thing that hash_dir does, as well as OpenSSL's generally poor handling of issuers with the same name and CRL lookup, but I don't think it's really worth trying to unexport these APIs. Change-Id: I18137bdc4cbaa4bd20ff55116a18f350df386e4a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65787 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com>
Anything left to do on my end on this one? |
@sethmlarson This LGTM, wasn't sure if there was anything you wanted to verify before merging. |
@alex I think this is good to go! Thank you :) |
|
This doesn't look connected to this PR? |
@alex I don't think it is, thanks for digging in too :) |
…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>
GH-115547 is a backport of this pull request to the 3.12 branch. |
…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>
GH-115548 is a backport of this pull request to the 3.10 branch. |
…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>
GH-115549 is a backport of this pull request to the 3.11 branch. |
…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>
gcc 9.5 has a bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95189 that has not been backported. Essentially, the compiler will falsely reason that it's comparing a string and optimise out everything behind a null-character. Dropping static const removes optimisation options for the compiler. In my tests, gcc 9.5 now emits a direct memcmp instead of attempting any optimisations. Add X509_STORE_get1_objects This will be needed for python/cpython#114573. Along the way, document the various functions that expose "query from X509_STORE". Most of them unfortunately leak the weird caching thing that hash_dir does, as well as OpenSSL's generally poor handling of issuers with the same name and CRL lookup, but I don't think it's really worth trying to unexport these APIs. Change-Id: I18137bdc4cbaa4bd20ff55116a18f350df386e4a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65787 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> Revert "Add X509_STORE_get1_objects" This reverts commit cd5439a827a29320f7005786a26454904e4b12dc.
…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>
This concurrency fix needs reworking for use with BoringSSL.
…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>
This will be needed for python/cpython#114573. Along the way, document the various functions that expose "query from X509_STORE". Most of them unfortunately leak the weird caching thing that hash_dir does, as well as OpenSSL's generally poor handling of issuers with the same name and CRL lookup, but I don't think it's really worth trying to unexport these APIs. Change-Id: I18137bdc4cbaa4bd20ff55116a18f350df386e4a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65787 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit ba5eb621d7d9bf2872386b4303fd5e9aa64f7230)
This will be needed for python/cpython#114573. Along the way, document the various functions that expose "query from X509_STORE". Most of them unfortunately leak the weird caching thing that hash_dir does, as well as OpenSSL's generally poor handling of issuers with the same name and CRL lookup, but I don't think it's really worth trying to unexport these APIs. Change-Id: I18137bdc4cbaa4bd20ff55116a18f350df386e4a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/65787 Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Bob Beck <bbe@google.com> (cherry picked from commit ba5eb621d7d9bf2872386b4303fd5e9aa64f7230)
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.