From 6447e8184cf6deca233d38ab3e9c9aa6ba60e9a5 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Tue, 26 Sep 2017 16:17:44 +0100 Subject: [PATCH] Merge tls1_check_curve into tls1_check_group_id The function tls_check_curve is only called on clients and contains almost identical functionaity to tls1_check_group_id when called from a client. Merge the two. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/4475) --- ssl/ssl_locl.h | 2 +- ssl/statem/statem_clnt.c | 14 +++++------ ssl/t1_lib.c | 50 +++++++++++++++------------------------- 3 files changed, 27 insertions(+), 39 deletions(-) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 95f679bb7c..c73035df70 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2347,7 +2347,7 @@ SSL_COMP *ssl3_comp_find(STACK_OF(SSL_COMP) *sk, int n); # ifndef OPENSSL_NO_EC __owur const TLS_GROUP_INFO *tls1_group_id_lookup(uint16_t curve_id); -__owur int tls1_check_curve(SSL *s, const unsigned char *p, size_t len); +__owur int tls1_check_group_id(SSL *s, uint16_t group_id); __owur uint16_t tls1_shared_group(SSL *s, int nmatch); __owur int tls1_set_groups(uint16_t **pext, size_t *pextlen, int *curves, size_t ncurves); diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 8ca47370cf..2ad33f2e7c 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -2037,29 +2037,29 @@ static int tls_process_ske_ecdhe(SSL *s, PACKET *pkt, EVP_PKEY **pkey, int *al) { #ifndef OPENSSL_NO_EC PACKET encoded_pt; - const unsigned char *ecparams; + unsigned int curve_type, curve_id; /* * Extract elliptic curve parameters and the server's ephemeral ECDH - * public key. For now we only support named (not generic) curves and + * public key. We only support named (not generic) curves and * ECParameters in this case is just three bytes. */ - if (!PACKET_get_bytes(pkt, &ecparams, 3)) { + if (!PACKET_get_1(pkt, &curve_type) || !PACKET_get_net_2(pkt, &curve_id)) { *al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE, SSL_R_LENGTH_TOO_SHORT); return 0; } /* - * Check curve is one of our preferences, if not server has sent an - * invalid curve. ECParameters is 3 bytes. + * Check curve is named curve type and one of our preferences, if not + * server has sent an invalid curve. */ - if (!tls1_check_curve(s, ecparams, 3)) { + if (curve_type != NAMED_CURVE_TYPE || !tls1_check_group_id(s, curve_id)) { *al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE, SSL_R_WRONG_CURVE); return 0; } - if ((s->s3->peer_tmp = ssl_generate_param_group(ecparams[2])) == NULL) { + if ((s->s3->peer_tmp = ssl_generate_param_group(curve_id)) == NULL) { *al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_TLS_PROCESS_SKE_ECDHE, SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS); diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 9582e21eea..bb097ed938 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -268,34 +268,6 @@ static int tls1_in_list(uint16_t id, const uint16_t *list, size_t listlen) return 0; } -/* Check a curve is one of our preferences */ -int tls1_check_curve(SSL *s, const unsigned char *p, size_t len) -{ - const uint16_t *curves; - size_t num_curves; - uint16_t curve_id; - - if (len != 3 || p[0] != NAMED_CURVE_TYPE) - return 0; - curve_id = (p[1] << 8) | p[2]; - /* Check curve matches Suite B preferences */ - if (tls1_suiteb(s)) { - unsigned long cid = s->s3->tmp.new_cipher->id; - if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256) { - if (curve_id != TLSEXT_curve_P_256) - return 0; - } else if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384) { - if (curve_id != TLSEXT_curve_P_384) - return 0; - } else /* Should never happen */ - return 0; - } - tls1_get_supported_groups(s, &curves, &num_curves); - if (!tls1_in_list(curve_id, curves, num_curves)) - return 0; - return tls_curve_allowed(s, curve_id, SSL_SECOP_CURVE_CHECK); -} - /*- * For nmatch >= 0, return the id of the |nmatch|th shared group or 0 * if there is no match. @@ -493,7 +465,7 @@ static int tls1_check_pkey_comp(SSL *s, EVP_PKEY *pkey) } /* Check a group id matches preferences */ -static int tls1_check_group_id(SSL *s, uint16_t group_id) +int tls1_check_group_id(SSL *s, uint16_t group_id) { const uint16_t *groups; size_t groups_len; @@ -501,14 +473,30 @@ static int tls1_check_group_id(SSL *s, uint16_t group_id) if (group_id == 0) return 0; - if (!tls_curve_allowed(s, group_id, SSL_SECOP_CURVE_CHECK)) - return 0; + /* Check for Suite B compliance */ + if (tls1_suiteb(s) && s->s3->tmp.new_cipher != NULL) { + unsigned long cid = s->s3->tmp.new_cipher->id; + + if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256) { + if (group_id != TLSEXT_curve_P_256) + return 0; + } else if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384) { + if (group_id != TLSEXT_curve_P_384) + return 0; + } else { + /* Should never happen */ + return 0; + } + } /* Check group is one of our preferences */ tls1_get_supported_groups(s, &groups, &groups_len); if (!tls1_in_list(group_id, groups, groups_len)) return 0; + if (!tls_curve_allowed(s, group_id, SSL_SECOP_CURVE_CHECK)) + return 0; + /* For clients, nothing more to check */ if (!s->server) return 1; -- 2.34.1