Commit 2813852d authored by Matt Caswell's avatar Matt Caswell
Browse files

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: default avatarTomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9176)
parent 2459dc1b

ssl/s3_lib.c

+2 −2
Original line number Diff line number Diff line
@@ -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;

ssl/ssl_lib.c

+1 −0
Original line number Diff line number Diff line
@@ -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

ssl/ssl_locl.h

+8 −5
Original line number Diff line number Diff line
@@ -566,8 +566,6 @@ 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 */

        unsigned char *tick; /* Session ticket */

        size_t ticklen;      /* Session ticket length */
@@ -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

ssl/ssl_sess.c

+0 −12
Original line number Diff line number Diff line
@@ -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);

ssl/statem/extensions_srvr.c

+5 −5
Original line number Diff line number Diff line
@@ -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);