Fix new_session_cb calls in TLSv1.3

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)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index ab8e443bece713151c363047e19a30ba21a9893f..491023f835cd28ce28aa71d18b04969ff036db00 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3156,10 +3156,11 @@ void ssl_update_cache(SSL *s, int 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))
-        && (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);

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 1e96022e62bbf542704e62595c510c545ea9db61..5f6c6b0382e6d1627ee3bc2bfd4dfe28237afb78 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -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
          */
+        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
@@ -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);
         }
 
-        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;
     }

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index a6baf2a5c9aed2888ca2e7a51044860bd07f37cd..08d5f416f38452be48353de8bd4af3fed4149b61 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -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 {
-            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++;