The branch master has been updated via 2595eef82c2b67ea75cc3368529078b643a1ecb6 (commit) via e2571e02d2b0cd83ed1c79d384fe941f27e603c0 (commit) from 4599ea9fe31953c0c50738ed4b91ade76a693356 (commit)
- Log ----------------------------------------------------------------- commit 2595eef82c2b67ea75cc3368529078b643a1ecb6 Author: Bernd Edlinger <bernd.edlin...@hotmail.de> Date: Fri Nov 19 16:38:55 2021 +0100 Add a test case for duplicate engine loading Reviewed-by: Tomas Mraz <to...@openssl.org> Reviewed-by: Dmitry Belyavskiy <beld...@gmail.com> (Merged from https://github.com/openssl/openssl/pull/17073) commit e2571e02d2b0cd83ed1c79d384fe941f27e603c0 Author: Bernd Edlinger <bernd.edlin...@hotmail.de> Date: Fri Nov 19 11:33:34 2021 +0100 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 <to...@openssl.org> Reviewed-by: Dmitry Belyavskiy <beld...@gmail.com> (Merged from https://github.com/openssl/openssl/pull/17073) ----------------------------------------------------------------------- Summary of changes: crypto/engine/eng_dyn.c | 4 ++- crypto/engine/eng_lib.c | 2 ++ crypto/engine/eng_list.c | 87 +++++++++++++++++++++++++++++++++++++++++++++ crypto/engine/eng_local.h | 9 +++++ test/recipes/20-test_dgst.t | 23 ++++++++++-- 5 files changed, 122 insertions(+), 3 deletions(-) diff --git a/crypto/engine/eng_dyn.c b/crypto/engine/eng_dyn.c index f401063d37..c8a54f7d44 100644 --- a/crypto/engine/eng_dyn.c +++ b/crypto/engine/eng_dyn.c @@ -484,7 +484,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 44e997e77b..05c6a67c1e 100644 --- a/crypto/engine/eng_lib.c +++ b/crypto/engine/eng_lib.c @@ -65,6 +65,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) @@ -90,6 +91,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 fec0ef7129..04c73c7628 100644 --- a/crypto/engine/eng_list.c +++ b/crypto/engine/eng_list.c @@ -27,6 +27,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 @@ -128,6 +134,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) { @@ -278,6 +363,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 455dc1fdb7..03a86299cf 100644 --- a/crypto/engine/eng_local.h +++ b/crypto/engine/eng_local.h @@ -99,6 +99,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. @@ -143,6 +148,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; diff --git a/test/recipes/20-test_dgst.t b/test/recipes/20-test_dgst.t index 5af74aec2a..e72038d852 100644 --- a/test/recipes/20-test_dgst.t +++ b/test/recipes/20-test_dgst.t @@ -12,12 +12,12 @@ use warnings; use File::Spec; use File::Basename; -use OpenSSL::Test qw/:DEFAULT with srctop_file/; +use OpenSSL::Test qw/:DEFAULT with srctop_file bldtop_file/; use OpenSSL::Test::Utils; setup("test_dgst"); -plan tests => 9; +plan tests => 10; sub tsignverify { my $testtext = shift; @@ -103,6 +103,25 @@ SKIP: { }; } +SKIP: { + skip "dgst with engine is not supported by this OpenSSL build", 1 + if disabled("engine") || disabled("dynamic-engine"); + + subtest "SHA1 generation by engine with `dgst` CLI" => sub { + plan tests => 1; + + my $testdata = srctop_file('test', 'data.bin'); + # intentionally using -engine twice, please do not remove the duplicate line + my @macdata = run(app(['openssl', 'dgst', '-sha1', + '-engine', $^O eq 'linux' ? bldtop_file("engines", "ossltest.so") : "ossltest", + '-engine', $^O eq 'linux' ? bldtop_file("engines", "ossltest.so") : "ossltest", + $testdata]), capture => 1); + chomp(@macdata); + my $expected = qr/SHA1\(\Q$testdata\E\)= 000102030405060708090a0b0c0d0e0f10111213/; + ok($macdata[0] =~ $expected, "SHA1: Check HASH value is as expected ($macdata[0]) vs ($expected)"); + } +} + subtest "HMAC generation with `dgst` CLI" => sub { plan tests => 2;