Fix a race condition in supported groups handling

In TLSv1.3 the supported groups can be negotiated each time a handshake
occurs, regardless of whether we are resuming or not. We should not store
the supported groups information in the session because session objects
can be shared between multiple threads and we can end up with race
conditions. For most users this won't be seen because, by default, we
use stateless tickets in TLSv1.3 which don't get shared. However if you
use SSL_OP_NO_TICKET (to get stateful tickets in TLSv1.3) then this can
happen.

The answer is to move the supported the supported group information into
the SSL object instead.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9176)

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 99ae48199c2d32eb91255a76e6b9b9cbc53c325c..dc148bc2b10411a9e7bd8026403bce620683b04f 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3601,8 +3601,8 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg)
 
             if (!s->session)
                 return 0;
-            clist = s->session->ext.supportedgroups;
-            clistlen = s->session->ext.supportedgroups_len;
+            clist = s->ext.peer_supportedgroups;
+            clistlen = s->ext.peer_supportedgroups_len;
             if (parg) {
                 size_t i;
                 int *cptr = parg;

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index f559bc10eff4ed9bdba71859d5c585204f3efb49..5584a1b08961b5fb93cfbfa6a7ec62215faa0ffd 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1179,6 +1179,7 @@ void SSL_free(SSL *s)
 #ifndef OPENSSL_NO_EC
     OPENSSL_free(s->ext.ecpointformats);
     OPENSSL_free(s->ext.supportedgroups);
+    OPENSSL_free(s->ext.peer_supportedgroups);
 #endif                          /* OPENSSL_NO_EC */
     sk_X509_EXTENSION_pop_free(s->ext.ocsp.exts, X509_EXTENSION_free);
 #ifndef OPENSSL_NO_OCSP

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 0cf3893e0648c59850d4173f94785ba2e727a681..48c7eb0e532beaa5f2ad89c23e8bf743fe2e6fa3 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -566,9 +566,7 @@ struct ssl_session_st {
         size_t ecpointformats_len;
         unsigned char *ecpointformats; /* peer's list */
 # endif                         /* OPENSSL_NO_EC */
-        size_t supportedgroups_len;
-        uint16_t *supportedgroups; /* peer's list */
-    /* RFC4507 info */
+        /* RFC4507 info */
         unsigned char *tick; /* Session ticket */
         size_t ticklen;      /* Session ticket length */
         /* Session lifetime hint in seconds */
@@ -1304,6 +1302,11 @@ struct ssl_st {
         size_t supportedgroups_len;
         /* our list */
         uint16_t *supportedgroups;
+
+        size_t peer_supportedgroups_len;
+         /* peer's list */
+        uint16_t *peer_supportedgroups;
+
         /* TLS Session Ticket extension override */
         TLS_SESSION_TICKET_EXT *session_ticket;
         /* TLS Session Ticket extension callback */
@@ -2240,8 +2243,8 @@ static ossl_inline int ssl_has_cert(const SSL *s, int idx)
 static ossl_inline void tls1_get_peer_groups(SSL *s, const uint16_t **pgroups,
                                              size_t *pgroupslen)
 {
-    *pgroups = s->session->ext.supportedgroups;
-    *pgroupslen = s->session->ext.supportedgroups_len;
+    *pgroups = s->ext.peer_supportedgroups;
+    *pgroupslen = s->ext.peer_supportedgroups_len;
 }
 
 # ifndef OPENSSL_UNIT_TEST

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 5ad2792a1b4c9087f684b4eaa748f8e6cdf0ea52..b7638780d0428f4d817164c95c8f680e9e0e81df 100644 (file)
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -125,7 +125,6 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket)
     dest->ext.hostname = NULL;
 #ifndef OPENSSL_NO_EC
     dest->ext.ecpointformats = NULL;
-    dest->ext.supportedgroups = NULL;
 #endif
     dest->ext.tick = NULL;
     dest->ext.alpn_selected = NULL;
@@ -201,14 +200,6 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket)
         if (dest->ext.ecpointformats == NULL)
             goto err;
     }
-    if (src->ext.supportedgroups) {
-        dest->ext.supportedgroups =
-            OPENSSL_memdup(src->ext.supportedgroups,
-                           src->ext.supportedgroups_len
-                                * sizeof(*src->ext.supportedgroups));
-        if (dest->ext.supportedgroups == NULL)
-            goto err;
-    }
 #endif
 
     if (ticket != 0 && src->ext.tick != NULL) {
@@ -797,9 +788,6 @@ void SSL_SESSION_free(SSL_SESSION *ss)
     OPENSSL_free(ss->ext.ecpointformats);
     ss->ext.ecpointformats = NULL;
     ss->ext.ecpointformats_len = 0;
-    OPENSSL_free(ss->ext.supportedgroups);
-    ss->ext.supportedgroups = NULL;
-    ss->ext.supportedgroups_len = 0;
 #endif                          /* OPENSSL_NO_EC */
 #ifndef OPENSSL_NO_PSK
     OPENSSL_free(ss->psk_identity_hint);

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index 6301b4e77caf6235bf09c75433791344c90bc378..5b83c267852d490b436203bbdd64a86a40ebe901 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -962,12 +962,12 @@ int tls_parse_ctos_supported_groups(SSL *s, PACKET *pkt, unsigned int context,
     }
 
     if (!s->hit || SSL_IS_TLS13(s)) {
-        OPENSSL_free(s->session->ext.supportedgroups);
-        s->session->ext.supportedgroups = NULL;
-        s->session->ext.supportedgroups_len = 0;
+        OPENSSL_free(s->ext.peer_supportedgroups);
+        s->ext.peer_supportedgroups = NULL;
+        s->ext.peer_supportedgroups_len = 0;
         if (!tls1_save_u16(&supported_groups_list,
-                           &s->session->ext.supportedgroups,
-                           &s->session->ext.supportedgroups_len)) {
+                           &s->ext.peer_supportedgroups,
+                           &s->ext.peer_supportedgroups_len)) {
             SSLfatal(s, SSL_AD_INTERNAL_ERROR,
                      SSL_F_TLS_PARSE_CTOS_SUPPORTED_GROUPS,
                      ERR_R_INTERNAL_ERROR);