commit 32305f88509c1d9ccb3ad676209a25fa59b95488
author Matt Caswell <matt@openssl.org> Thu Mar 15 17:47:29 2018 +0000
committer Matt Caswell <matt@openssl.org> Mon Mar 19 12:21:17 2018 +0000
tree 465d5906e932f6f930061a32e60c003b89f2b1a9
parent 51cf8ba038aae10df9895b0001715938f7ad0c75

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

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 1873237..5e44d4c 100644
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -417,7 +417,13 @@
     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;

ssl/statem/statem_srvr.c

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 50be825..5542a78 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -3691,6 +3691,10 @@
     } 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 @@
                  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) {

ssl/t1_lib.c

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 596fdd4..796e9d4 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1409,7 +1409,7 @@
     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 @@
          * 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;

test/sslapitest.c

diff --git a/test/sslapitest.c b/test/sslapitest.c
index 8e91151..29eb2f8 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -1021,15 +1021,20 @@
         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;
         }
     }