From 6af87546375953d5937fb4bdaac6a887765af615 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 5 May 2017 10:27:14 +0100 Subject: [PATCH] Send the supported_groups extension in EE where applicable The TLSv1.3 spec says that a server SHOULD send supported_groups in the EE message if there is a group that it prefers to the one used in the key_share. Clients MAY act on that. At the moment we don't do anything with it on the client side, but that may change in the future. Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/3395) --- include/openssl/ssl.h | 1 + ssl/ssl_err.c | 2 ++ ssl/statem/extensions.c | 2 +- ssl/statem/extensions_srvr.c | 57 ++++++++++++++++++++++++++++++++++++ ssl/statem/statem_lib.c | 4 +-- ssl/statem/statem_locl.h | 6 ++++ 6 files changed, 68 insertions(+), 4 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 764ecea31a..e89e97f7f1 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2496,6 +2496,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_TLS_CONSTRUCT_STOC_SERVER_NAME 459 # define SSL_F_TLS_CONSTRUCT_STOC_SESSION_TICKET 460 # define SSL_F_TLS_CONSTRUCT_STOC_STATUS_REQUEST 461 +# define SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_GROUPS 544 # define SSL_F_TLS_CONSTRUCT_STOC_USE_SRTP 462 # define SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO 521 # define SSL_F_TLS_GET_MESSAGE_BODY 351 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index a845dae421..18a38dfe47 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -398,6 +398,8 @@ static ERR_STRING_DATA SSL_str_functs[] = { "tls_construct_stoc_session_ticket"}, {ERR_FUNC(SSL_F_TLS_CONSTRUCT_STOC_STATUS_REQUEST), "tls_construct_stoc_status_request"}, + {ERR_FUNC(SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_GROUPS), + "tls_construct_stoc_supported_groups"}, {ERR_FUNC(SSL_F_TLS_CONSTRUCT_STOC_USE_SRTP), "tls_construct_stoc_use_srtp"}, {ERR_FUNC(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO), diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 847ff139a4..8984577d4f 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -151,7 +151,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { TLSEXT_TYPE_supported_groups, SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_3_ENCRYPTED_EXTENSIONS, NULL, tls_parse_ctos_supported_groups, NULL, - NULL /* TODO(TLS1.3): Need to add this */, + tls_construct_stoc_supported_groups, tls_construct_ctos_supported_groups, NULL }, #else diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 7ba1aac8d4..b12505c1f7 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -868,6 +868,63 @@ int tls_construct_stoc_ec_pt_formats(SSL *s, WPACKET *pkt, unsigned int context, } #endif +int tls_construct_stoc_supported_groups(SSL *s, WPACKET *pkt, + unsigned int context, X509 *x, + size_t chainidx, int *al) +{ + const unsigned char *groups; + size_t numgroups, i, first = 1; + + /* s->s3->group_id is non zero if we accepted a key_share */ + if (s->s3->group_id == 0) + return 1; + + /* Get our list of supported groups */ + if (!tls1_get_curvelist(s, 0, &groups, &numgroups) || numgroups == 0) { + SSLerr(SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_GROUPS, ERR_R_INTERNAL_ERROR); + return 0; + } + + /* Copy group ID if supported */ + for (i = 0; i < numgroups; i++, groups += 2) { + if (tls_curve_allowed(s, groups, SSL_SECOP_CURVE_SUPPORTED)) { + if (first) { + /* + * Check if the client is already using our preferred group. If + * so we don't need to add this extension + */ + if (s->s3->group_id == GET_GROUP_ID(groups, 0)) + return 1; + + /* Add extension header */ + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_supported_groups) + /* Sub-packet for supported_groups extension */ + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_start_sub_packet_u16(pkt)) { + SSLerr(SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_GROUPS, + ERR_R_INTERNAL_ERROR); + return 0; + } + + first = 0; + } + if (!WPACKET_put_bytes_u8(pkt, groups[0]) + || !WPACKET_put_bytes_u8(pkt, groups[1])) { + SSLerr(SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_GROUPS, + ERR_R_INTERNAL_ERROR); + return 0; + } + } + } + + if (!WPACKET_close(pkt) || !WPACKET_close(pkt)) { + SSLerr(SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_GROUPS, ERR_R_INTERNAL_ERROR); + return 0; + } + + return 1; +} + int tls_construct_stoc_session_ticket(SSL *s, WPACKET *pkt, unsigned int context, X509 *x, size_t chainidx, int *al) diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 36d55343b8..33206c6ea9 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -1957,9 +1957,7 @@ int check_in_list(SSL *s, unsigned int group_id, const unsigned char *groups, return 0; for (i = 0; i < num_groups; i++, groups += 2) { - unsigned int share_id = (groups[0] << 8) | (groups[1]); - - if (group_id == share_id + if (group_id == GET_GROUP_ID(groups, 0) && (!checkallow || tls_curve_allowed(s, groups, SSL_SECOP_CURVE_CHECK))) { return 1; diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 3b9311e02f..49a5ed5cef 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -55,6 +55,9 @@ int statem_flush(SSL *s); typedef int (*confunc_f) (SSL *s, WPACKET *pkt); +#define GET_GROUP_ID(group, idx) \ + (unsigned int)(((group)[(idx) * 2] << 8) | (group)[((idx) * 2) + 1]) + int check_in_list(SSL *s, unsigned int group_id, const unsigned char *groups, size_t num_groups, int checkallow); int create_synthetic_message_hash(SSL *s); @@ -230,6 +233,9 @@ int tls_construct_stoc_early_data(SSL *s, WPACKET *pkt, unsigned int context, int tls_construct_stoc_ec_pt_formats(SSL *s, WPACKET *pkt, unsigned int context, X509 *x, size_t chainidx, int *al); #endif +int tls_construct_stoc_supported_groups(SSL *s, WPACKET *pkt, + unsigned int context, X509 *x, + size_t chainidx, int *al); int tls_construct_stoc_session_ticket(SSL *s, WPACKET *pkt, unsigned int context, X509 *x, size_t chainidx, int *al); -- 2.25.1