Commit 1ee21259 authored by Todd Short's avatar Todd Short Committed by Rich Salz
Browse files

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: default avatarMatt Caswell <matt@openssl.org>
Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3323)
parent 01dfaa08

crypto/ex_data.c

+8 −1
Original line number Diff line number Diff line
@@ -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++) {

doc/man3/CRYPTO_get_ex_new_index.pod

+4 −3
Original line number Diff line number Diff line
@@ -17,7 +17,7 @@ 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,

 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);
@@ -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<SSL> and B<SSL_SESSION> objects.  The B<to> and B<from> parameters

for B<SSL>, B<SSL_SESSION>, B<EC_KEY> objects and B<BIO> chains via

BIO_dup_chain().  The B<to> and B<from> parameters

are pointers to the destination and source B<CRYPTO_EX_DATA> structures,

respectively.  The B<from_d> parameter needs to be cast to a B<void **pptr>

as the API has currently the wrong signature; that will be changed in a

ssl/ssl_sess.c

+11 −3
Original line number Diff line number Diff line
@@ -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)

test/exdatatest.c

+122 −4
Original line number Diff line number Diff line
@@ -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;