[openssl] master update
Matt Caswell
matt at openssl.org
Wed Feb 24 12:25:41 UTC 2021
The branch master has been updated
via 81c15ed00bbe5cb4b864ad9b1fab12a26fa91201 (commit)
via de4a88a979193e1f28c65c1f902828dd91d10ba5 (commit)
from b0001d0cf2539b9309712e3e04f407dcbb04352c (commit)
- Log -----------------------------------------------------------------
commit 81c15ed00bbe5cb4b864ad9b1fab12a26fa91201
Author: Matt Caswell <matt at openssl.org>
Date: Tue Feb 16 10:10:26 2021 +0000
Test errors from a provider can still be accessed after unload
Providers can create errors that may refer to const strings within the
provider module itself. If the provider gets unloaded we need to be sure
that we can still access the errors in the error stack.
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14213)
commit de4a88a979193e1f28c65c1f902828dd91d10ba5
Author: Matt Caswell <matt at openssl.org>
Date: Mon Feb 15 16:59:43 2021 +0000
Duplicate the file and func error strings
Errors raised from a provider that is subsequently unloaded from memory
may have references to strings representing the file and function that
are no longer present because the provider is no longer in memory. This
can cause crashes. To avoid this we duplicate the file and func strings.
Fixes #13623
Reviewed-by: Paul Dale <pauli at openssl.org>
Reviewed-by: Tomas Mraz <tomas at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14213)
-----------------------------------------------------------------------
Summary of changes:
crypto/err/err.c | 2 +-
crypto/err/err_local.h | 21 ++++++++++++--
include/openssl/err.h.in | 4 +--
test/build.info | 6 ++--
test/p_test.c | 73 ++++++++++++++++++++++++++++++++++++++++++++----
test/provider_test.c | 64 +++++++++++++++++++++++++++++++++---------
6 files changed, 143 insertions(+), 27 deletions(-)
diff --git a/crypto/err/err.c b/crypto/err/err.c
index fe91ca7b5d..e5f9866813 100644
--- a/crypto/err/err.c
+++ b/crypto/err/err.c
@@ -190,7 +190,7 @@ static void ERR_STATE_free(ERR_STATE *s)
if (s == NULL)
return;
for (i = 0; i < ERR_NUM_ERRORS; i++) {
- err_clear_data(s, i, 1);
+ err_clear(s, i, 1);
}
OPENSSL_free(s);
}
diff --git a/crypto/err/err_local.h b/crypto/err/err_local.h
index 03e05b7a1c..abb6996e13 100644
--- a/crypto/err/err_local.h
+++ b/crypto/err/err_local.h
@@ -48,9 +48,21 @@ static ossl_inline void err_set_debug(ERR_STATE *es, size_t i,
const char *file, int line,
const char *fn)
{
- es->err_file[i] = file;
+ /*
+ * We dup the file and fn strings because they may be provider owned. If the
+ * provider gets unloaded, they may not be valid anymore.
+ */
+ OPENSSL_free(es->err_file[i]);
+ if (file == NULL || file[0] == '\0')
+ es->err_file[i] = NULL;
+ else
+ es->err_file[i] = OPENSSL_strdup(file);
es->err_line[i] = line;
- es->err_func[i] = fn;
+ OPENSSL_free(es->err_func[i]);
+ if (fn == NULL || fn[0] == '\0')
+ es->err_func[i] = NULL;
+ else
+ es->err_func[i] = OPENSSL_strdup(fn);
}
static ossl_inline void err_set_data(ERR_STATE *es, size_t i,
@@ -67,8 +79,11 @@ static ossl_inline void err_clear(ERR_STATE *es, size_t i, int deall)
es->err_marks[i] = 0;
es->err_flags[i] = 0;
es->err_buffer[i] = 0;
- es->err_file[i] = NULL;
es->err_line[i] = -1;
+ OPENSSL_free(es->err_file[i]);
+ es->err_file[i] = NULL;
+ OPENSSL_free(es->err_func[i]);
+ es->err_func[i] = NULL;
}
ERR_STATE *err_get_state_int(void);
diff --git a/include/openssl/err.h.in b/include/openssl/err.h.in
index c012f65d08..f7d5c174a1 100644
--- a/include/openssl/err.h.in
+++ b/include/openssl/err.h.in
@@ -62,9 +62,9 @@ struct err_state_st {
char *err_data[ERR_NUM_ERRORS];
size_t err_data_size[ERR_NUM_ERRORS];
int err_data_flags[ERR_NUM_ERRORS];
- const char *err_file[ERR_NUM_ERRORS];
+ char *err_file[ERR_NUM_ERRORS];
int err_line[ERR_NUM_ERRORS];
- const char *err_func[ERR_NUM_ERRORS];
+ char *err_func[ERR_NUM_ERRORS];
int top, bottom;
};
# endif
diff --git a/test/build.info b/test/build.info
index 5bf35dcb10..3b0fa12d20 100644
--- a/test/build.info
+++ b/test/build.info
@@ -762,17 +762,17 @@ IF[{- !$disabled{tests} -}]
PROGRAMS{noinst}=provider_internal_test
DEFINE[provider_internal_test]=PROVIDER_INIT_FUNCTION_NAME=p_test_init
SOURCE[provider_internal_test]=provider_internal_test.c p_test.c
- INCLUDE[provider_internal_test]=../include ../apps/include
+ INCLUDE[provider_internal_test]=../include ../apps/include ..
DEPEND[provider_internal_test]=../libcrypto.a libtestutil.a
PROGRAMS{noinst}=provider_test
DEFINE[provider_test]=PROVIDER_INIT_FUNCTION_NAME=p_test_init
SOURCE[provider_test]=provider_test.c p_test.c
- INCLUDE[provider_test]=../include ../apps/include
+ INCLUDE[provider_test]=../include ../apps/include ..
DEPEND[provider_test]=../libcrypto.a libtestutil.a
IF[{- !$disabled{module} -}]
MODULES{noinst}=p_test
SOURCE[p_test]=p_test.c
- INCLUDE[p_test]=../include
+ INCLUDE[p_test]=../include ..
IF[{- defined $target{shared_defflag} -}]
SOURCE[p_test]=p_test.ld
GENERATE[p_test.ld]=../util/providers.num
diff --git a/test/p_test.c b/test/p_test.c
index dfd62ebd83..57597086aa 100644
--- a/test/p_test.c
+++ b/test/p_test.c
@@ -26,11 +26,22 @@
# define OSSL_provider_init PROVIDER_INIT_FUNCTION_NAME
#endif
+#include "e_os.h"
#include <openssl/core.h>
#include <openssl/core_dispatch.h>
+#include <openssl/err.h>
+
+typedef struct p_test_ctx {
+ char *thisfile;
+ char *thisfunc;
+ const OSSL_CORE_HANDLE *handle;
+} P_TEST_CTX;
static OSSL_FUNC_core_gettable_params_fn *c_gettable_params = NULL;
static OSSL_FUNC_core_get_params_fn *c_get_params = NULL;
+static OSSL_FUNC_core_new_error_fn *c_new_error;
+static OSSL_FUNC_core_set_error_debug_fn *c_set_error_debug;
+static OSSL_FUNC_core_vset_error_fn *c_vset_error;
/* Tell the core what params we provide and what type they are */
static const OSSL_PARAM p_param_types[] = {
@@ -42,15 +53,17 @@ static const OSSL_PARAM p_param_types[] = {
static OSSL_FUNC_provider_gettable_params_fn p_gettable_params;
static OSSL_FUNC_provider_get_params_fn p_get_params;
static OSSL_FUNC_provider_get_reason_strings_fn p_get_reason_strings;
+static OSSL_FUNC_provider_teardown_fn p_teardown;
static const OSSL_PARAM *p_gettable_params(void *_)
{
return p_param_types;
}
-static int p_get_params(void *vhand, OSSL_PARAM params[])
+static int p_get_params(void *provctx, OSSL_PARAM params[])
{
- const OSSL_CORE_HANDLE *hand = vhand;
+ P_TEST_CTX *ctx = (P_TEST_CTX *)provctx;
+ const OSSL_CORE_HANDLE *hand = ctx->handle;
OSSL_PARAM *p = params;
int ok = 1;
@@ -101,6 +114,14 @@ static int p_get_params(void *vhand, OSSL_PARAM params[])
return ok;
}
+static void p_set_error(int lib, int reason, const char *file, int line,
+ const char *func)
+{
+ c_new_error(NULL);
+ c_set_error_debug(NULL, file, line, func);
+ c_vset_error(NULL, ERR_PACK(lib, 0, reason), NULL, NULL);
+}
+
static const OSSL_ITEM *p_get_reason_strings(void *_)
{
static const OSSL_ITEM reason_strings[] = {
@@ -116,6 +137,7 @@ static const OSSL_DISPATCH p_test_table[] = {
{ OSSL_FUNC_PROVIDER_GET_PARAMS, (void (*)(void))p_get_params },
{ OSSL_FUNC_PROVIDER_GET_REASON_STRINGS,
(void (*)(void))p_get_reason_strings},
+ { OSSL_FUNC_PROVIDER_TEARDOWN, (void (*)(void))p_teardown },
{ 0, NULL }
};
@@ -124,6 +146,8 @@ int OSSL_provider_init(const OSSL_CORE_HANDLE *handle,
const OSSL_DISPATCH **out,
void **provctx)
{
+ P_TEST_CTX *ctx;
+
for (; in->function_id != 0; in++) {
switch (in->function_id) {
case OSSL_FUNC_CORE_GETTABLE_PARAMS:
@@ -132,15 +156,54 @@ int OSSL_provider_init(const OSSL_CORE_HANDLE *handle,
case OSSL_FUNC_CORE_GET_PARAMS:
c_get_params = OSSL_FUNC_core_get_params(in);
break;
+ case OSSL_FUNC_CORE_NEW_ERROR:
+ c_new_error = OSSL_FUNC_core_new_error(in);
+ break;
+ case OSSL_FUNC_CORE_SET_ERROR_DEBUG:
+ c_set_error_debug = OSSL_FUNC_core_set_error_debug(in);
+ break;
+ case OSSL_FUNC_CORE_VSET_ERROR:
+ c_vset_error = OSSL_FUNC_core_vset_error(in);
+ break;
default:
/* Just ignore anything we don't understand */
break;
}
}
- /* Because we use this in get_params, we need to pass it back */
- *provctx = (void *)handle;
-
+ /*
+ * We want to test that libcrypto doesn't use the file and func pointers
+ * that we provide to it via c_set_error_debug beyond the time that they
+ * are valid for. Therefore we dynamically allocate these strings now and
+ * free them again when the provider is torn down. If anything tries to
+ * use those strings after that point there will be a use-after-free and
+ * asan will complain (and hence the tests will fail).
+ * This file isn't linked against libcrypto, so we use malloc and strdup
+ * instead of OPENSSL_malloc and OPENSSL_strdup
+ */
+ ctx = malloc(sizeof(*ctx));
+ if (ctx == NULL)
+ return 0;
+ ctx->thisfile = strdup(OPENSSL_FILE);
+ ctx->thisfunc = strdup(OPENSSL_FUNC);
+ ctx->handle = handle;
+
+ /*
+ * Set a spurious error to check error handling works correctly. This will
+ * be ignored
+ */
+ p_set_error(ERR_LIB_PROV, 1, ctx->thisfile, OPENSSL_LINE, ctx->thisfunc);
+
+ *provctx = (void *)ctx;
*out = p_test_table;
return 1;
}
+
+static void p_teardown(void *provctx)
+{
+ P_TEST_CTX *ctx = (P_TEST_CTX *)provctx;
+
+ free(ctx->thisfile);
+ free(ctx->thisfunc);
+ free(ctx);
+}
diff --git a/test/provider_test.c b/test/provider_test.c
index acb9f2000e..7406bb4318 100644
--- a/test/provider_test.c
+++ b/test/provider_test.c
@@ -19,41 +19,79 @@ static OSSL_PARAM greeting_request[] = {
{ NULL, 0, NULL, 0, 0 }
};
-static int test_provider(const char *name)
+static int test_provider(OSSL_LIB_CTX **libctx, const char *name)
{
OSSL_PROVIDER *prov = NULL;
const char *greeting = NULL;
char expected_greeting[256];
+ int ok = 0;
+ long err;
BIO_snprintf(expected_greeting, sizeof(expected_greeting),
"Hello OpenSSL %.20s, greetings from %s!",
OPENSSL_VERSION_STR, name);
- return
- TEST_ptr(prov = OSSL_PROVIDER_load(NULL, name))
- && TEST_true(OSSL_PROVIDER_get_params(prov, greeting_request))
- && TEST_ptr(greeting = greeting_request[0].data)
- && TEST_size_t_gt(greeting_request[0].data_size, 0)
- && TEST_str_eq(greeting, expected_greeting)
- && TEST_true(OSSL_PROVIDER_unload(prov));
+ if (!TEST_ptr(prov = OSSL_PROVIDER_load(*libctx, name))
+ || !TEST_true(OSSL_PROVIDER_get_params(prov, greeting_request))
+ || !TEST_ptr(greeting = greeting_request[0].data)
+ || !TEST_size_t_gt(greeting_request[0].data_size, 0)
+ || !TEST_str_eq(greeting, expected_greeting)
+ || !TEST_true(OSSL_PROVIDER_unload(prov)))
+ goto err;
+
+ prov = NULL;
+
+ /*
+ * We must free the libctx to force the provider to really be unloaded from
+ * memory
+ */
+ OSSL_LIB_CTX_free(*libctx);
+ *libctx = NULL;
+
+ /* Make sure we got the error we were expecting */
+ err = ERR_peek_last_error();
+ if (!TEST_int_gt(err, 0)
+ || !TEST_int_eq(ERR_GET_REASON(err), 1))
+ goto err;
+
+ /* We print out all the data to make sure it can still be accessed */
+ ERR_print_errors_fp(stderr);
+ ok = 1;
+ err:
+ OSSL_PROVIDER_unload(prov);
+ OSSL_LIB_CTX_free(*libctx);
+ *libctx = NULL;
+ return ok;
}
static int test_builtin_provider(void)
{
+ OSSL_LIB_CTX *libctx = OSSL_LIB_CTX_new();
const char *name = "p_test_builtin";
+ int ok;
- return
- TEST_true(OSSL_PROVIDER_add_builtin(NULL, name,
- PROVIDER_INIT_FUNCTION_NAME))
- && test_provider(name);
+ ok =
+ TEST_ptr(libctx)
+ && TEST_true(OSSL_PROVIDER_add_builtin(libctx, name,
+ PROVIDER_INIT_FUNCTION_NAME))
+ && test_provider(&libctx, name);
+
+ OSSL_LIB_CTX_free(libctx);
+
+ return ok;
}
#ifndef NO_PROVIDER_MODULE
static int test_loaded_provider(void)
{
+ OSSL_LIB_CTX *libctx = OSSL_LIB_CTX_new();
const char *name = "p_test";
- return test_provider(name);
+ if (!TEST_ptr(libctx))
+ return 0;
+
+ /* test_provider will free libctx as part of the test */
+ return test_provider(&libctx, name);
}
#endif
More information about the openssl-commits
mailing list