From aa6bd216dd2691d1254eabcbd584691eb3b4b9b8 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Tue, 16 Mar 2021 07:47:09 -0700 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/14750) --- doc/man3/SSL_CTX_set1_curves.pod | 18 +++++++++++------- ssl/s3_lib.c | 11 +++++++++-- ssl/ssl_asn1.c | 8 +++++++- ssl/ssl_local.h | 7 +++++++ ssl/statem/extensions_clnt.c | 23 +++++++++++++++++++++++ ssl/statem/extensions_srvr.c | 3 +++ ssl/statem/statem_clnt.c | 2 ++ ssl/statem/statem_srvr.c | 4 +++- 8 files changed, 65 insertions(+), 11 deletions(-) diff --git a/doc/man3/SSL_CTX_set1_curves.pod b/doc/man3/SSL_CTX_set1_curves.pod index 5eebb2b933..65892e46a5 100644 --- 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_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 1b491e7f92..7839a4d318 100644 --- 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 de93ccbde6..b27a58df7c 100644 --- 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 0a6c4bf9ec..8f3a2f93d6 100644 --- 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 b3ef1bc16a..fe9f8a9de6 100644 --- 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 b2d7ff8f39..6b3b33e239 100644 --- 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 dab4d1c4bc..85ed3e4259 100644 --- 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 bad3619170..768e1110e6 100644 --- 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; -- 2.39.5