From 32305f88509c1d9ccb3ad676209a25fa59b95488 Mon Sep 17 00:00:00 2001
From: Matt Caswell <matt@openssl.org>
Date: Thu, 15 Mar 2018 17:47:29 +0000
Subject: [PATCH] Always call the new_session_cb when issuing a
 NewSessionTicket in TLSv1.3

Conceptually in TLSv1.3 there can be multiple sessions associated with a
single connection. Each NewSessionTicket issued can be considered a
separate session. We can end up issuing multiple NewSessionTickets on a
single connection at the moment (e.g. in a post-handshake auth scenario).
Each of those issued tickets should have the new_session_cb called, it
should go into the session cache separately and it should have a unique
id associated with it (so that they can be found individually in the
cache).

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5644)
---
 ssl/ssl_sess.c           |  8 +++++++-
 ssl/statem/statem_srvr.c |  5 ++++-
 ssl/t1_lib.c             |  7 ++++---
 test/sslapitest.c        | 13 +++++++++----
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 1873237c70..5e44d4c41f 100644
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -417,7 +417,13 @@ int ssl_get_new_session(SSL *s, int session)
     s->session = NULL;
 
     if (session) {
-        if (!ssl_generate_session_id(s, ss)) {
+        if (SSL_IS_TLS13(s)) {
+            /*
+             * We generate the session id while constructing the
+             * NewSessionTicket in TLSv1.3.
+             */
+            ss->session_id_length = 0;
+        } else if (!ssl_generate_session_id(s, ss)) {
             /* SSLfatal() already called */
             SSL_SESSION_free(ss);
             return 0;
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 50be8253c5..5542a78e21 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -3691,6 +3691,10 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
     } age_add_u;
 
     if (SSL_IS_TLS13(s)) {
+        if (!ssl_generate_session_id(s, s->session)) {
+            /* SSLfatal() already called */
+            goto err;
+        }
         if (ssl_randbytes(s, age_add_u.age_add_c, sizeof(age_add_u)) <= 0) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR,
                      SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET,
@@ -3776,7 +3780,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
                  SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, ERR_R_INTERNAL_ERROR);
         goto err;
     }
-    sess->session_id_length = 0; /* ID is irrelevant for the ticket */
 
     slen = i2d_SSL_SESSION(sess, NULL);
     if (slen == 0 || slen > slen_full) {
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 596fdd4c34..796e9d4827 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1409,7 +1409,7 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
     OPENSSL_free(sdec);
     if (sess) {
         /* Some additional consistency checks */
-        if (slen != 0 || sess->session_id_length != 0) {
+        if (slen != 0) {
             SSL_SESSION_free(sess);
             return SSL_TICKET_NO_DECRYPT;
         }
@@ -1419,9 +1419,10 @@ SSL_TICKET_RETURN tls_decrypt_ticket(SSL *s, const unsigned char *etick,
          * structure. If it is empty set length to zero as required by
          * standard.
          */
-        if (sesslen)
+        if (sesslen) {
             memcpy(sess->session_id, sess_id, sesslen);
-        sess->session_id_length = sesslen;
+            sess->session_id_length = sesslen;
+        }
         *psess = sess;
         if (renew_ticket)
             return SSL_TICKET_SUCCESS_RENEW;
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 8e91151780..29eb2f8260 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -1021,15 +1021,20 @@ static int execute_test_session(int maxprot, int use_int_cache,
         goto end;
 
     if (use_ext_cache) {
-        if (!TEST_int_eq(new_called, 0)
-                || !TEST_int_eq(remove_called, 0))
+        if (!TEST_int_eq(remove_called, 0))
             goto end;
 
         if (maxprot == TLS1_3_VERSION) {
-            if (!TEST_int_eq(get_called, 0))
+            /*
+             * Every time we issue a NewSessionTicket we are creating a new
+             * session for next time in TLSv1.3
+             */
+            if (!TEST_int_eq(new_called, 1)
+                    || !TEST_int_eq(get_called, 0))
                 goto end;
         } else {
-            if (!TEST_int_eq(get_called, 1))
+            if (!TEST_int_eq(new_called, 0)
+                    || !TEST_int_eq(get_called, 1))
                 goto end;
         }
     }
-- 
GitLab