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/9162)

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 6c3f04db423c27b6f21cf7b8fdb11adc78bc21e8..e51877a6b45c7ec706696eb10087b87575e1b79d 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3586,8 +3586,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 4e4477d2e1fc3fd17506efbb0e16788910d018b7..43a1099ba86248753a12294871751dd5a01daa89 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1188,6 +1188,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 e521152da384c1be18030b8891c66f9281dcb9a8..2f76b66352d93de5adb06f55c853e736d9858d5e 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -570,9 +570,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 */
@@ -1487,6 +1485,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 */
@@ -2258,8 +2261,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 508182a41bbd7b31c08cab8c616b42c4c75e37b0..9809fcc882290941b722e0e3b19ba8b430857aa3 100644 (file)
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -125,7 +125,6 @@ SSL_SESSION *ssl_session_dup(const 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(const 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 6181f3f26842b48d17cd88485b99291a68767f69..68fb0863c3b22c45e5a4006d51306ed87d27c556 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);