From 34e5292c578321b80d8e474db4be6d90519d8f33 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Sun, 24 Sep 2017 03:26:26 +0100 Subject: [PATCH] Rename tls1_get_curvelist. Rename tls1_get_curvelist to tls1_get_grouplist, change to void as it can never fail and remove unnecessary return value checks. Clean up the code. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/=4412) --- ssl/ssl_locl.h | 4 +- ssl/statem/extensions.c | 14 +------ ssl/statem/extensions_clnt.c | 16 ++------ ssl/statem/extensions_srvr.c | 16 ++------ ssl/t1_lib.c | 76 +++++++++++++++++------------------- 5 files changed, 46 insertions(+), 80 deletions(-) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 2a187c0a64..58156f37fe 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2354,8 +2354,8 @@ __owur EVP_PKEY *ssl_generate_param_group(uint16_t id); # endif /* OPENSSL_NO_EC */ __owur int tls_curve_allowed(SSL *s, uint16_t curve, int op); -__owur int tls1_get_curvelist(SSL *s, int sess, const uint16_t **pcurves, - size_t *num_curves); +void tls1_get_grouplist(SSL *s, int sess, const uint16_t **pcurves, + size_t *num_curves); __owur int tls1_set_server_sigalgs(SSL *s); diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 8b88b21584..2991310124 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -1144,18 +1144,8 @@ static int final_key_share(SSL *s, unsigned int context, int sent, int *al) /* Check if a shared group exists */ /* Get the clients list of supported groups. */ - if (!tls1_get_curvelist(s, 1, &clntcurves, &clnt_num_curves)) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_FINAL_KEY_SHARE, ERR_R_INTERNAL_ERROR); - return 0; - } - - /* Get our list of available groups */ - if (!tls1_get_curvelist(s, 0, &pcurves, &num_curves)) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_FINAL_KEY_SHARE, ERR_R_INTERNAL_ERROR); - return 0; - } + tls1_get_grouplist(s, 1, &clntcurves, &clnt_num_curves); + tls1_get_grouplist(s, 0, &pcurves, &num_curves); /* Find the first group we allow that is also in client's list */ for (i = 0; i < num_curves; i++) { diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index d8a7f2f6a9..047f2d0cea 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -149,11 +149,7 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL *s, WPACKET *pkt, * Add TLS extension supported_groups to the ClientHello message */ /* TODO(TLS1.3): Add support for DHE groups */ - if (!tls1_get_curvelist(s, 0, &pcurves, &num_curves)) { - SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_SUPPORTED_GROUPS, - ERR_R_INTERNAL_ERROR); - return EXT_RETURN_FAIL; - } + tls1_get_grouplist(s, 0, &pcurves, &num_curves); if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_supported_groups) /* Sub-packet for supported_groups extension */ @@ -608,10 +604,7 @@ EXT_RETURN tls_construct_ctos_key_share(SSL *s, WPACKET *pkt, return EXT_RETURN_FAIL; } - if (!tls1_get_curvelist(s, 0, &pcurves, &num_curves)) { - SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_KEY_SHARE, ERR_R_INTERNAL_ERROR); - return EXT_RETURN_FAIL; - } + tls1_get_grouplist(s, 0, &pcurves, &num_curves); /* * TODO(TLS1.3): Make the number of key_shares sent configurable. For @@ -1541,10 +1534,7 @@ int tls_parse_stoc_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x, } /* Validate the selected group is one we support */ - if (!tls1_get_curvelist(s, 0, &pcurves, &num_curves)) { - SSLerr(SSL_F_TLS_PARSE_STOC_KEY_SHARE, ERR_R_INTERNAL_ERROR); - return 0; - } + tls1_get_grouplist(s, 0, &pcurves, &num_curves); for (i = 0; i < num_curves; i++) { if (group_id == pcurves[i]) break; diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index ebae44899a..3be9754b2a 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -520,18 +520,9 @@ int tls_parse_ctos_key_share(SSL *s, PACKET *pkt, unsigned int context, X509 *x, } /* Get our list of supported curves */ - if (!tls1_get_curvelist(s, 0, &srvrcurves, &srvr_num_curves)) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PARSE_CTOS_KEY_SHARE, ERR_R_INTERNAL_ERROR); - return 0; - } - + tls1_get_grouplist(s, 0, &srvrcurves, &srvr_num_curves); /* Get the clients list of supported curves. */ - if (!tls1_get_curvelist(s, 1, &clntcurves, &clnt_num_curves)) { - *al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_TLS_PARSE_CTOS_KEY_SHARE, ERR_R_INTERNAL_ERROR); - return 0; - } + tls1_get_grouplist(s, 1, &clntcurves, &clnt_num_curves); if (clnt_num_curves == 0) { /* * This can only happen if the supported_groups extension was not sent, @@ -894,7 +885,8 @@ EXT_RETURN tls_construct_stoc_supported_groups(SSL *s, WPACKET *pkt, return EXT_RETURN_NOT_SENT; /* Get our list of supported groups */ - if (!tls1_get_curvelist(s, 0, &groups, &numgroups) || numgroups == 0) { + tls1_get_grouplist(s, 0, &groups, &numgroups); + if (numgroups == 0) { SSLerr(SSL_F_TLS_CONSTRUCT_STOC_SUPPORTED_GROUPS, ERR_R_INTERNAL_ERROR); return EXT_RETURN_FAIL; } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index e21bb8b7d1..78e42fe239 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -213,43 +213,42 @@ static uint16_t tls1_nid2group_id(int nid) * The latter indicates an internal error: we should not be accepting such * lists in the first place. */ -int tls1_get_curvelist(SSL *s, int sess, const uint16_t **pcurves, - size_t *num_curves) +void tls1_get_grouplist(SSL *s, int sess, const uint16_t **pcurves, + size_t *pcurveslen) { - size_t pcurveslen = 0; if (sess) { *pcurves = s->session->ext.supportedgroups; - pcurveslen = s->session->ext.supportedgroups_len; - } else { - /* For Suite B mode only include P-256, P-384 */ - switch (tls1_suiteb(s)) { - case SSL_CERT_FLAG_SUITEB_128_LOS: - *pcurves = suiteb_curves; - pcurveslen = OSSL_NELEM(suiteb_curves); - break; + *pcurveslen = s->session->ext.supportedgroups_len; + return; + } + /* For Suite B mode only include P-256, P-384 */ + switch (tls1_suiteb(s)) { + case SSL_CERT_FLAG_SUITEB_128_LOS: + *pcurves = suiteb_curves; + *pcurveslen = OSSL_NELEM(suiteb_curves); + break; - case SSL_CERT_FLAG_SUITEB_128_LOS_ONLY: - *pcurves = suiteb_curves; - pcurveslen = 1; - break; + case SSL_CERT_FLAG_SUITEB_128_LOS_ONLY: + *pcurves = suiteb_curves; + *pcurveslen = 1; + break; - case SSL_CERT_FLAG_SUITEB_192_LOS: - *pcurves = suiteb_curves + 1; - pcurveslen = 1; - break; - default: - *pcurves = s->ext.supportedgroups; - pcurveslen = s->ext.supportedgroups_len; - } - if (!*pcurves) { + case SSL_CERT_FLAG_SUITEB_192_LOS: + *pcurves = suiteb_curves + 1; + *pcurveslen = 1; + break; + + default: + if (s->ext.supportedgroups == NULL) { *pcurves = eccurves_default; - pcurveslen = OSSL_NELEM(eccurves_default); + *pcurveslen = OSSL_NELEM(eccurves_default); + } else { + *pcurves = s->ext.supportedgroups; + *pcurveslen = s->ext.supportedgroups_len; } + break; } - - *num_curves = pcurveslen; - return 1; } /* See if curve is allowed by security callback */ @@ -293,8 +292,7 @@ int tls1_check_curve(SSL *s, const unsigned char *p, size_t len) } else /* Should never happen */ return 0; } - if (!tls1_get_curvelist(s, 0, &curves, &num_curves)) - return 0; + tls1_get_grouplist(s, 0, &curves, &num_curves); for (i = 0; i < num_curves; i++) { if (curve_id == curves[i]) return tls_curve_allowed(s, curve_id, SSL_SECOP_CURVE_CHECK); @@ -337,17 +335,15 @@ uint16_t tls1_shared_group(SSL *s, int nmatch) nmatch = 0; } /* - * Avoid truncation. tls1_get_curvelist takes an int + * Avoid truncation. tls1_get_grouplist takes an int * but s->options is a long... */ - if (!tls1_get_curvelist(s, + tls1_get_grouplist(s, (s->options & SSL_OP_CIPHER_SERVER_PREFERENCE) != 0, - &supp, &num_supp)) - return 0; - if (!tls1_get_curvelist(s, + &supp, &num_supp); + tls1_get_grouplist(s, (s->options & SSL_OP_CIPHER_SERVER_PREFERENCE) == 0, - &pref, &num_pref)) - return 0; + &pref, &num_pref); for (k = 0, i = 0; i < num_pref; i++) { uint16_t id = pref[i]; @@ -511,8 +507,7 @@ static int tls1_check_group_id(SSL *s, uint16_t group_id) return 0; /* Check group is one of our preferences */ - if (!tls1_get_curvelist(s, 0, &groups, &groups_len)) - return 0; + tls1_get_grouplist(s, 0, &groups, &groups_len); for (i = 0; i < groups_len; i++) { if (groups[i] == group_id) break; @@ -525,8 +520,7 @@ static int tls1_check_group_id(SSL *s, uint16_t group_id) return 1; /* Check group is one of peers preferences */ - if (!tls1_get_curvelist(s, 1, &groups, &groups_len)) - return 0; + tls1_get_grouplist(s, 1, &groups, &groups_len); /* * RFC 4492 does not require the supported elliptic curves extension -- 2.25.1