Avoid loading of a dynamic engine twice

Use the address of the bind function as a DYNAMIC_ID,
since the true name of the engine is not known
before the bind function returns,
but invoking the bind function before the engine
is unloaded results in memory corruption.

Fixes #17023

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

(cherry picked from commit e2571e02d2b0cd83ed1c79d384fe941f27e603c0)

diff --git a/crypto/engine/eng_dyn.c b/crypto/engine/eng_dyn.c
index 06e677290a700b47dca493d422bc418fbb1e7f59..cb1a30799ebecdd5ccea50015e98846efe2da3d1 100644 (file)
--- a/crypto/engine/eng_dyn.c
+++ b/crypto/engine/eng_dyn.c
@@ -477,7 +477,9 @@ static int dynamic_load(ENGINE *e, dynamic_data_ctx *ctx)
     engine_set_all_null(e);
 
     /* Try to bind the ENGINE onto our own ENGINE structure */
-    if (!ctx->bind_engine(e, ctx->engine_id, &fns)) {
+    if (!engine_add_dynamic_id(e, (ENGINE_DYNAMIC_ID)ctx->bind_engine, 1)
+            || !ctx->bind_engine(e, ctx->engine_id, &fns)) {
+        engine_remove_dynamic_id(e, 1);
         ctx->bind_engine = NULL;
         ctx->v_check = NULL;
         DSO_free(ctx->dynamic_dso);

diff --git a/crypto/engine/eng_lib.c b/crypto/engine/eng_lib.c
index 5bd584c5999a29c0527f975f978251447da3c4c6..29da4a1699b1d5ab4ed97dcc160680d50ae93577 100644 (file)
--- a/crypto/engine/eng_lib.c
+++ b/crypto/engine/eng_lib.c
@@ -67,6 +67,7 @@ void engine_set_all_null(ENGINE *e)
     e->load_pubkey = NULL;
     e->cmd_defns = NULL;
     e->flags = 0;
+    e->dynamic_id = NULL;
 }
 
 int engine_free_util(ENGINE *e, int not_locked)
@@ -92,6 +93,7 @@ int engine_free_util(ENGINE *e, int not_locked)
      */
     if (e->destroy)
         e->destroy(e);
+    engine_remove_dynamic_id(e, not_locked);
     CRYPTO_free_ex_data(CRYPTO_EX_INDEX_ENGINE, e, &e->ex_data);
     OPENSSL_free(e);
     return 1;

diff --git a/crypto/engine/eng_list.c b/crypto/engine/eng_list.c
index 1352fb7c961d83dbae0872194f747bb144b533cd..4f9eb99d15a7b9c3493c888ca4fc6429b91a51fa 100644 (file)
--- a/crypto/engine/eng_list.c
+++ b/crypto/engine/eng_list.c
@@ -24,6 +24,12 @@
 static ENGINE *engine_list_head = NULL;
 static ENGINE *engine_list_tail = NULL;
 
+/*
+ * The linked list of currently loaded dynamic engines.
+ */
+static ENGINE *engine_dyn_list_head = NULL;
+static ENGINE *engine_dyn_list_tail = NULL;
+
 /*
  * This cleanup function is only needed internally. If it should be called,
  * we register it with the "engine_cleanup_int()" stack to be called during
@@ -126,6 +132,85 @@ static int engine_list_remove(ENGINE *e)
     return 1;
 }
 
+/* Add engine to dynamic engine list. */
+int engine_add_dynamic_id(ENGINE *e, ENGINE_DYNAMIC_ID dynamic_id,
+                          int not_locked)
+{
+    int result = 0;
+    ENGINE *iterator = NULL;
+
+    if (e == NULL)
+        return 0;
+
+    if (e->dynamic_id == NULL && dynamic_id == NULL)
+        return 0;
+
+    if (not_locked && !CRYPTO_THREAD_write_lock(global_engine_lock))
+        return 0;
+
+    if (dynamic_id != NULL) {
+        iterator = engine_dyn_list_head;
+        while (iterator != NULL) {
+            if (iterator->dynamic_id == dynamic_id)
+                goto err;
+            iterator = iterator->next;
+        }
+        if (e->dynamic_id != NULL)
+            goto err;
+        e->dynamic_id = dynamic_id;
+    }
+
+    if (engine_dyn_list_head == NULL) {
+        /* We are adding to an empty list. */
+        if (engine_dyn_list_tail != NULL)
+            goto err;
+        engine_dyn_list_head = e;
+        e->prev_dyn = NULL;
+    } else {
+        /* We are adding to the tail of an existing list. */
+        if (engine_dyn_list_tail == NULL
+            || engine_dyn_list_tail->next_dyn != NULL)
+            goto err;
+        engine_dyn_list_tail->next_dyn = e;
+        e->prev_dyn = engine_dyn_list_tail;
+    }
+
+    engine_dyn_list_tail = e;
+    e->next_dyn = NULL;
+    result = 1;
+
+ err:
+    if (not_locked)
+        CRYPTO_THREAD_unlock(global_engine_lock);
+    return result;
+}
+
+/* Remove engine from dynamic engine list. */
+void engine_remove_dynamic_id(ENGINE *e, int not_locked)
+{
+    if (e == NULL || e->dynamic_id == NULL)
+        return;
+
+    if (not_locked && !CRYPTO_THREAD_write_lock(global_engine_lock))
+        return;
+
+    e->dynamic_id = NULL;
+
+    /* un-link e from the chain. */
+    if (e->next_dyn != NULL)
+        e->next_dyn->prev_dyn = e->prev_dyn;
+    if (e->prev_dyn != NULL)
+        e->prev_dyn->next_dyn = e->next_dyn;
+    /* Correct our head/tail if necessary. */
+    if (engine_dyn_list_head == e)
+        engine_dyn_list_head = e->next_dyn;
+    if (engine_dyn_list_tail == e)
+        engine_dyn_list_tail = e->prev_dyn;
+
+    if (not_locked)
+        CRYPTO_THREAD_unlock(global_engine_lock);
+}
+
 /* Get the first/last "ENGINE" type available. */
 ENGINE *ENGINE_get_first(void)
 {
@@ -272,6 +357,8 @@ static void engine_cpy(ENGINE *dest, const ENGINE *src)
     dest->load_pubkey = src->load_pubkey;
     dest->cmd_defns = src->cmd_defns;
     dest->flags = src->flags;
+    dest->dynamic_id = src->dynamic_id;
+    engine_add_dynamic_id(dest, NULL, 0);
 }
 
 ENGINE *ENGINE_by_id(const char *id)

diff --git a/crypto/engine/eng_local.h b/crypto/engine/eng_local.h
index 8ef7172b9f45a15442ed9a0dc14617ca92ff5281..ce2b7ad8d3a448c9aabeb4069d9a100bacf8ee2e 100644 (file)
--- a/crypto/engine/eng_local.h
+++ b/crypto/engine/eng_local.h
@@ -118,6 +118,11 @@ void engine_pkey_asn1_meths_free(ENGINE *e);
 extern CRYPTO_ONCE engine_lock_init;
 DECLARE_RUN_ONCE(do_engine_lock_init)
 
+typedef void (*ENGINE_DYNAMIC_ID)(void);
+int engine_add_dynamic_id(ENGINE *e, ENGINE_DYNAMIC_ID dynamic_id,
+                          int not_locked);
+void engine_remove_dynamic_id(ENGINE *e, int not_locked);
+
 /*
  * This is a structure for storing implementations of various crypto
  * algorithms and functions.
@@ -162,6 +167,10 @@ struct engine_st {
     /* Used to maintain the linked-list of engines. */
     struct engine_st *prev;
     struct engine_st *next;
+    /* Used to maintain the linked-list of dynamic engines. */
+    struct engine_st *prev_dyn;
+    struct engine_st *next_dyn;
+    ENGINE_DYNAMIC_ID dynamic_id;
 };
 
 typedef struct st_engine_pile ENGINE_PILE;