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: Rich Salz <rsalz@openssl.org>
(cherry picked from commit 27c76b9b8010b536687318739c6f631ce4194688)

Conflicts:
	ssl/ssl.h
	ssl/ssl_err.c

diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index ce7269ac5d4cf732aed2fa24b0dafc6923d74673..f9063898a7527e2c5213766b5320fd26cd25a7ea 100644 (file)
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1855,6 +1855,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 */

diff --git a/ssl/ssl.h b/ssl/ssl.h
index 3f4d0ac9ed53aaf6367f41ced87cd2f5ba488ca1..ca341b4eb715507f7124f1d3073fec035bb10f36 100644 (file)
--- a/ssl/ssl.h
+++ b/ssl/ssl.h
@@ -2144,6 +2144,7 @@ void ERR_load_SSL_strings(void);
 # define SSL_F_SSL_READ                                   223
 # define SSL_F_SSL_RSA_PRIVATE_DECRYPT                    187
 # define SSL_F_SSL_RSA_PUBLIC_ENCRYPT                     188
+# 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_SESS_CERT_NEW                          225

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index f2ef076b784c6a8be77a8e746bd09f6fc047af4e..967448f3ca5f8799cbadc5d2f125e52a5c1b34b3 100644 (file)
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -287,6 +287,7 @@ static ERR_STRING_DATA SSL_str_functs[] = {
     {ERR_FUNC(SSL_F_SSL_READ), "SSL_read"},
     {ERR_FUNC(SSL_F_SSL_RSA_PRIVATE_DECRYPT), "SSL_RSA_PRIVATE_DECRYPT"},
     {ERR_FUNC(SSL_F_SSL_RSA_PUBLIC_ENCRYPT), "SSL_RSA_PUBLIC_ENCRYPT"},
+    {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_SESS_CERT_NEW), "SSL_SESS_CERT_NEW"},

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 60ad58d2bbc9aaa7927988c6718d1b8584406e10..da2b19b07269f22f82c314adc333b4d1e7719f88 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -831,6 +831,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,

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index b9432fd450dbf41587aefd21d09f4b5150fba42b..98b9107b5af0b7aa08d0552c48199e97624b4fc9 100644 (file)
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -224,6 +224,117 @@ 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;
+        }
+    }
+
+    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)
 {