CVE-2015-1791: Concurrent Execution using Shared Resource with Improper Synchronization ('Race Condition') (CWE-362)
value: Race condition in the ssl3_get_new_session_ticket function in ssl/s3_clnt.c in OpenSSL before 0.9.8zg, 1.0.0 before 1.0.0s, 1.0.1 before 1.0.1n, and 1.0.2 before 1.0.2b, when used for a multi-threaded client, allows remote attackers to cause a denial of service (double free and application crash) or possibly have unspecified other impact by providing a NewSessionTicket during an attempt to reuse a ticket that had been obtained earlier.
CWE description: The product contains a code sequence that can run concurrently with other code, and the code sequence requires temporary, exclusive access to a shared resource, but a timing window exists in which the shared resource can be modified by another code sequence that is operating concurrently.
Published: 2015-06-12T19:59Z
Repository: https://github.com/openssl/openssl
Language: C
Files changed: include/openssl/ssl.h, ssl/s3_clnt.c, ssl/ssl_err.c, ssl/ssl_locl.h, ssl/ssl_sess.c
Commit date: 2015-06-02 09:30:12 +0100
Commit message: 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>

Diff: diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 3e2dac6746..4e18b65605 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2046,10 +2046,11 @@ void ERR_load_SSL_strings(void);
 # define SSL_F_SSL_PREPARE_CLIENTHELLO_TLSEXT             281
 # define SSL_F_SSL_PREPARE_SERVERHELLO_TLSEXT             282
 # define SSL_F_SSL_READ                                   223
 # 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
 # define SSL_F_SSL_SESS_CERT_NEW                          225
 # define SSL_F_SSL_SET_CERT                               191
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index f70dce4b0d..d6f53b0dea 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -2236,10 +2236,42 @@ int ssl3_get_new_session_ticket(SSL *s)
         SSLerr(SSL_F_SSL3_GET_NEW_SESSION_TICKET, SSL_R_LENGTH_MISMATCH);
         goto f_err;
     }
 
     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 */
     if (ticklen + 6 != n) {
         al = SSL_AD_DECODE_ERROR;
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 86f8fa891a..4b4d89ce7a 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -272,10 +272,11 @@ static ERR_STRING_DATA SSL_str_functs[] = {
     {ERR_FUNC(SSL_F_SSL_READ), "SSL_read"},
     {ERR_FUNC(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT),
      "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_SESSION_set1_id_context"},
     {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 9d1f80ab8f..3252631e1c 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1858,10 +1858,11 @@ __owur SESS_CERT *ssl_sess_cert_new(void);
 void ssl_sess_cert_free(SESS_CERT *sc);
 __owur int ssl_set_peer_cert_type(SESS_CERT *c, int type);
 __owur int ssl_get_new_session(SSL *s, int session);
 __owur int ssl_get_prev_session(SSL *s, unsigned char *session, int len,
                          const unsigned char *limit);
+__owur SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket);
 __owur 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);
 __owur int ssl_cipher_ptr_id_cmp(const SSL_CIPHER *const *ap,
                           const SSL_CIPHER *const *bp);
 __owur STACK_OF(SSL_CIPHER) *ssl_create_cipher_list(const SSL_METHOD *meth,
diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index f1c209508c..fd940541d5 100644
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -223,10 +223,126 @@ SSL_SESSION *SSL_SESSION_new(void)
     ss->srp_username = NULL;
 #endif
     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_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)
 {
     if (len)
         *len = s->session_id_length;

Fixed code: # define SSL_F_SSL_SESSION_DUP                            348

    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;
    }

    {ERR_FUNC(SSL_F_SSL_SESSION_DUP), "ssl_session_dup"},
__owur SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket);
/*
 * 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_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;
}