From 4a1b42801997f3083211a24592d1a691a0747250 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Sun, 24 Sep 2017 01:45:27 +0100 Subject: [PATCH] Rewrite compression and group checks. Replace existing compression and groups check with two functions. tls1_check_pkey_comp() checks a keys compression algorithms is consistent with extensions. tls1_check_group_id() checks is a group is consistent with extensions and preferences. Rename tls1_ec_nid2curve_id() to tls1_nid2group_id() and make it static. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/=4412) --- ssl/ssl_locl.h | 1 - ssl/t1_lib.c | 256 +++++++++++++++++++++++-------------------------- 2 files changed, 122 insertions(+), 135 deletions(-) diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index dc4a0f4701..2a187c0a64 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2340,7 +2340,6 @@ 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 uint16_t tls1_ec_nid2curve_id(int nid); __owur int tls1_check_curve(SSL *s, const unsigned char *p, size_t len); __owur uint16_t tls1_shared_group(SSL *s, int nmatch); __owur int tls1_set_groups(uint16_t **pext, size_t *pextlen, diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index c7a8a53dc4..e21bb8b7d1 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -194,7 +194,7 @@ const TLS_GROUP_INFO *tls1_group_id_lookup(uint16_t curve_id) return &nid_list[curve_id - 1]; } -uint16_t tls1_ec_nid2curve_id(int nid) +static uint16_t tls1_nid2group_id(int nid) { size_t i; for (i = 0; i < OSSL_NELEM(nid_list); i++) { @@ -385,7 +385,7 @@ int tls1_set_groups(uint16_t **pext, size_t *pextlen, unsigned long idmask; uint16_t id; /* TODO(TLS1.3): Convert for DH groups */ - id = tls1_ec_nid2curve_id(groups[i]); + id = tls1_nid2group_id(groups[i]); idmask = 1L << id; if (!id || (dup_list & idmask)) { OPENSSL_free(glist); @@ -446,88 +446,102 @@ int tls1_set_groups_list(uint16_t **pext, size_t *pextlen, const char *str) return 1; return tls1_set_groups(pext, pextlen, ncb.nid_arr, ncb.nidcnt); } - -/* For an EC key set TLS id and required compression based on parameters */ -static int tls1_set_ec_id(uint16_t *pcurve_id, unsigned char *comp_id, - EC_KEY *ec) +/* Return group id of a key */ +static uint16_t tls1_get_group_id(EVP_PKEY *pkey) { - int curve_nid; + EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); const EC_GROUP *grp; - if (!ec) + + if (ec == NULL) return 0; - /* Determine if it is a prime field */ grp = EC_KEY_get0_group(ec); - if (!grp) - return 0; - /* Determine curve ID */ - curve_nid = EC_GROUP_get_curve_name(grp); - *pcurve_id = tls1_ec_nid2curve_id(curve_nid); - /* If no id return error: we don't support arbitrary explicit curves */ - if (*pcurve_id == 0) - return 0; - if (comp_id) { - if (EC_KEY_get0_public_key(ec) == NULL) - return 0; - if (EC_KEY_get_conv_form(ec) == POINT_CONVERSION_UNCOMPRESSED) { - *comp_id = TLSEXT_ECPOINTFORMAT_uncompressed; - } else { - if ((nid_list[*pcurve_id - 1].flags & TLS_CURVE_TYPE) == TLS_CURVE_PRIME) - *comp_id = TLSEXT_ECPOINTFORMAT_ansiX962_compressed_prime; - else - *comp_id = TLSEXT_ECPOINTFORMAT_ansiX962_compressed_char2; - } - } - return 1; + return tls1_nid2group_id(EC_GROUP_get_curve_name(grp)); } -/* Check an EC key is compatible with extensions */ -static int tls1_check_ec_key(SSL *s, uint16_t curve_id, unsigned char *comp_id) +/* Check a key is compatible with compression extension */ +static int tls1_check_pkey_comp(SSL *s, EVP_PKEY *pkey) { - const unsigned char *pformats; - const uint16_t *pcurves; - size_t num_formats, num_curves, i; - int j; + const EC_KEY *ec; + const EC_GROUP *grp; + unsigned char comp_id; + size_t i; + + /* If not an EC key nothing to check */ + if (EVP_PKEY_id(pkey) != EVP_PKEY_EC) + return 1; + ec = EVP_PKEY_get0_EC_KEY(pkey); + grp = EC_KEY_get0_group(ec); + + /* Get required compression id */ + if (EC_KEY_get_conv_form(ec) == POINT_CONVERSION_UNCOMPRESSED) { + comp_id = TLSEXT_ECPOINTFORMAT_uncompressed; + } else if (SSL_IS_TLS13(s)) { + /* Compression not allowed in TLS 1.3 */ + return 0; + } else { + int field_type = EC_METHOD_get_field_type(EC_GROUP_method_of(grp)); + + if (field_type == NID_X9_62_prime_field) + comp_id = TLSEXT_ECPOINTFORMAT_ansiX962_compressed_prime; + else if (field_type == NID_X9_62_prime_field) + comp_id = TLSEXT_ECPOINTFORMAT_ansiX962_compressed_char2; + else + return 0; + } /* * If point formats extension present check it, otherwise everything is * supported (see RFC4492). */ - if (comp_id && s->session->ext.ecpointformats) { - pformats = s->session->ext.ecpointformats; - num_formats = s->session->ext.ecpointformats_len; - for (i = 0; i < num_formats; i++, pformats++) { - if (*comp_id == *pformats) - break; - } - if (i == num_formats) - return 0; - } - if (curve_id == 0) + if (s->session->ext.ecpointformats == NULL) return 1; - /* Check curve is consistent with client and server preferences */ - for (j = 0; j <= 1; j++) { - if (!tls1_get_curvelist(s, j, &pcurves, &num_curves)) - return 0; - if (j == 1 && num_curves == 0) { - /* - * If we've not received any curves then skip this check. - * RFC 4492 does not require the supported elliptic curves extension - * so if it is not sent we can just choose any curve. - * It is invalid to send an empty list in the elliptic curves - * extension, so num_curves == 0 always means no extension. - */ - break; - } - for (i = 0; i < num_curves; i++) { - if (pcurves[i] == curve_id) - break; - } - if (i == num_curves) - return 0; - /* For clients can only check sent curve list */ - if (!s->server) + + for (i = 0; i < s->session->ext.ecpointformats_len; i++) { + if (s->session->ext.ecpointformats[i] == comp_id) + return 1; + } + return 0; +} +/* Check a group id matches preferences */ +static int tls1_check_group_id(SSL *s, uint16_t group_id) + { + const uint16_t *groups; + size_t i, groups_len; + + if (group_id == 0) + return 0; + + /* Check group is one of our preferences */ + if (!tls1_get_curvelist(s, 0, &groups, &groups_len)) + return 0; + for (i = 0; i < groups_len; i++) { + if (groups[i] == group_id) break; } - return 1; + if (i == groups_len) + return 0; + + /* For clients, nothing more to check */ + if (!s->server) + return 1; + + /* Check group is one of peers preferences */ + if (!tls1_get_curvelist(s, 1, &groups, &groups_len)) + return 0; + + /* + * RFC 4492 does not require the supported elliptic curves extension + * so if it is not sent we can just choose any curve. + * It is invalid to send an empty list in the supported groups + * extension, so groups_len == 0 always means no extension. + */ + if (groups_len == 0) + return 1; + + for (i = 0; i < groups_len; i++) { + if (groups[i] == group_id) + return 1; + } + return 0; } void tls1_get_formatlist(SSL *s, const unsigned char **pformats, @@ -555,25 +569,19 @@ void tls1_get_formatlist(SSL *s, const unsigned char **pformats, */ static int tls1_check_cert_param(SSL *s, X509 *x, int check_ee_md) { - unsigned char comp_id; - uint16_t curve_id; + uint16_t group_id; EVP_PKEY *pkey; - int rv; pkey = X509_get0_pubkey(x); - if (!pkey) + if (pkey == NULL) return 0; /* If not EC nothing to do */ if (EVP_PKEY_id(pkey) != EVP_PKEY_EC) return 1; - rv = tls1_set_ec_id(&curve_id, &comp_id, EVP_PKEY_get0_EC_KEY(pkey)); - if (!rv) + /* Check compression */ + if (!tls1_check_pkey_comp(s, pkey)) return 0; - /* - * Can't check curve_id for client certs as we don't have a supported - * curves extension. - */ - rv = tls1_check_ec_key(s, s->server ? curve_id : 0, &comp_id); - if (!rv) + group_id = tls1_get_group_id(pkey); + if (!tls1_check_group_id(s, group_id)) return 0; /* * Special case for suite B. We *MUST* sign using SHA256+P-256 or @@ -585,19 +593,19 @@ static int tls1_check_cert_param(SSL *s, X509 *x, int check_ee_md) CERT *c = s->cert; /* Check to see we have necessary signing algorithm */ - if (curve_id == TLSEXT_curve_P_256) + if (group_id == TLSEXT_curve_P_256) check_md = NID_ecdsa_with_SHA256; - else if (curve_id == TLSEXT_curve_P_384) + else if (group_id == TLSEXT_curve_P_384) check_md = NID_ecdsa_with_SHA384; else return 0; /* Should never happen */ - for (i = 0; i < c->shared_sigalgslen; i++) + for (i = 0; i < c->shared_sigalgslen; i++) { if (check_md == c->shared_sigalgs[i]->sigandhash) - break; - if (i == c->shared_sigalgslen) - return 0; + return 1;; + } + return 0; } - return rv; + return 1; } /* @@ -612,27 +620,19 @@ static int tls1_check_cert_param(SSL *s, X509 *x, int check_ee_md) */ int tls1_check_ec_tmp_key(SSL *s, unsigned long cid) { + /* If not Suite B just need a shared group */ + if (!tls1_suiteb(s)) + return tls1_shared_group(s, 0) != 0; /* * If Suite B, AES128 MUST use P-256 and AES256 MUST use P-384, no other * curves permitted. */ - if (tls1_suiteb(s)) { - uint16_t curve_id; - - /* Curve to check determined by ciphersuite */ - if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256) - curve_id = TLSEXT_curve_P_256; - else if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384) - curve_id = TLSEXT_curve_P_384; - else - return 0; - /* Check this curve is acceptable */ - if (!tls1_check_ec_key(s, curve_id, NULL)) - return 0; - return 1; - } - /* Need a shared curve */ - return tls1_shared_group(s, 0) != 0; + if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256) + return tls1_check_group_id(s, TLSEXT_curve_P_256); + if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384) + return tls1_check_group_id(s, TLSEXT_curve_P_384); + + return 0; } #else @@ -944,28 +944,27 @@ int tls12_check_peer_sigalg(SSL *s, uint16_t sig, EVP_PKEY *pkey) } #ifndef OPENSSL_NO_EC if (pkeyid == EVP_PKEY_EC) { - EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); - int curve = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec)); - if (SSL_IS_TLS13(s)) { - if (EC_KEY_get_conv_form(ec) != POINT_CONVERSION_UNCOMPRESSED) { - SSLerr(SSL_F_TLS12_CHECK_PEER_SIGALG, - SSL_R_ILLEGAL_POINT_COMPRESSION); - return 0; - } - /* For TLS 1.3 check curve matches signature algorithm */ + /* Check point compression is permitted */ + if (!tls1_check_pkey_comp(s, pkey)) { + SSLerr(SSL_F_TLS12_CHECK_PEER_SIGALG, + SSL_R_ILLEGAL_POINT_COMPRESSION); + return 0; + } + + /* For TLS 1.3 or Suite B check curve matches signature algorithm */ + if (SSL_IS_TLS13(s) || tls1_suiteb(s)) { + EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); + int curve = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec)); + if (lu->curve != NID_undef && curve != lu->curve) { SSLerr(SSL_F_TLS12_CHECK_PEER_SIGALG, SSL_R_WRONG_CURVE); return 0; } - } else { - unsigned char comp_id; - uint16_t curve_id; - - /* Check compression and curve matches extensions */ - if (!tls1_set_ec_id(&curve_id, &comp_id, ec)) - return 0; - if (!s->server && !tls1_check_ec_key(s, curve_id, &comp_id)) { + } + if (!SSL_IS_TLS13(s)) { + /* Check curve matches extensions */ + if (!tls1_check_group_id(s, tls1_get_group_id(pkey))) { SSLerr(SSL_F_TLS12_CHECK_PEER_SIGALG, SSL_R_WRONG_CURVE); return 0; } @@ -977,17 +976,6 @@ int tls12_check_peer_sigalg(SSL *s, uint16_t sig, EVP_PKEY *pkey) SSL_R_WRONG_SIGNATURE_TYPE); return 0; } - /* - * Suite B also requires P-256+SHA256 and P-384+SHA384: - * this matches the TLS 1.3 requirements so we can just - * check the curve is the expected TLS 1.3 value. - * If this fails an inappropriate digest is being used. - */ - if (curve != lu->curve) { - SSLerr(SSL_F_TLS12_CHECK_PEER_SIGALG, - SSL_R_ILLEGAL_SUITEB_DIGEST); - return 0; - } } } } else if (tls1_suiteb(s)) { -- 2.25.1