From: Richard Levitte <levitte@openssl.org>
Date: Wed, 30 Apr 2025 09:38:04 +0000 (+0200)
Subject: Rework the "by store" X509_LOOKUP method to open the given URI early
X-Git-Tag: openssl-3.0.17~40
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=340383f5f49f84ed802dac268e6c12971d837f75;p=thirdparty%2Fopenssl.git

Rework the "by store" X509_LOOKUP method to open the given URI early

The cached X509_LOOKUP method data is no longer just the URI, but now
includes the OSSL_STORE_CTX pointer, and required parameters to reopen
the URI at any time.  cache_objects() is modified to handle this, and
only (re)open the URI when it wasn't previously opened, or when it was
closed by an earlier call.

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.

This assumes that if the URI could be opened once, it can be opened
again.

Fixes #27461

(cherry picked from commit 0c48ee2bf513cbc2f1de2ff8bc11750e4b593620)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/27551)

(cherry picked from commit 08220efd4aeee63094cd366099572b9e20076581)
---

diff --git a/crypto/x509/by_store.c b/crypto/x509/by_store.c
index 050735ce324..ddbd169c905 100644
--- a/crypto/x509/by_store.c
+++ b/crypto/x509/by_store.c
@@ -7,23 +7,34 @@
  * https://www.openssl.org/source/license.html
  */
 
+#include <openssl/safestack.h>
 #include <openssl/store.h>
 #include "internal/cryptlib.h"
 #include "crypto/x509.h"
 #include "x509_local.h"
 
+typedef struct cached_store_st {
+    char *uri;
+    OSSL_LIB_CTX *libctx;
+    char *propq;
+    OSSL_STORE_CTX *ctx;
+} CACHED_STORE;
+
+DEFINE_STACK_OF(CACHED_STORE)
+
 /* Generic object loader, given expected type and criterion */
-static int cache_objects(X509_LOOKUP *lctx, const char *uri,
-                         const OSSL_STORE_SEARCH *criterion,
-                         int depth, OSSL_LIB_CTX *libctx, const char *propq)
+static int cache_objects(X509_LOOKUP *lctx, CACHED_STORE *store,
+                         const OSSL_STORE_SEARCH *criterion, int depth)
 {
     int ok = 0;
-    OSSL_STORE_CTX *ctx = NULL;
+    OSSL_STORE_CTX *ctx = store->ctx;
     X509_STORE *xstore = X509_LOOKUP_get_store(lctx);
 
-    if ((ctx = OSSL_STORE_open_ex(uri, libctx, propq, NULL, NULL, NULL,
-                                  NULL, NULL)) == NULL)
+    if (ctx == NULL
+        && (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.
@@ -62,9 +73,15 @@ static int cache_objects(X509_LOOKUP *lctx, const char *uri,
              * This is an entry in the "directory" represented by the current
              * uri.  if |depth| allows, dive into it.
              */
-            if (depth > 0)
-                ok = cache_objects(lctx, OSSL_STORE_INFO_get0_NAME(info),
-                                   criterion, depth - 1, libctx, propq);
+            if (depth > 0) {
+                CACHED_STORE substore;
+
+                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 {
             /*
              * We know that X509_STORE_add_{cert|crl} increments the object's
@@ -88,27 +105,38 @@ static int cache_objects(X509_LOOKUP *lctx, const char *uri,
             break;
     }
     OSSL_STORE_close(ctx);
+    store->ctx = NULL;
 
     return ok;
 }
 
 
-/* Because OPENSSL_free is a macro and for C type match */
-static void free_uri(OPENSSL_STRING data)
+static void free_store(CACHED_STORE *store)
 {
-    OPENSSL_free(data);
+    if (store != NULL) {
+        OSSL_STORE_close(store->ctx);
+        OPENSSL_free(store->uri);
+        OPENSSL_free(store->propq);
+        OPENSSL_free(store);
+    }
 }
 
 static void by_store_free(X509_LOOKUP *ctx)
 {
-    STACK_OF(OPENSSL_STRING) *uris = X509_LOOKUP_get_method_data(ctx);
-    sk_OPENSSL_STRING_pop_free(uris, free_uri);
+    STACK_OF(CACHED_STORE) *stores = X509_LOOKUP_get_method_data(ctx);
+    sk_CACHED_STORE_pop_free(stores, free_store);
 }
 
 static int by_store_ctrl_ex(X509_LOOKUP *ctx, int cmd, const char *argp,
                             long argl, char **retp, OSSL_LIB_CTX *libctx,
                             const char *propq)
 {
+    /*
+     * In some cases below, failing to use the defaults shouldn't result in
+     * an error.  |use_default| is used as the return code in those cases.
+     */
+    int use_default = argp == NULL;
+
     switch (cmd) {
     case X509_L_ADD_STORE:
         /* If no URI is given, use the default cert dir as default URI */
@@ -118,21 +146,50 @@ static int by_store_ctrl_ex(X509_LOOKUP *ctx, int cmd, const char *argp,
             argp = X509_get_default_cert_dir();
 
         {
-            STACK_OF(OPENSSL_STRING) *uris = X509_LOOKUP_get_method_data(ctx);
-            char *data = OPENSSL_strdup(argp);
+            STACK_OF(CACHED_STORE) *stores = X509_LOOKUP_get_method_data(ctx);
+            CACHED_STORE *store = OPENSSL_zalloc(sizeof(*store));
 
-            if (data == NULL) {
+            if (store == NULL) {
                 return 0;
             }
-            if (uris == NULL) {
-                uris = sk_OPENSSL_STRING_new_null();
-                X509_LOOKUP_set_method_data(ctx, uris);
+
+            store->uri = OPENSSL_strdup(argp);
+            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
+                || (propq != NULL && store->propq == NULL)
+                || store->uri == NULL) {
+                free_store(store);
+                return use_default;
+            }
+
+            if (stores == NULL) {
+                stores = sk_CACHED_STORE_new_null();
+                if (stores != NULL)
+                    X509_LOOKUP_set_method_data(ctx, stores);
+            }
+            if (stores == NULL || sk_CACHED_STORE_push(stores, store) <= 0) {
+                free_store(store);
+                return 0;
             }
-            return sk_OPENSSL_STRING_push(uris, data) > 0;
+            return 1;
         }
-    case X509_L_LOAD_STORE:
+    case X509_L_LOAD_STORE: {
         /* This is a shortcut for quick loading of specific containers */
-        return cache_objects(ctx, argp, NULL, 0, libctx, propq);
+        CACHED_STORE store;
+
+        store.uri = (char *)argp;
+        store.libctx = libctx;
+        store.propq = (char *)propq;
+        store.ctx = NULL;
+        return cache_objects(ctx, &store, NULL, 0);
+    }
+    default:
+        /* Unsupported command */
+        return 0;
     }
 
     return 0;
@@ -148,13 +205,13 @@ static int by_store(X509_LOOKUP *ctx, X509_LOOKUP_TYPE type,
                     const OSSL_STORE_SEARCH *criterion, X509_OBJECT *ret,
                     OSSL_LIB_CTX *libctx, const char *propq)
 {
-    STACK_OF(OPENSSL_STRING) *uris = X509_LOOKUP_get_method_data(ctx);
+    STACK_OF(CACHED_STORE) *stores = X509_LOOKUP_get_method_data(ctx);
     int i;
     int ok = 0;
 
-    for (i = 0; i < sk_OPENSSL_STRING_num(uris); i++) {
-        ok = cache_objects(ctx, sk_OPENSSL_STRING_value(uris, i), criterion,
-                           1 /* depth */, libctx, propq);
+    for (i = 0; i < sk_CACHED_STORE_num(stores); i++) {
+        ok = cache_objects(ctx, sk_CACHED_STORE_value(stores, i), criterion,
+                           1 /* depth */);
 
         if (ok)
             break;