0001-Don-t-keep-the-store-open-in-by_store_ctrl_ex.patch 4.7 KB

123456789101112131415161718192021222324252627282930313233343536373839404142434445464748495051525354555657585960616263646566676869707172737475767778798081828384858687888990919293949596979899100101102103104105106107108109110111112113114115116117118119120121122123124125126127
  1. From c0d968f0ac56ad507ab0101e537e7d530e9f0448 Mon Sep 17 00:00:00 2001
  2. From: Matt Caswell <[email protected]>
  3. Date: Thu, 7 Aug 2025 17:50:17 +0100
  4. Subject: [PATCH] Don't keep the store open in by_store_ctrl_ex
  5. MIME-Version: 1.0
  6. Content-Type: text/plain; charset=UTF-8
  7. Content-Transfer-Encoding: 8bit
  8. Previously #27529 made a change to `by_store_ctrl_ex` in order to open
  9. the OSSL_STORE early. The reason given in that PR is:
  10. "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and
  11. get to see possible errors when the URI is loaded"
  12. That PR then kept the store open until cache_objects is called and then
  13. reused it. Unfortunately by the time cache_objects() is called we could be
  14. in a multi-threaded scenario where the X509_STORE is being shared by
  15. multiple threads. We then get a race condition where multiple threads are
  16. all using (and ultimately closing) the same `OSSL_STORE_CTX`.
  17. The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex()
  18. and `cache_objects` is presumably an optimisation to avoid having to open
  19. the store twice. But this does not work because of the above issue.
  20. We just take the hit and open it again.
  21. Fixes #28171
  22. Reviewed-by: Tomas Mraz <[email protected]>
  23. Reviewed-by: Saša Nedvědický <[email protected]>
  24. (Merged from https://github.com/openssl/openssl/pull/28385)
  25. ---
  26. crypto/x509/by_store.c | 26 +++++++++++++-------------
  27. 1 file changed, 13 insertions(+), 13 deletions(-)
  28. --- a/crypto/x509/by_store.c
  29. +++ b/crypto/x509/by_store.c
  30. @@ -17,7 +17,6 @@ typedef struct cached_store_st {
  31. char *uri;
  32. OSSL_LIB_CTX *libctx;
  33. char *propq;
  34. - OSSL_STORE_CTX *ctx;
  35. } CACHED_STORE;
  36. DEFINE_STACK_OF(CACHED_STORE)
  37. @@ -27,14 +26,12 @@ static int cache_objects(X509_LOOKUP *lc
  38. const OSSL_STORE_SEARCH *criterion, int depth)
  39. {
  40. int ok = 0;
  41. - OSSL_STORE_CTX *ctx = store->ctx;
  42. + OSSL_STORE_CTX *ctx;
  43. X509_STORE *xstore = X509_LOOKUP_get_store(lctx);
  44. - if (ctx == NULL
  45. - && (ctx = OSSL_STORE_open_ex(store->uri, store->libctx, store->propq,
  46. - NULL, NULL, NULL, NULL, NULL)) == NULL)
  47. + if ((ctx = OSSL_STORE_open_ex(store->uri, store->libctx, store->propq,
  48. + NULL, NULL, NULL, NULL, NULL)) == NULL)
  49. return 0;
  50. - store->ctx = ctx;
  51. /*
  52. * We try to set the criterion, but don't care if it was valid or not.
  53. @@ -79,7 +76,6 @@ static int cache_objects(X509_LOOKUP *lc
  54. substore.uri = (char *)OSSL_STORE_INFO_get0_NAME(info);
  55. substore.libctx = store->libctx;
  56. substore.propq = store->propq;
  57. - substore.ctx = NULL;
  58. ok = cache_objects(lctx, &substore, criterion, depth - 1);
  59. }
  60. } else {
  61. @@ -105,7 +101,6 @@ static int cache_objects(X509_LOOKUP *lc
  62. break;
  63. }
  64. OSSL_STORE_close(ctx);
  65. - store->ctx = NULL;
  66. return ok;
  67. }
  68. @@ -114,7 +109,6 @@ static int cache_objects(X509_LOOKUP *lc
  69. static void free_store(CACHED_STORE *store)
  70. {
  71. if (store != NULL) {
  72. - OSSL_STORE_close(store->ctx);
  73. OPENSSL_free(store->uri);
  74. OPENSSL_free(store->propq);
  75. OPENSSL_free(store);
  76. @@ -148,6 +142,7 @@ static int by_store_ctrl_ex(X509_LOOKUP
  77. {
  78. STACK_OF(CACHED_STORE) *stores = X509_LOOKUP_get_method_data(ctx);
  79. CACHED_STORE *store = OPENSSL_zalloc(sizeof(*store));
  80. + OSSL_STORE_CTX *sctx;
  81. if (store == NULL) {
  82. return 0;
  83. @@ -157,14 +152,20 @@ static int by_store_ctrl_ex(X509_LOOKUP
  84. store->libctx = libctx;
  85. if (propq != NULL)
  86. store->propq = OPENSSL_strdup(propq);
  87. - store->ctx = OSSL_STORE_open_ex(argp, libctx, propq, NULL, NULL,
  88. - NULL, NULL, NULL);
  89. - if (store->ctx == NULL
  90. + /*
  91. + * We open this to check for errors now - so we can report those
  92. + * errors early.
  93. + */
  94. + sctx = OSSL_STORE_open_ex(argp, libctx, propq, NULL, NULL,
  95. + NULL, NULL, NULL);
  96. + if (sctx == NULL
  97. || (propq != NULL && store->propq == NULL)
  98. || store->uri == NULL) {
  99. + OSSL_STORE_close(sctx);
  100. free_store(store);
  101. return use_default;
  102. }
  103. + OSSL_STORE_close(sctx);
  104. if (stores == NULL) {
  105. stores = sk_CACHED_STORE_new_null();
  106. @@ -184,7 +185,6 @@ static int by_store_ctrl_ex(X509_LOOKUP
  107. store.uri = (char *)argp;
  108. store.libctx = libctx;
  109. store.propq = (char *)propq;
  110. - store.ctx = NULL;
  111. return cache_objects(ctx, &store, NULL, 0);
  112. }
  113. default: