Fix a mem leak when the FIPS provider is used in a different thread

We were neglecting to register the main thread to receive thread stop
notifications. This is important if the thread that starts the FIPS
provider is not the same one that is used when OPENSSL_cleanup() is
called.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21964)

diff --git a/crypto/initthread.c b/crypto/initthread.c
index 54a33c32865f7bd5c6d5638624cadebb69815de2..23ad0a07391071f07a72add10da4ff8815d87fed 100644 (file)
--- a/crypto/initthread.c
+++ b/crypto/initthread.c
@@ -249,6 +249,15 @@ void ossl_ctx_thread_stop(OSSL_LIB_CTX *ctx)
 
 #else
 
+static void ossl_arg_thread_stop(void *arg);
+
+/* Register the current thread so that we are informed if it gets stopped */
+int ossl_thread_register_fips(OSSL_LIB_CTX *libctx)
+{
+    return c_thread_start(FIPS_get_core_handle(libctx), ossl_arg_thread_stop,
+                          libctx);
+}
+
 void *ossl_thread_event_ctx_new(OSSL_LIB_CTX *libctx)
 {
     THREAD_EVENT_HANDLER **hands = NULL;
@@ -268,6 +277,16 @@ void *ossl_thread_event_ctx_new(OSSL_LIB_CTX *libctx)
     if (!CRYPTO_THREAD_set_local(tlocal, hands))
         goto err;
 
+    /*
+     * We should ideally call ossl_thread_register_fips() here. This function
+     * is called during the startup of the FIPS provider and we need to ensure
+     * that the main thread is registered to receive thread callbacks in order
+     * to free |hands| that we allocated above. However we are too early in
+     * the FIPS provider initialisation that FIPS_get_core_handle() doesn't work
+     * yet. So we defer this to the main provider OSSL_provider_init_int()
+     * function.
+     */
+
     return tlocal;
  err:
     OPENSSL_free(hands);
@@ -379,8 +398,7 @@ int ossl_init_thread_start(const void *index, void *arg,
          * libcrypto to tell us about later thread stop events. c_thread_start
          * is a callback to libcrypto defined in fipsprov.c
          */
-        if (!c_thread_start(FIPS_get_core_handle(ctx), ossl_arg_thread_stop,
-                            ctx))
+        if (!ossl_thread_register_fips(ctx))
             return 0;
     }
 #endif

diff --git a/include/crypto/context.h b/include/crypto/context.h
index 56c68f4ec114fa1db592adc01a97b700e64d80fd..af81e15e1dc515803adb3247ab713f119576c8bf 100644 (file)
--- a/include/crypto/context.h
+++ b/include/crypto/context.h
@@ -21,6 +21,7 @@ void *ossl_child_prov_ctx_new(OSSL_LIB_CTX *);
 void *ossl_prov_drbg_nonce_ctx_new(OSSL_LIB_CTX *);
 void *ossl_self_test_set_callback_new(OSSL_LIB_CTX *);
 void *ossl_rand_crng_ctx_new(OSSL_LIB_CTX *);
+int ossl_thread_register_fips(OSSL_LIB_CTX *);
 void *ossl_thread_event_ctx_new(OSSL_LIB_CTX *);
 void *ossl_fips_prov_ossl_ctx_new(OSSL_LIB_CTX *);
 #if defined(OPENSSL_THREADS)

diff --git a/providers/fips/fipsprov.c b/providers/fips/fipsprov.c
index 607ee1176316b1629e4d4b776d878dd88765f8d7..7ec409710b6cc02835d4d00f83a06d3caad895d0 100644 (file)
--- a/providers/fips/fipsprov.c
+++ b/providers/fips/fipsprov.c
@@ -705,6 +705,15 @@ int OSSL_provider_init_int(const OSSL_CORE_HANDLE *handle,
 
     fgbl->handle = handle;
 
+    /*
+     * We need to register this thread to receive thread lifecycle callbacks.
+     * This wouldn't matter if the current thread is also the same thread that
+     * closes the FIPS provider down. But if that happens on a different thread
+     * then memory leaks could otherwise occur.
+     */
+    if (!ossl_thread_register_fips(libctx))
+        goto err;
+
     /*
      * We did initial set up of selftest_params in a local copy, because we
      * could not create fgbl until c_CRYPTO_zalloc was defined in the loop