Commit 27c76b9b authored by Matt Caswell's avatar Matt Caswell
Browse files

Fix race condition in NewSessionTicket 

If a NewSessionTicket is received by a multi-threaded client when
attempting to reuse a previous ticket then a race condition can occur
potentially leading to a double free of the ticket data.

CVE-2015-1791

This also fixes RT#3808 where a session ID is changed for a session already
in the client session cache. Since the session ID is the key to the cache
this breaks the cache access.

Parts of this patch were inspired by this Akamai change:
https://github.com/akamai/openssl/commit/c0bf69a791239ceec64509f9f19fcafb2461b0d3



Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
parent 8744ba5e

ssl/s3_clnt.c

+32 −0
Original line number Diff line number Diff line
@@ -2229,6 +2229,38 @@ int ssl3_get_new_session_ticket(SSL *s) 
    }



    p = d = (unsigned char *)s->init_msg;



    if (s->session->session_id_length > 0) {

        int i = s->session_ctx->session_cache_mode;

        SSL_SESSION *new_sess;

        /*

         * We reused an existing session, so we need to replace it with a new

         * one

         */

        if (i & SSL_SESS_CACHE_CLIENT) {

            /*

             * Remove the old session from the cache

             */

            if (i & SSL_SESS_CACHE_NO_INTERNAL_STORE) {

                if (s->session_ctx->remove_session_cb != NULL)

                    s->session_ctx->remove_session_cb(s->session_ctx,

                                                      s->session);

            } else {

                /* We carry on if this fails */

                SSL_CTX_remove_session(s->session_ctx, s->session);

            }

        }



        if ((new_sess = ssl_session_dup(s->session, 0)) == 0) {

            al = SSL_AD_INTERNAL_ERROR;

            SSLerr(SSL_F_SSL3_GET_NEW_SESSION_TICKET, ERR_R_MALLOC_FAILURE);

            goto f_err;

        }



        SSL_SESSION_free(s->session);

        s->session = new_sess;

    }



    n2l(p, s->session->tlsext_tick_lifetime_hint);

    n2s(p, ticklen);

    /* ticket_lifetime_hint + ticket_length + ticket */

ssl/ssl.h

+1 −0
Original line number Diff line number Diff line
@@ -2787,6 +2787,7 @@ void ERR_load_SSL_strings(void); 
# define SSL_F_SSL_RSA_PUBLIC_ENCRYPT                     188

# define SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT                320

# define SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT                321

# define SSL_F_SSL_SESSION_DUP                            348

# define SSL_F_SSL_SESSION_NEW                            189

# define SSL_F_SSL_SESSION_PRINT_FP                       190

# define SSL_F_SSL_SESSION_SET1_ID_CONTEXT                312

ssl/ssl_err.c

+1 −0
Original line number Diff line number Diff line
@@ -312,6 +312,7 @@ static ERR_STRING_DATA SSL_str_functs[] = { 
     "SSL_SCAN_CLIENTHELLO_TLSEXT"},

    {ERR_FUNC(SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT),

     "SSL_SCAN_SERVERHELLO_TLSEXT"},

    {ERR_FUNC(SSL_F_SSL_SESSION_DUP), "ssl_session_dup"},

    {ERR_FUNC(SSL_F_SSL_SESSION_NEW), "SSL_SESSION_new"},

    {ERR_FUNC(SSL_F_SSL_SESSION_PRINT_FP), "SSL_SESSION_print_fp"},

    {ERR_FUNC(SSL_F_SSL_SESSION_SET1_ID_CONTEXT),

ssl/ssl_locl.h

+1 −0
Original line number Diff line number Diff line
@@ -1058,6 +1058,7 @@ int ssl_set_peer_cert_type(SESS_CERT *c, int type); 
int ssl_get_new_session(SSL *s, int session);

int ssl_get_prev_session(SSL *s, unsigned char *session, int len,

                         const unsigned char *limit);

SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket);

int ssl_cipher_id_cmp(const SSL_CIPHER *a, const SSL_CIPHER *b);

DECLARE_OBJ_BSEARCH_GLOBAL_CMP_FN(SSL_CIPHER, SSL_CIPHER, ssl_cipher_id);

int ssl_cipher_ptr_id_cmp(const SSL_CIPHER *const *ap,

ssl/ssl_sess.c

+123 −0
Original line number Diff line number Diff line
@@ -227,6 +227,129 @@ SSL_SESSION *SSL_SESSION_new(void) 
    return (ss);

}



/*

 * Create a new SSL_SESSION and duplicate the contents of |src| into it. If

 * ticket == 0 then no ticket information is duplicated, otherwise it is.

 */

SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket)

{

    SSL_SESSION *dest;



    dest = OPENSSL_malloc(sizeof(*src));

    if (dest == NULL) {

        goto err;

    }

    memcpy(dest, src, sizeof(*dest));



#ifndef OPENSSL_NO_KRB5

    dest->krb5_client_princ_len = dest->krb5_client_princ_len;

    if (src->krb5_client_princ_len > 0)

        memcpy(dest->krb5_client_princ, src->krb5_client_princ,

               src->krb5_client_princ_len);

#endif



#ifndef OPENSSL_NO_PSK

    if (src->psk_identity_hint) {

        dest->psk_identity_hint = BUF_strdup(src->psk_identity_hint);

        if (dest->psk_identity_hint == NULL) {

            goto err;

        }

    } else {

        dest->psk_identity_hint = NULL;

    }

    if (src->psk_identity) {

        dest->psk_identity = BUF_strdup(src->psk_identity);

        if (dest->psk_identity == NULL) {

            goto err;

        }

    } else {

        dest->psk_identity = NULL;

    }

#endif



    if (src->sess_cert != NULL)

        CRYPTO_add(&src->sess_cert->references, 1, CRYPTO_LOCK_SSL_SESS_CERT);



    if (src->peer != NULL)

        CRYPTO_add(&src->peer->references, 1, CRYPTO_LOCK_X509);



    dest->references = 1;



    if(src->ciphers != NULL) {

        dest->ciphers = sk_SSL_CIPHER_dup(src->ciphers);

        if (dest->ciphers == NULL)

            goto err;

    } else {

        dest->ciphers = NULL;

    }



    if (!CRYPTO_dup_ex_data(CRYPTO_EX_INDEX_SSL_SESSION,

                                            &dest->ex_data, &src->ex_data)) {

        goto err;

    }



    /* We deliberately don't copy the prev and next pointers */

    dest->prev = NULL;

    dest->next = NULL;



#ifndef OPENSSL_NO_TLSEXT

    if (src->tlsext_hostname) {

        dest->tlsext_hostname = BUF_strdup(src->tlsext_hostname);

        if (dest->tlsext_hostname == NULL) {

            goto err;

        }

    } else {

        dest->tlsext_hostname = NULL;

    }

# ifndef OPENSSL_NO_EC

    if (src->tlsext_ecpointformatlist) {

        dest->tlsext_ecpointformatlist =

            BUF_memdup(src->tlsext_ecpointformatlist,

                       src->tlsext_ecpointformatlist_length);

        if (dest->tlsext_ecpointformatlist == NULL)

            goto err;

        dest->tlsext_ecpointformatlist_length =

            src->tlsext_ecpointformatlist_length;

    }

    if (src->tlsext_ellipticcurvelist) {

        dest->tlsext_ellipticcurvelist =

            BUF_memdup(src->tlsext_ellipticcurvelist,

                       src->tlsext_ellipticcurvelist_length);

        if (dest->tlsext_ellipticcurvelist == NULL)

            goto err;

        dest->tlsext_ellipticcurvelist_length =

            src->tlsext_ellipticcurvelist_length;

    }

# endif

#endif



    if (ticket != 0) {

        dest->tlsext_tick_lifetime_hint = src->tlsext_tick_lifetime_hint;

        dest->tlsext_ticklen = src->tlsext_ticklen;

        if((dest->tlsext_tick = OPENSSL_malloc(src->tlsext_ticklen)) == NULL) {

            goto err;

        }

    }



#ifndef OPENSSL_NO_SRP

    dest->srp_username = NULL;

    if (src->srp_username) {

        dest->srp_username = BUF_strdup(src->srp_username);

        if (dest->srp_username == NULL) {

            goto err;

        }

    } else {

        dest->srp_username = NULL;

    }

#endif



    return dest;

err:

    SSLerr(SSL_F_SSL_SESSION_DUP, ERR_R_MALLOC_FAILURE);

    SSL_SESSION_free(dest);

    return NULL;

}



const unsigned char *SSL_SESSION_get_id(const SSL_SESSION *s,

                                        unsigned int *len)

{