From b3c31a6572bd7b89f469deb3c78f85f6e303df47 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Sun, 19 Mar 2017 16:14:33 +0100 Subject: [PATCH] Fix the error handling in CRYPTO_dup_ex_data. Fix a strict aliasing issue in ui_dup_method_data. Add test coverage for CRYPTO_dup_ex_data, use OPENSSL_assert. Reviewed-by: Rich Salz Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/2988) --- crypto/ex_data.c | 20 +++++++---- doc/man3/CRYPTO_get_ex_new_index.pod | 11 +++--- include/openssl/crypto.h | 2 +- test/exdatatest.c | 50 +++++++++++++++++++--------- 4 files changed, 56 insertions(+), 27 deletions(-) diff --git a/crypto/ex_data.c b/crypto/ex_data.c index 84b65551c6..4a3201a953 100644 --- a/crypto/ex_data.c +++ b/crypto/ex_data.c @@ -124,7 +124,7 @@ static int dummy_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, void *from_d, int idx, long argl, void *argp) { - return 0; + return 1; } int CRYPTO_free_ex_index(int class_index, int idx) @@ -254,10 +254,11 @@ int CRYPTO_dup_ex_data(int class_index, CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from) { int mx, j, i; - char *ptr; + void *ptr; EX_CALLBACK *stack[10]; EX_CALLBACK **storage = NULL; EX_CALLBACKS *ip; + int toret = 0; if (from->sk == NULL) /* Nothing to copy over */ @@ -280,21 +281,28 @@ int CRYPTO_dup_ex_data(int class_index, CRYPTO_EX_DATA *to, } CRYPTO_THREAD_unlock(ex_data_lock); - if (mx > 0 && storage == NULL) { + if (mx == 0) + return 1; + if (storage == NULL) { CRYPTOerr(CRYPTO_F_CRYPTO_DUP_EX_DATA, ERR_R_MALLOC_FAILURE); return 0; } + if (!CRYPTO_set_ex_data(to, mx - 1, NULL)) + goto err; for (i = 0; i < mx; i++) { ptr = CRYPTO_get_ex_data(from, i); if (storage[i] && storage[i]->dup_func) - storage[i]->dup_func(to, from, &ptr, i, - storage[i]->argl, storage[i]->argp); + if (!storage[i]->dup_func(to, from, &ptr, i, + storage[i]->argl, storage[i]->argp)) + goto err; CRYPTO_set_ex_data(to, i, ptr); } + toret = 1; + err: if (storage != stack) OPENSSL_free(storage); - return 1; + return toret; } diff --git a/doc/man3/CRYPTO_get_ex_new_index.pod b/doc/man3/CRYPTO_get_ex_new_index.pod index ede5fc14ce..0853ce588c 100644 --- a/doc/man3/CRYPTO_get_ex_new_index.pod +++ b/doc/man3/CRYPTO_get_ex_new_index.pod @@ -130,12 +130,15 @@ the same callback handles different types of exdata. dup_func() is called when a structure is being copied. This is only done for B and B objects. The B and B parameters are pointers to the destination and source B structures, -respectively. The B parameter is a pointer to the source exdata. -When the dup_func() returns, the value in B is copied to the -destination ex_data. If the pointer contained in B is not modified +respectively. The B parameter needs to be cast to a B +as the API has currently the wrong signature; that will be changed in a +future version. The B<*pptr> is a pointer to the source exdata. +When the dup_func() returns, the value in B<*pptr> is copied to the +destination ex_data. If the pointer contained in B<*pptr> is not modified by the dup_func(), then both B and B will point to the same data. The B, B and B parameters are as described for the other -two callbacks. +two callbacks. If the dup_func() returns B<0> the whole CRYPTO_dup_ex_data() +will fail. =head1 RETURN VALUES diff --git a/include/openssl/crypto.h b/include/openssl/crypto.h index 3b75dbe577..a8b8dc1696 100644 --- a/include/openssl/crypto.h +++ b/include/openssl/crypto.h @@ -175,7 +175,7 @@ typedef void CRYPTO_EX_new (void *parent, void *ptr, CRYPTO_EX_DATA *ad, typedef void CRYPTO_EX_free (void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp); typedef int CRYPTO_EX_dup (CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, - void *srcp, int idx, long argl, void *argp); + void *from_d, int idx, long argl, void *argp); __owur int CRYPTO_get_ex_new_index(int class_index, long argl, void *argp, CRYPTO_EX_new *new_func, CRYPTO_EX_dup *dup_func, CRYPTO_EX_free *free_func); diff --git a/test/exdatatest.c b/test/exdatatest.c index e82fdbfaea..e0eadd32dd 100644 --- a/test/exdatatest.c +++ b/test/exdatatest.c @@ -10,7 +10,6 @@ #include #include #include -#include #include static long saved_argl; @@ -20,26 +19,28 @@ static int saved_idx; static void exnew(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp) { - assert(idx == saved_idx); - assert(argl == saved_argl); - assert(argp == saved_argp); + OPENSSL_assert(idx == saved_idx); + OPENSSL_assert(argl == saved_argl); + OPENSSL_assert(argp == saved_argp); + OPENSSL_assert(ptr == NULL); } static int exdup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, void *from_d, int idx, long argl, void *argp) { - assert(idx == saved_idx); - assert(argl == saved_argl); - assert(argp == saved_argp); - return 0; + OPENSSL_assert(idx == saved_idx); + OPENSSL_assert(argl == saved_argl); + OPENSSL_assert(argp == saved_argp); + OPENSSL_assert(from_d != NULL); + return 1; } static void exfree(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp) { - assert(idx == saved_idx); - assert(argl == saved_argl); - assert(argp == saved_argp); + OPENSSL_assert(idx == saved_idx); + OPENSSL_assert(argl == saved_argl); + OPENSSL_assert(argp == saved_argp); } typedef struct myobj_st { @@ -55,14 +56,14 @@ static MYOBJ *MYOBJ_new() obj->id = ++count; obj->st = CRYPTO_new_ex_data(CRYPTO_EX_INDEX_APP, obj, &obj->ex_data); - assert(obj->st != 0); + OPENSSL_assert(obj->st != 0); return obj; } static void MYOBJ_sethello(MYOBJ *obj, char *cp) { obj->st = CRYPTO_set_ex_data(&obj->ex_data, saved_idx, cp); - assert(obj->st != 0); + OPENSSL_assert(obj->st != 0); } static char *MYOBJ_gethello(MYOBJ *obj) @@ -76,9 +77,19 @@ static void MYOBJ_free(MYOBJ *obj) OPENSSL_free(obj); } +static MYOBJ *MYOBJ_dup(MYOBJ *in) +{ + MYOBJ *obj = MYOBJ_new(); + + obj->st = CRYPTO_dup_ex_data(CRYPTO_EX_INDEX_APP, &obj->ex_data, + &in->ex_data); + OPENSSL_assert(obj->st != 0); + return obj; +} + int main() { - MYOBJ *t1, *t2; + MYOBJ *t1, *t2, *t3; const char *cp; char *p; @@ -92,15 +103,22 @@ int main() t2 = MYOBJ_new(); MYOBJ_sethello(t1, p); cp = MYOBJ_gethello(t1); - assert(cp == p); + OPENSSL_assert(cp == p); if (cp != p) return 1; cp = MYOBJ_gethello(t2); - assert(cp == NULL); + OPENSSL_assert(cp == NULL); if (cp != NULL) return 1; + t3 = MYOBJ_dup(t1); + cp = MYOBJ_gethello(t3); + OPENSSL_assert(cp == p); + if (cp != p) + return 1; + cp = MYOBJ_gethello(t2); MYOBJ_free(t1); MYOBJ_free(t2); + MYOBJ_free(t3); free(saved_argp); free(p); return 0; -- 2.34.1