From 8ab4fed9bdcc5b8498b3d59d2fa72dd045f63539 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Wed, 26 Apr 2017 14:05:49 -0400 Subject: [PATCH] Fix ex_data and session_dup issues Code was added in commit b3c31a65 that overwrote the last ex_data value using CRYPTO_dup_ex_data() causing a memory leak, and potentially confusing the ex_data dup() callback. In ssl_session_dup(), fix error handling (properly reference and up-ref shared data) and new-up the ex_data before calling CRYPTO_dup_ex_data(); all other structures that dup ex_data have the destination ex_data new'd before the dup. Fix up some of the ex_data documentation. Reviewed-by: Matt Caswell Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3625) --- crypto/ex_data.c | 9 +- doc/crypto/CRYPTO_get_ex_new_index.pod | 7 +- ssl/ssl_sess.c | 14 ++- test/exdatatest.c | 131 ++++++++++++++++++++++--- 4 files changed, 143 insertions(+), 18 deletions(-) diff --git a/crypto/ex_data.c b/crypto/ex_data.c index 4a3201a953..22c4d3d9b9 100644 --- a/crypto/ex_data.c +++ b/crypto/ex_data.c @@ -287,7 +287,14 @@ int CRYPTO_dup_ex_data(int class_index, CRYPTO_EX_DATA *to, CRYPTOerr(CRYPTO_F_CRYPTO_DUP_EX_DATA, ERR_R_MALLOC_FAILURE); return 0; } - if (!CRYPTO_set_ex_data(to, mx - 1, NULL)) + /* + * Make sure the ex_data stack is at least |mx| elements long to avoid + * issues in the for loop that follows; so go get the |mx|'th element + * (if it does not exist CRYPTO_get_ex_data() returns NULL), and assign + * to itself. This is normally a no-op; but ensures the stack is the + * proper size + */ + if (!CRYPTO_set_ex_data(to, mx - 1, CRYPTO_get_ex_data(to, mx - 1))) goto err; for (i = 0; i < mx; i++) { diff --git a/doc/crypto/CRYPTO_get_ex_new_index.pod b/doc/crypto/CRYPTO_get_ex_new_index.pod index 0853ce588c..a5bf620972 100644 --- a/doc/crypto/CRYPTO_get_ex_new_index.pod +++ b/doc/crypto/CRYPTO_get_ex_new_index.pod @@ -17,8 +17,8 @@ CRYPTO_get_ex_data, CRYPTO_free_ex_data, CRYPTO_new_ex_data CRYPTO_EX_dup *dup_func, CRYPTO_EX_free *free_func); - typedef int CRYPTO_EX_new(void *parent, void *ptr, CRYPTO_EX_DATA *ad, - int idx, long argl, void *argp); + typedef void CRYPTO_EX_new(void *parent, void *ptr, CRYPTO_EX_DATA *ad, + int idx, long argl, void *argp); 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, @@ -128,7 +128,8 @@ initially registered via CRYPTO_get_ex_new_index() and can be used if 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 +for B, B, B objects and B chains via +BIO_dup_chain(). The B and B parameters are pointers to the destination and source B structures, 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 diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 3f068840b9..92ba599566 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -138,6 +138,8 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) #ifndef OPENSSL_NO_SRP dest->srp_username = NULL; #endif + dest->peer_chain = NULL; + dest->peer = NULL; memset(&dest->ex_data, 0, sizeof(dest->ex_data)); /* We deliberately don't copy the prev and next pointers */ @@ -150,8 +152,14 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) if (dest->lock == NULL) goto err; - if (src->peer != NULL) - X509_up_ref(src->peer); + if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_SSL_SESSION, dest, &dest->ex_data)) + goto err; + + if (src->peer != NULL) { + if (!X509_up_ref(src->peer)) + goto err; + dest->peer = src->peer; + } if (src->peer_chain != NULL) { dest->peer_chain = X509_chain_up_ref(src->peer_chain); @@ -207,7 +215,7 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) } #endif - if (ticket != 0) { + if (ticket != 0 && src->tlsext_tick != NULL) { dest->tlsext_tick = OPENSSL_memdup(src->tlsext_tick, src->tlsext_ticklen); if (dest->tlsext_tick == NULL) diff --git a/test/exdatatest.c b/test/exdatatest.c index e0eadd32dd..7998622de8 100644 --- a/test/exdatatest.c +++ b/test/exdatatest.c @@ -15,6 +15,12 @@ static long saved_argl; static void *saved_argp; static int saved_idx; +static int saved_idx2; + +/* + * SIMPLE EX_DATA IMPLEMENTATION + * Apps explicitly set/get ex_data as needed + */ static void exnew(void *parent, void *ptr, CRYPTO_EX_DATA *ad, int idx, long argl, void *argp) @@ -43,6 +49,75 @@ static void exfree(void *parent, void *ptr, CRYPTO_EX_DATA *ad, OPENSSL_assert(argp == saved_argp); } +/* + * PRE-ALLOCATED EX_DATA IMPLEMENTATION + * Extended data structure is allocated in exnew2/freed in exfree2 + * Data is stored inside extended data structure + */ + +typedef struct myobj_ex_data_st { + char *hello; + int new; + int dup; +} MYOBJ_EX_DATA; + +static void exnew2(void *parent, void *ptr, CRYPTO_EX_DATA *ad, + int idx, long argl, void *argp) +{ + int ret; + MYOBJ_EX_DATA *ex_data; + + OPENSSL_assert(idx == saved_idx2); + OPENSSL_assert(argl == saved_argl); + OPENSSL_assert(argp == saved_argp); + OPENSSL_assert(ptr == NULL); + + ex_data = OPENSSL_zalloc(sizeof(*ex_data)); + OPENSSL_assert(ex_data != NULL); + ret = CRYPTO_set_ex_data(ad, saved_idx2, ex_data); + OPENSSL_assert(ret); + + ex_data->new = 1; +} + +static int exdup2(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, + void *from_d, int idx, long argl, void *argp) +{ + MYOBJ_EX_DATA **update_ex_data = (MYOBJ_EX_DATA**)from_d; + MYOBJ_EX_DATA *ex_data = CRYPTO_get_ex_data(to, saved_idx2); + + OPENSSL_assert(idx == saved_idx2); + OPENSSL_assert(argl == saved_argl); + OPENSSL_assert(argp == saved_argp); + OPENSSL_assert(from_d != NULL); + OPENSSL_assert(*update_ex_data != NULL); + OPENSSL_assert(ex_data != NULL); + OPENSSL_assert(ex_data->new); + + /* Copy hello over */ + ex_data->hello = (*update_ex_data)->hello; + /* indicate this is a dup */ + ex_data->dup = 1; + /* Keep my original ex_data */ + *update_ex_data = ex_data; + return 1; +} + +static void exfree2(void *parent, void *ptr, CRYPTO_EX_DATA *ad, + int idx, long argl, void *argp) +{ + MYOBJ_EX_DATA *ex_data = CRYPTO_get_ex_data(ad, saved_idx2); + int ret; + + OPENSSL_assert(ex_data != NULL); + OPENSSL_free(ex_data); + OPENSSL_assert(idx == saved_idx2); + OPENSSL_assert(argl == saved_argl); + OPENSSL_assert(argp == saved_argp); + ret = CRYPTO_set_ex_data(ad, saved_idx2, NULL); + OPENSSL_assert(ret); +} + typedef struct myobj_st { CRYPTO_EX_DATA ex_data; int id; @@ -71,6 +146,25 @@ static char *MYOBJ_gethello(MYOBJ *obj) return CRYPTO_get_ex_data(&obj->ex_data, saved_idx); } +static void MYOBJ_sethello2(MYOBJ *obj, char *cp) +{ + MYOBJ_EX_DATA* ex_data = CRYPTO_get_ex_data(&obj->ex_data, saved_idx2); + if (ex_data != NULL) + ex_data->hello = cp; + else + obj->st = 0; +} + +static char *MYOBJ_gethello2(MYOBJ *obj) +{ + MYOBJ_EX_DATA* ex_data = CRYPTO_get_ex_data(&obj->ex_data, saved_idx2); + if (ex_data != NULL) + return ex_data->hello; + + obj->st = 0; + return NULL; +} + static void MYOBJ_free(MYOBJ *obj) { CRYPTO_free_ex_data(CRYPTO_EX_INDEX_APP, obj, &obj->ex_data); @@ -90,36 +184,51 @@ static MYOBJ *MYOBJ_dup(MYOBJ *in) int main() { MYOBJ *t1, *t2, *t3; + MYOBJ_EX_DATA *ex_data; const char *cp; char *p; - p = strdup("hello world"); + p = OPENSSL_strdup("hello world"); saved_argl = 21; - saved_argp = malloc(1); + saved_argp = OPENSSL_malloc(1); saved_idx = CRYPTO_get_ex_new_index(CRYPTO_EX_INDEX_APP, saved_argl, saved_argp, exnew, exdup, exfree); + saved_idx2 = CRYPTO_get_ex_new_index(CRYPTO_EX_INDEX_APP, + saved_argl, saved_argp, + exnew2, exdup2, exfree2); t1 = MYOBJ_new(); t2 = MYOBJ_new(); + OPENSSL_assert(t1->st && t2->st); + ex_data = CRYPTO_get_ex_data(&t1->ex_data, saved_idx2); + OPENSSL_assert(ex_data != NULL); + ex_data = CRYPTO_get_ex_data(&t2->ex_data, saved_idx2); + OPENSSL_assert(ex_data != NULL); MYOBJ_sethello(t1, p); cp = MYOBJ_gethello(t1); OPENSSL_assert(cp == p); - if (cp != p) - return 1; cp = MYOBJ_gethello(t2); OPENSSL_assert(cp == NULL); - if (cp != NULL) - return 1; + MYOBJ_sethello2(t1, p); + cp = MYOBJ_gethello2(t1); + OPENSSL_assert(cp == p); + OPENSSL_assert(t1->st); + cp = MYOBJ_gethello2(t2); + OPENSSL_assert(cp == NULL); + OPENSSL_assert(t2->st); t3 = MYOBJ_dup(t1); + ex_data = CRYPTO_get_ex_data(&t3->ex_data, saved_idx2); + OPENSSL_assert(ex_data != NULL); + OPENSSL_assert(ex_data->dup); cp = MYOBJ_gethello(t3); OPENSSL_assert(cp == p); - if (cp != p) - return 1; - cp = MYOBJ_gethello(t2); + cp = MYOBJ_gethello2(t3); + OPENSSL_assert(cp == p); + OPENSSL_assert(t3->st); MYOBJ_free(t1); MYOBJ_free(t2); MYOBJ_free(t3); - free(saved_argp); - free(p); + OPENSSL_free(saved_argp); + OPENSSL_free(p); return 0; } -- 2.25.1