Don't write the tick_identity to the session

Sessions must be immutable once they can be shared with multiple threads.
We were breaking that rule by writing the ticket index into it during the
handshake. This can lead to incorrect behaviour, including failed
connections in multi-threaded environments.

Reported by David Benjamin.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/8383)

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index ae6417b59257e98b2700413be39e0e38c853266c..a51cc6caa910bd1f55e1334a417f5282d4b18354 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -578,7 +578,6 @@ struct ssl_session_st {
         /* Session lifetime hint in seconds */
         unsigned long tick_lifetime_hint;
         uint32_t tick_age_add;
-        int tick_identity;
         /* Max number of bytes that can be sent as early data */
         uint32_t max_early_data;
         /* The ALPN protocol selected for this session */
@@ -1364,6 +1363,13 @@ struct ssl_st {
          * as this extension is optional on server side.
          */
         uint8_t max_fragment_len_mode;
+
+        /*
+         * On the client side the number of ticket identities we sent in the
+         * ClientHello. On the server side the identity of the ticket we
+         * selected.
+         */
+        int tick_identity;
     } ext;
 
     /*
@@ -2064,9 +2070,6 @@ typedef enum downgrade_en {
 #define TLSEXT_KEX_MODE_FLAG_KE                                 1
 #define TLSEXT_KEX_MODE_FLAG_KE_DHE                             2
 
-/* An invalid index into the TLSv1.3 PSK identities */
-#define TLSEXT_PSK_BAD_IDENTITY                                 -1
-
 #define SSL_USE_PSS(s) (s->s3->tmp.peer_sigalg != NULL && \
                         s->s3->tmp.peer_sigalg->sig == EVP_PKEY_RSA_PSS)
 

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 60d4da044230f924ef33e1de14ab04903fd60eb4..dcf2bfeadf247c57d9d4f1cf02ebef7a57ffd020 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -989,7 +989,6 @@ static int final_server_name(SSL *s, unsigned int context, int sent)
                 ss->ext.ticklen = 0;
                 ss->ext.tick_lifetime_hint = 0;
                 ss->ext.tick_age_add = 0;
-                ss->ext.tick_identity = 0;
                 if (!ssl_generate_session_id(s, ss)) {
                     SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_FINAL_SERVER_NAME,
                              ERR_R_INTERNAL_ERROR);
@@ -1645,7 +1644,6 @@ static int final_early_data(SSL *s, unsigned int context, int sent)
 
     if (s->max_early_data == 0
             || !s->hit
-            || s->session->ext.tick_identity != 0
             || s->early_data_state != SSL_EARLY_DATA_ACCEPTING
             || !s->ext.early_data_ok
             || s->hello_retry_request != SSL_HRR_NONE

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index f01e9eef64d9e11d4385de467626dcb0c1b43e2b..9d7a4f83041c674366744a1260d2ccae508b7973 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -996,7 +996,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
     const EVP_MD *handmd = NULL, *mdres = NULL, *mdpsk = NULL;
     int dores = 0;
 
-    s->session->ext.tick_identity = TLSEXT_PSK_BAD_IDENTITY;
+    s->ext.tick_identity = 0;
 
     /*
      * Note: At this stage of the code we only support adding a single
@@ -1086,6 +1086,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
         agems += s->session->ext.tick_age_add;
 
         reshashsize = EVP_MD_size(mdres);
+        s->ext.tick_identity++;
         dores = 1;
     }
 
@@ -1145,6 +1146,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
                      ERR_R_INTERNAL_ERROR);
             return EXT_RETURN_FAIL;
         }
+        s->ext.tick_identity++;
     }
 
     if (!WPACKET_close(pkt)
@@ -1183,11 +1185,6 @@ EXT_RETURN tls_construct_ctos_psk(SSL *s, WPACKET *pkt, unsigned int context,
         return EXT_RETURN_FAIL;
     }
 
-    if (dores)
-        s->session->ext.tick_identity = 0;
-    if (s->psksession != NULL)
-        s->psksession->ext.tick_identity = (dores ? 1 : 0);
-
     return EXT_RETURN_SENT;
 #else
     return EXT_RETURN_NOT_SENT;
@@ -1932,8 +1929,7 @@ int tls_parse_stoc_early_data(SSL *s, PACKET *pkt, unsigned int context,
     }
 
     if (!s->ext.early_data_ok
-            || !s->hit
-            || s->session->ext.tick_identity != 0) {
+            || !s->hit) {
         /*
          * If we get here then we didn't send early data, or we didn't resume
          * using the first identity, or the SNI/ALPN is not consistent so the
@@ -1961,17 +1957,28 @@ int tls_parse_stoc_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
         return 0;
     }
 
-    if (s->session->ext.tick_identity == (int)identity) {
+    if (identity >= (unsigned int)s->ext.tick_identity) {
+        SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_TLS_PARSE_STOC_PSK,
+                 SSL_R_BAD_PSK_IDENTITY);
+        return 0;
+    }
+
+    /*
+     * Session resumption tickets are always sent before PSK tickets. If the
+     * ticket index is 0 then it must be for a session resumption ticket if we
+     * sent two tickets, or if we didn't send a PSK ticket.
+     */
+    if (identity == 0 && (s->psksession == NULL || s->ext.tick_identity == 2)) {
         s->hit = 1;
         SSL_SESSION_free(s->psksession);
         s->psksession = NULL;
         return 1;
     }
 
-    if (s->psksession == NULL
-            || s->psksession->ext.tick_identity != (int)identity) {
-        SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_TLS_PARSE_STOC_PSK,
-                 SSL_R_BAD_PSK_IDENTITY);
+    if (s->psksession == NULL) {
+        /* Should never happen */
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PARSE_STOC_PSK,
+                 ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
@@ -1990,6 +1997,9 @@ int tls_parse_stoc_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
     s->session = s->psksession;
     s->psksession = NULL;
     s->hit = 1;
+    /* Early data is only allowed if we used the first ticket */
+    if (identity != 0)
+        s->ext.early_data_ok = 0;
 #endif
 
     return 1;

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 73ceadc34a9388d2e492e123a522fc5173aa5981..7b1c5c9eb0984fa945c77c1e3f7e39ec0929ca33 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -1277,7 +1277,7 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
         goto err;
     }
 
-    sess->ext.tick_identity = id;
+    s->ext.tick_identity = id;
 
     SSL_SESSION_free(s->session);
     s->session = sess;
@@ -1951,7 +1951,7 @@ EXT_RETURN tls_construct_stoc_psk(SSL *s, WPACKET *pkt, unsigned int context,
 
     if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_psk)
             || !WPACKET_start_sub_packet_u16(pkt)
-            || !WPACKET_put_bytes_u16(pkt, s->session->ext.tick_identity)
+            || !WPACKET_put_bytes_u16(pkt, s->ext.tick_identity)
             || !WPACKET_close(pkt)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR,
                  SSL_F_TLS_CONSTRUCT_STOC_PSK, ERR_R_INTERNAL_ERROR);

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 1f5511339a4615fae42c8d534528c6f724aa5afa..2a9b7377523f4cf6f8c01eda6df637e5f02c3ff1 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1613,10 +1613,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
          * so the PAC-based session secret is always preserved. It'll be
          * overwritten if the server refuses resumption.
          */
-        if (s->session->session_id_length > 0
-                || (SSL_IS_TLS13(s)
-                    && s->session->ext.tick_identity
-                       != TLSEXT_PSK_BAD_IDENTITY)) {
+        if (s->session->session_id_length > 0) {
             tsan_counter(&s->session_ctx->stats.sess_miss);
             if (!ssl_get_new_session(s, 0)) {
                 /* SSLfatal() already called */