123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127 |
- From c0d968f0ac56ad507ab0101e537e7d530e9f0448 Mon Sep 17 00:00:00 2001
- From: Matt Caswell <[email protected]>
- Date: Thu, 7 Aug 2025 17:50:17 +0100
- Subject: [PATCH] Don't keep the store open in by_store_ctrl_ex
- MIME-Version: 1.0
- Content-Type: text/plain; charset=UTF-8
- Content-Transfer-Encoding: 8bit
- Previously #27529 made a change to `by_store_ctrl_ex` in order to open
- the OSSL_STORE early. The reason given in that PR is:
- "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and
- get to see possible errors when the URI is loaded"
- That PR then kept the store open until cache_objects is called and then
- reused it. Unfortunately by the time cache_objects() is called we could be
- in a multi-threaded scenario where the X509_STORE is being shared by
- multiple threads. We then get a race condition where multiple threads are
- all using (and ultimately closing) the same `OSSL_STORE_CTX`.
- The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex()
- and `cache_objects` is presumably an optimisation to avoid having to open
- the store twice. But this does not work because of the above issue.
- We just take the hit and open it again.
- Fixes #28171
- Reviewed-by: Tomas Mraz <[email protected]>
- Reviewed-by: Saša Nedvědický <[email protected]>
- (Merged from https://github.com/openssl/openssl/pull/28385)
- ---
- crypto/x509/by_store.c | 26 +++++++++++++-------------
- 1 file changed, 13 insertions(+), 13 deletions(-)
- --- a/crypto/x509/by_store.c
- +++ b/crypto/x509/by_store.c
- @@ -17,7 +17,6 @@ typedef struct cached_store_st {
- char *uri;
- OSSL_LIB_CTX *libctx;
- char *propq;
- - OSSL_STORE_CTX *ctx;
- } CACHED_STORE;
-
- DEFINE_STACK_OF(CACHED_STORE)
- @@ -27,14 +26,12 @@ static int cache_objects(X509_LOOKUP *lc
- const OSSL_STORE_SEARCH *criterion, int depth)
- {
- int ok = 0;
- - OSSL_STORE_CTX *ctx = store->ctx;
- + OSSL_STORE_CTX *ctx;
- X509_STORE *xstore = X509_LOOKUP_get_store(lctx);
-
- - if (ctx == NULL
- - && (ctx = OSSL_STORE_open_ex(store->uri, store->libctx, store->propq,
- - NULL, NULL, NULL, NULL, NULL)) == NULL)
- + if ((ctx = OSSL_STORE_open_ex(store->uri, store->libctx, store->propq,
- + NULL, NULL, NULL, NULL, NULL)) == NULL)
- return 0;
- - store->ctx = ctx;
-
- /*
- * We try to set the criterion, but don't care if it was valid or not.
- @@ -79,7 +76,6 @@ static int cache_objects(X509_LOOKUP *lc
- substore.uri = (char *)OSSL_STORE_INFO_get0_NAME(info);
- substore.libctx = store->libctx;
- substore.propq = store->propq;
- - substore.ctx = NULL;
- ok = cache_objects(lctx, &substore, criterion, depth - 1);
- }
- } else {
- @@ -105,7 +101,6 @@ static int cache_objects(X509_LOOKUP *lc
- break;
- }
- OSSL_STORE_close(ctx);
- - store->ctx = NULL;
-
- return ok;
- }
- @@ -114,7 +109,6 @@ static int cache_objects(X509_LOOKUP *lc
- static void free_store(CACHED_STORE *store)
- {
- if (store != NULL) {
- - OSSL_STORE_close(store->ctx);
- OPENSSL_free(store->uri);
- OPENSSL_free(store->propq);
- OPENSSL_free(store);
- @@ -148,6 +142,7 @@ static int by_store_ctrl_ex(X509_LOOKUP
- {
- STACK_OF(CACHED_STORE) *stores = X509_LOOKUP_get_method_data(ctx);
- CACHED_STORE *store = OPENSSL_zalloc(sizeof(*store));
- + OSSL_STORE_CTX *sctx;
-
- if (store == NULL) {
- return 0;
- @@ -157,14 +152,20 @@ static int by_store_ctrl_ex(X509_LOOKUP
- store->libctx = libctx;
- if (propq != NULL)
- store->propq = OPENSSL_strdup(propq);
- - store->ctx = OSSL_STORE_open_ex(argp, libctx, propq, NULL, NULL,
- - NULL, NULL, NULL);
- - if (store->ctx == NULL
- + /*
- + * We open this to check for errors now - so we can report those
- + * errors early.
- + */
- + sctx = OSSL_STORE_open_ex(argp, libctx, propq, NULL, NULL,
- + NULL, NULL, NULL);
- + if (sctx == NULL
- || (propq != NULL && store->propq == NULL)
- || store->uri == NULL) {
- + OSSL_STORE_close(sctx);
- free_store(store);
- return use_default;
- }
- + OSSL_STORE_close(sctx);
-
- if (stores == NULL) {
- stores = sk_CACHED_STORE_new_null();
- @@ -184,7 +185,6 @@ static int by_store_ctrl_ex(X509_LOOKUP
- store.uri = (char *)argp;
- store.libctx = libctx;
- store.propq = (char *)propq;
- - store.ctx = NULL;
- return cache_objects(ctx, &store, NULL, 0);
- }
- default:
|