From ef7daaf915d7e0b7b48027f9ac4d47493adef0bb Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 3 Nov 2016 18:50:41 +0000 Subject: [PATCH] Validate that the provided key_share is in supported_groups Reviewed-by: Rich Salz --- ssl/statem/statem_srvr.c | 2 +- ssl/t1_lib.c | 133 +++++++++++++++++++++++++++++---------- 2 files changed, 101 insertions(+), 34 deletions(-) diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 0e910aa2f2..c8c0b8e17a 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1450,7 +1450,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) } /* Check we've got a key_share for TLSv1.3 */ - if (s->version == TLS1_3_VERSION && s->s3->peer_tmp == NULL) { + if (s->version == TLS1_3_VERSION && s->s3->peer_tmp == NULL && !s->hit) { /* No suitable share */ /* TODO(1.3): Send a HelloRetryRequest */ al = SSL_AD_HANDSHAKE_FAILURE; diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 46ca29099e..044c403ebf 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1649,7 +1649,7 @@ int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al) } #endif - if (s->version == TLS1_3_VERSION) { + if (s->version == TLS1_3_VERSION && !s->hit) { unsigned char *encodedPoint; size_t encoded_pt_len = 0; EVP_PKEY *ckey = NULL, *skey = NULL; @@ -1885,6 +1885,79 @@ static void ssl_check_for_safari(SSL *s, const CLIENTHELLO_MSG *hello) } #endif /* !OPENSSL_NO_EC */ + +/* + * Process the supported_groups extension if present. Returns success if the + * extension is absent, or if it has been successfully processed. + * + * Returns + * 1 on success + * 0 on failure + */ +static int tls_process_supported_groups(SSL *s, CLIENTHELLO_MSG *hello) +{ +#ifndef OPENSSL_NO_EC + PACKET supported_groups_list; + RAW_EXTENSION *suppgroups = tls_get_extension_by_type(hello->pre_proc_exts, + hello->num_extensions, + TLSEXT_TYPE_supported_groups); + + if (suppgroups == NULL) + return 1; + + /* Each group is 2 bytes and we must have at least 1. */ + if (!PACKET_as_length_prefixed_2(&suppgroups->data, + &supported_groups_list) + || PACKET_remaining(&supported_groups_list) == 0 + || (PACKET_remaining(&supported_groups_list) % 2) != 0) { + return 0; + } + + if (!s->hit + && !PACKET_memdup(&supported_groups_list, + &s->session->tlsext_supportedgroupslist, + &s->session->tlsext_supportedgroupslist_length)) { + return 0; + } +#endif + return 1; +} + +/* + * Checks a list of |groups| to determine if the |group_id| is in it. If it is + * and |checkallow| is 1 then additionally check if the group is allowed to be + * used. + * + * Returns: + * 1 if the group is in the list (and allowed if |checkallow| is 1) + * 0 otherwise + */ +static int check_in_list(SSL *s, unsigned int group_id, + const unsigned char *groups, size_t num_groups, + int checkallow) +{ + size_t i; + + if (groups == NULL || num_groups == 0) + 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 + && (!checkallow || tls_curve_allowed(s, groups, + SSL_SECOP_CURVE_CHECK))) { + break; + } + } + + if (i == num_groups) { + /* Not in list */ + return 0; + } + + return 1; +} + /* * Loop through all remaining ClientHello extensions that we collected earlier * and haven't already processed. For each one parse it and update the SSL @@ -1933,6 +2006,15 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) s->srtp_profile = NULL; + /* + * We process the supported_groups extension first so that is done before + * we get to key_share which needs to use the information in it. + */ + if (!tls_process_supported_groups(s, hello)) { + *al = TLS1_AD_INTERNAL_ERROR; + return 0; + } + /* * We parse all extensions to ensure the ClientHello is well-formed but, * unless an extension specifies otherwise, we ignore extensions upon @@ -2074,26 +2156,6 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) return 0; } } - } else if (currext->type == TLSEXT_TYPE_supported_groups) { - PACKET supported_groups_list; - - /* Each group is 2 bytes and we must have at least 1. */ - if (!PACKET_as_length_prefixed_2(&currext->data, - &supported_groups_list) - || PACKET_remaining(&supported_groups_list) == 0 - || (PACKET_remaining(&supported_groups_list) % 2) != 0) { - return 0; - } - - if (!s->hit) { - if (!PACKET_memdup(&supported_groups_list, - &s->session->tlsext_supportedgroupslist, - &s-> - session->tlsext_supportedgroupslist_length)) { - *al = TLS1_AD_INTERNAL_ERROR; - return 0; - } - } } #endif /* OPENSSL_NO_EC */ else if (currext->type == TLSEXT_TYPE_session_ticket) { @@ -2251,11 +2313,11 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) && !(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC)) { s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC; } else if (currext->type == TLSEXT_TYPE_key_share - && s->version == TLS1_3_VERSION) { + && s->version == TLS1_3_VERSION && !s->hit) { unsigned int group_id; PACKET key_share_list, encoded_pt; const unsigned char *curves; - size_t num_curves, i; + size_t num_curves; int group_nid; unsigned int curve_flags; @@ -2283,23 +2345,28 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CLIENTHELLO_MSG *hello, int *al) return 0; } - /* Find a share that we can use */ - if (!tls1_get_curvelist(s, 0, &curves, &num_curves)) { + /* Check this share is in supported_groups */ + if (!tls1_get_curvelist(s, 1, &curves, &num_curves)) { *al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); return 0; } - for (i = 0; i < num_curves; i++, curves += 2) { - unsigned int share_id = (curves[0] << 8) | (curves[1]); - if (group_id == share_id - && tls_curve_allowed(s, curves, - SSL_SECOP_CURVE_CHECK)) { - break; - } + if (!check_in_list(s, group_id, curves, num_curves, 0)) { + *al = SSL_AD_HANDSHAKE_FAILURE; + SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, + SSL_R_BAD_KEY_SHARE); + return 0; } - if (i == num_curves) { + /* Find a share that we can use */ + if (!tls1_get_curvelist(s, 0, &curves, &num_curves)) { + *al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, + ERR_R_INTERNAL_ERROR); + return 0; + } + if (!check_in_list(s, group_id, curves, num_curves, 1)) { /* Share not suitable */ continue; } -- 2.25.1