Fix new_session_cb calls in TLSv1.3
authorMatt Caswell <matt@openssl.org>
Tue, 1 Aug 2017 09:49:47 +0000 (10:49 +0100)
committerMatt Caswell <matt@openssl.org>
Tue, 1 Aug 2017 12:09:31 +0000 (13:09 +0100)
If a new_session_cb is set then it was only ever getting invoked if !s->hit
is true. This is sensible for <=TLSv1.2 but does not work for TLSv1.3.

Fixes #4045

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4068)

ssl/ssl_lib.c
ssl/statem/statem_clnt.c
ssl/statem/statem_lib.c

index ab8e443..491023f 100644 (file)
@@ -3156,10 +3156,11 @@ void ssl_update_cache(SSL *s, int mode)
         return;
 
     i = s->session_ctx->session_cache_mode;
         return;
 
     i = s->session_ctx->session_cache_mode;
-    if ((i & mode) && (!s->hit)
-        && ((i & SSL_SESS_CACHE_NO_INTERNAL_STORE)
+    if ((i & mode) != 0
+        && (!s->hit || SSL_IS_TLS13(s))
+        && ((i & SSL_SESS_CACHE_NO_INTERNAL_STORE) != 0
             || SSL_CTX_add_session(s->session_ctx, s->session))
             || SSL_CTX_add_session(s->session_ctx, s->session))
-        && (s->session_ctx->new_session_cb != NULL)) {
+        && s->session_ctx->new_session_cb != NULL) {
         SSL_SESSION_up_ref(s->session);
         if (!s->session_ctx->new_session_cb(s, s->session))
             SSL_SESSION_free(s->session);
         SSL_SESSION_up_ref(s->session);
         if (!s->session_ctx->new_session_cb(s, s->session))
             SSL_SESSION_free(s->session);
index 1e96022..5f6c6b0 100644 (file)
@@ -2462,6 +2462,12 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
          * We reused an existing session, so we need to replace it with a new
          * one
          */
          * We reused an existing session, so we need to replace it with a new
          * one
          */
+        if ((new_sess = ssl_session_dup(s->session, 0)) == 0) {
+            al = SSL_AD_INTERNAL_ERROR;
+            SSLerr(SSL_F_TLS_PROCESS_NEW_SESSION_TICKET, ERR_R_MALLOC_FAILURE);
+            goto f_err;
+        }
+
         if (i & SSL_SESS_CACHE_CLIENT) {
             /*
              * Remove the old session from the cache. We carry on if this fails
         if (i & SSL_SESS_CACHE_CLIENT) {
             /*
              * Remove the old session from the cache. We carry on if this fails
@@ -2469,12 +2475,6 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
             SSL_CTX_remove_session(s->session_ctx, s->session);
         }
 
             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_TLS_PROCESS_NEW_SESSION_TICKET, ERR_R_MALLOC_FAILURE);
-            goto f_err;
-        }
-
         SSL_SESSION_free(s->session);
         s->session = new_sess;
     }
         SSL_SESSION_free(s->session);
         s->session = new_sess;
     }
index a6baf2a..08d5f41 100644 (file)
@@ -1028,7 +1028,12 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs)
             s->ctx->stats.sess_accept_good++;
             s->handshake_func = ossl_statem_accept;
         } else {
             s->ctx->stats.sess_accept_good++;
             s->handshake_func = ossl_statem_accept;
         } else {
-            ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
+            /*
+             * In TLSv1.3 we update the cache as part of processing the
+             * NewSessionTicket
+             */
+            if (!SSL_IS_TLS13(s))
+                ssl_update_cache(s, SSL_SESS_CACHE_CLIENT);
             if (s->hit)
                 s->ctx->stats.sess_hit++;
 
             if (s->hit)
                 s->ctx->stats.sess_hit++;