From: Todd Short Date: Wed, 26 Apr 2017 18:05:49 +0000 (-0400) Subject: Fix ex_data and session_dup issues X-Git-Tag: OpenSSL_1_1_1-pre1~1395 X-Git-Url: https://git.openssl.org/?p=openssl.git;a=commitdiff_plain;h=1ee2125922d3302e3ea738442bf2b051a445cac0;hp=01dfaa08b1960049f91485f2e5eec6c6bd03db39 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/3323) --- 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/man3/CRYPTO_get_ex_new_index.pod b/doc/man3/CRYPTO_get_ex_new_index.pod index 0853ce588c..a5bf620972 100644 --- a/doc/man3/CRYPTO_get_ex_new_index.pod +++ b/doc/man3/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 5bef168abd..b17fcdcb54 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -151,6 +151,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 */ @@ -163,8 +165,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); @@ -220,7 +228,7 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) } #endif - if (ticket != 0) { + if (ticket != 0 && src->ext.tick != NULL) { dest->ext.tick = OPENSSL_memdup(src->ext.tick, src->ext.ticklen); if (dest->ext.tick == NULL) diff --git a/test/exdatatest.c b/test/exdatatest.c index 8880f6f40f..9b88b1ad1e 100644 --- a/test/exdatatest.c +++ b/test/exdatatest.c @@ -17,8 +17,14 @@ static long saved_argl; static void *saved_argp; static int saved_idx; +static int saved_idx2; static int gbl_result; +/* + * 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) { @@ -49,6 +55,72 @@ static void exfree(void *parent, void *ptr, CRYPTO_EX_DATA *ad, gbl_result = 0; } +/* + * 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) +{ + MYOBJ_EX_DATA *ex_data = OPENSSL_zalloc(sizeof(*ex_data)); + if (!TEST_int_eq(idx, saved_idx2) + || !TEST_long_eq(argl, saved_argl) + || !TEST_ptr_eq(argp, saved_argp) + || !TEST_ptr_null(ptr) + || !TEST_ptr(ex_data) + || !TEST_true(CRYPTO_set_ex_data(ad, saved_idx2, ex_data))) { + gbl_result = 0; + OPENSSL_free(ex_data); + } else { + 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); + if (!TEST_int_eq(idx, saved_idx2) + || !TEST_long_eq(argl, saved_argl) + || !TEST_ptr_eq(argp, saved_argp) + || !TEST_ptr(from_d) + || !TEST_ptr(*update_ex_data) + || !TEST_ptr(ex_data) + || !TEST_true(ex_data->new)) { + gbl_result = 0; + } else { + /* 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); + OPENSSL_free(ex_data); + if (!TEST_int_eq(idx, saved_idx2) + || !TEST_long_eq(argl, saved_argl) + || !TEST_ptr_eq(argp, saved_argp) + || !TEST_ptr(ex_data) + || !TEST_true(CRYPTO_set_ex_data(ad, saved_idx2, NULL))) + gbl_result = 0; +} + typedef struct myobj_st { CRYPTO_EX_DATA ex_data; int id; @@ -77,6 +149,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 (TEST_ptr(ex_data)) + ex_data->hello = cp; + else + obj->st = gbl_result = 0; +} + +static char *MYOBJ_gethello2(MYOBJ *obj) +{ + MYOBJ_EX_DATA* ex_data = CRYPTO_get_ex_data(&obj->ex_data, saved_idx2); + if (TEST_ptr(ex_data)) + return ex_data->hello; + + obj->st = gbl_result = 0; + return NULL; +} + static void MYOBJ_free(MYOBJ *obj) { CRYPTO_free_ex_data(CRYPTO_EX_INDEX_APP, obj, &obj->ex_data); @@ -95,44 +186,71 @@ static MYOBJ *MYOBJ_dup(MYOBJ *in) static int test_exdata(void) { MYOBJ *t1, *t2, *t3; + MYOBJ_EX_DATA *ex_data; const char *cp; char *p; gbl_result = 1; - 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(); if (!TEST_int_eq(t1->st, 1) || !TEST_int_eq(t2->st, 1)) return 0; + if (!TEST_ptr(CRYPTO_get_ex_data(&t1->ex_data, saved_idx2))) + return 0; + if (!TEST_ptr(CRYPTO_get_ex_data(&t2->ex_data, saved_idx2))) + return 0; MYOBJ_sethello(t1, p); cp = MYOBJ_gethello(t1); if (!TEST_ptr_eq(cp, p)) return 0; + MYOBJ_sethello2(t1, p); + cp = MYOBJ_gethello2(t1); + if (!TEST_ptr_eq(cp, p)) + return 0; + cp = MYOBJ_gethello(t2); if (!TEST_ptr_null(cp)) return 0; + cp = MYOBJ_gethello2(t2); + if (!TEST_ptr_null(cp)) + return 0; + t3 = MYOBJ_dup(t1); if (!TEST_int_eq(t3->st, 1)) return 0; + ex_data = CRYPTO_get_ex_data(&t3->ex_data, saved_idx2); + if (!TEST_ptr(ex_data)) + return 0; + if (!TEST_int_eq(ex_data->dup, 1)) + return 0; + cp = MYOBJ_gethello(t3); if (!TEST_ptr_eq(cp, p)) return 0; + cp = MYOBJ_gethello2(t3); + if (!TEST_ptr_eq(cp, p)) + return 0; + MYOBJ_free(t1); MYOBJ_free(t2); MYOBJ_free(t3); - free(saved_argp); - free(p); + OPENSSL_free(saved_argp); + OPENSSL_free(p); if (gbl_result) return 1;