|
@@ -0,0 +1,127 @@
|
|
|
+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:
|