Promote SSL_get_negotiated_group() for non-TLSv1.3

It can be useful to know what group was used for the handshake's
key exchange process even on non-TLS 1.3 connections.  Allow this
API, new in OpenSSL 3.0.0, to be used on other TLS versions as well.
Since pre-TLS-1.3 key exchange occurs only on full handshakes, this
necessitates adding a field to the SSL_SESSION object to carry the
group information across resumptions.  The key exchange group in the
SSL_SESSION can also be relevant in TLS 1.3 when the resumption handshake
uses the "psk_ke" key-exchange mode, so also track whether a fresh key
exchange was done for TLS 1.3.

Since the new field is optional in the ASN.1 sense, there is no need
to increment SSL_SESSION_ASN1_VERSION (which incurs strong incompatibility
churn).

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14750)

diff --git a/doc/man3/SSL_CTX_set1_curves.pod b/doc/man3/SSL_CTX_set1_curves.pod
index 5eebb2b933c83a19ef00ee4d5e84c2ab80a0f188..65892e46a5eec61c89a3b8401e31be6f6593ccac 100644 (file)
--- a/doc/man3/SSL_CTX_set1_curves.pod
+++ b/doc/man3/SSL_CTX_set1_curves.pod
@@ -77,10 +77,15 @@ NID_undef is returned. If the NID for the shared group is unknown then the value
 is set to the bitwise OR of TLSEXT_nid_unknown (0x1000000) and the id of the
 group.
 
-SSL_get_negotiated_group() returns the NID of the negotiated group on a TLSv1.3
-connection for key exchange. This can be called by either client or server. If
-the NID for the shared group is unknown then the value is set to the bitwise OR
-of TLSEXT_nid_unknown (0x1000000) and the id of the group.
+SSL_get_negotiated_group() returns the NID of the negotiated group used for
+the handshake key exchange process.  For TLSv1.3 connections this typically
+reflects the state of the current connection, though in the case of PSK-only
+resumption, the returned value will be from a previous connection.  For earlier
+TLS versions, when a session has been resumed, it always reflects the group
+used for key exchange during the initial handshake (otherwise it is from the
+current, non-resumption, connection).  This can be called by either client or
+server. If the NID for the shared group is unknown then the value is set to the
+bitwise OR of TLSEXT_nid_unknown (0x1000000) and the id of the group.
 
 All these functions are implemented as macros.
 
@@ -110,9 +115,8 @@ is -1.
 When called on a client B<ssl>, SSL_get_shared_group() has no meaning and
 returns -1.
 
-SSL_get_negotiated_group() returns the NID of the negotiated group on a
-TLSv1.3 connection for key exchange. Or it returns NID_undef if no negotiated
-group.
+SSL_get_negotiated_group() returns the NID of the negotiated group used for
+key exchange, or NID_undef if there was no negotiated group.
 
 =head1 SEE ALSO
 

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 1b491e7f922401ce0719d7e3a7ece6e85af7fd3c..7839a4d318f5efa6fe4c407470f9def3530aee8a 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3636,9 +3636,16 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg)
             return id;
         }
     case SSL_CTRL_GET_NEGOTIATED_GROUP:
-        ret = tls1_group_id2nid(s->s3.group_id, 1);
-        break;
+        {
+            unsigned int id;
 
+            if (SSL_IS_TLS13(s) && s->s3.did_kex)
+                id = s->s3.group_id;
+            else
+                id = s->session->kex_group;
+            ret = tls1_group_id2nid(id, 1);
+            break;
+        }
     case SSL_CTRL_SET_SIGALGS:
         return tls1_set_sigalgs(s->cert, parg, larg, 0);
 

diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c
index de93ccbde6ed972f92a5b498bce7be6c3ce1f634..b27a58df7ce9a38b9893eba95cf13c23ce95c57f 100644 (file)
--- a/ssl/ssl_asn1.c
+++ b/ssl/ssl_asn1.c
@@ -43,6 +43,7 @@ typedef struct {
     ASN1_OCTET_STRING *alpn_selected;
     uint32_t tlsext_max_fragment_len_mode;
     ASN1_OCTET_STRING *ticket_appdata;
+    uint32_t kex_group;
 } SSL_SESSION_ASN1;
 
 ASN1_SEQUENCE(SSL_SESSION_ASN1) = {
@@ -73,7 +74,8 @@ ASN1_SEQUENCE(SSL_SESSION_ASN1) = {
     ASN1_EXP_OPT_EMBED(SSL_SESSION_ASN1, max_early_data, ZUINT32, 15),
     ASN1_EXP_OPT(SSL_SESSION_ASN1, alpn_selected, ASN1_OCTET_STRING, 16),
     ASN1_EXP_OPT_EMBED(SSL_SESSION_ASN1, tlsext_max_fragment_len_mode, ZUINT32, 17),
-    ASN1_EXP_OPT(SSL_SESSION_ASN1, ticket_appdata, ASN1_OCTET_STRING, 18)
+    ASN1_EXP_OPT(SSL_SESSION_ASN1, ticket_appdata, ASN1_OCTET_STRING, 18),
+    ASN1_EXP_OPT_EMBED(SSL_SESSION_ASN1, kex_group, UINT32, 19)
 } static_ASN1_SEQUENCE_END(SSL_SESSION_ASN1)
 
 IMPLEMENT_STATIC_ASN1_ENCODE_FUNCTIONS(SSL_SESSION_ASN1)
@@ -134,6 +136,8 @@ int i2d_SSL_SESSION(const SSL_SESSION *in, unsigned char **pp)
     as.version = SSL_SESSION_ASN1_VERSION;
     as.ssl_version = in->ssl_version;
 
+    as.kex_group = in->kex_group;
+
     if (in->cipher == NULL)
         l = in->cipher_id;
     else
@@ -272,6 +276,8 @@ SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, const unsigned char **pp,
 
     ret->ssl_version = (int)as->ssl_version;
 
+    ret->kex_group = as->kex_group;
+
     if (as->cipher->length != 2) {
         ERR_raise(ERR_LIB_SSL, SSL_R_CIPHER_CODE_WRONG_LENGTH);
         goto err;

diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h
index 0a6c4bf9ecf92a207a032ef282330d1c12b42066..8f3a2f93d6d4fd2767d02482d3b61090d0924325 100644 (file)
--- a/ssl/ssl_local.h
+++ b/ssl/ssl_local.h
@@ -599,6 +599,7 @@ struct ssl_session_st {
     const SSL_CIPHER *cipher;
     unsigned long cipher_id;    /* when ASN.1 loaded, this needs to be used to
                                  * load the 'cipher' structure */
+    unsigned int kex_group;      /* TLS group from key exchange */
     CRYPTO_EX_DATA ex_data;     /* application specific data */
     /*
      * These are used to make removal of session-ids more efficient and to
@@ -1412,6 +1413,12 @@ struct ssl_st {
          */
         char is_probably_safari;
 
+        /*
+         * Track whether we did a key exchange this handshake or not, so
+         * SSL_get_negotiated_group() knows whether to fall back to the
+         * value in the SSL_SESSION.
+         */
+        char did_kex;
         /* For clients: peer temporary key */
         /* The group_id for the key exchange key */
         uint16_t group_id;

diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index b3ef1bc16a761045774c8982922295f6aa001879..fe9f8a9de671916b8bf35162fca98fae1bc7c869 100644 (file)
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -1793,6 +1793,28 @@ int tls_parse_stoc_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
         SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_R_BAD_KEY_SHARE);
         return 0;
     }
+    /* Retain this group in the SSL_SESSION */
+    if (!s->hit) {
+        s->session->kex_group = group_id;
+    } else if (group_id != s->session->kex_group) {
+        /*
+         * If this is a resumption but changed what group was used, we need
+         * to record the new group in the session, but the session is not
+         * a new session and could be in use by other threads.  So, make
+         * a copy of the session to record the new information so that it's
+         * useful for any sessions resumed from tickets issued on this
+         * connection.
+         */
+        SSL_SESSION *new_sess;
+
+        if ((new_sess = ssl_session_dup(s->session, 0)) == NULL) {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_MALLOC_FAILURE);
+            return 0;
+        }
+        SSL_SESSION_free(s->session);
+        s->session = new_sess;
+        s->session->kex_group = group_id;
+    }
 
     if ((ginf = tls1_group_id_lookup(s->ctx, group_id)) == NULL) {
         SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_R_BAD_KEY_SHARE);
@@ -1836,6 +1858,7 @@ int tls_parse_stoc_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
             return 0;
         }
     }
+    s->s3.did_kex = 1;
 #endif
 
     return 1;

diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index b2d7ff8f39c4ff3595b3f759c2592fc268aafb49..6b3b33e239dde652d7db79b7ca5f62fe5320194b 100644 (file)
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -669,6 +669,8 @@ int tls_parse_ctos_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
         }
 
         s->s3.group_id = group_id;
+        /* Cache the selected group ID in the SSL_SESSION */
+        s->session->kex_group = group_id;
 
         if (EVP_PKEY_set1_encoded_public_key(s->s3.peer_tmp,
                 PACKET_data(&encoded_pt),
@@ -1705,6 +1707,7 @@ EXT_RETURN tls_construct_stoc_key_share(SSL *s, WPACKET *pkt,
             return EXT_RETURN_FAIL;
         }
     }
+    s->s3.did_kex = 1;
     return EXT_RETURN_SENT;
 #else
     return EXT_RETURN_FAIL;

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index dab4d1c4bc675aa3dad2ac5298cc0891a0f4ce9c..85ed3e42597c382b386fb5b136f2248b4be2b2cd 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -2167,6 +2167,8 @@ static int tls_process_ske_ecdhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey)
         *pkey = X509_get0_pubkey(s->session->peer);
     /* else anonymous ECDH, so no certificate or pkey. */
 
+    /* Cache the agreed upon group in the SSL_SESSION */
+    s->session->kex_group = curve_id;
     return 1;
 }
 

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index bad361917004ade7efd855a8a74b11437a45aab1..768e1110e6a44fed915a57174cabeb33b350429b 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -2519,8 +2519,10 @@ int tls_construct_server_key_exchange(SSL *s, WPACKET *pkt)
                      SSL_R_UNSUPPORTED_ELLIPTIC_CURVE);
             goto err;
         }
-        s->s3.tmp.pkey = ssl_generate_pkey_group(s, curve_id);
+        /* Cache the group used in the SSL_SESSION */
+        s->session->kex_group = curve_id;
         /* Generate a new key for this curve */
+        s->s3.tmp.pkey = ssl_generate_pkey_group(s, curve_id);
         if (s->s3.tmp.pkey == NULL) {
             /* SSLfatal() already called */
             goto err;