From 6977e8ee4a718a76351ba5275a9f0be4e530eab5 Mon Sep 17 00:00:00 2001 From: Kurt Roeckx Date: Fri, 4 Dec 2015 22:25:11 +0100 Subject: [PATCH] Make SSL_{CTX}_set_tmp_ecdh() call SSL_{CTX_}set1_curves() SSL_{CTX}_set_tmp_ecdh() allows to set 1 EC curve and then tries to use it. On the other hand SSL_{CTX_}set1_curves() allows you to set a list of curves, but only when SSL_{CTX_}set_ecdh_auto() was called to turn it on. Reviewed-by: Dr. Stephen Henson --- CHANGES | 4 +++ ssl/s3_lib.c | 57 ++++++++++++++++++---------------------- ssl/ssl_cert.c | 10 ------- ssl/ssl_lib.c | 2 +- ssl/ssl_locl.h | 1 - ssl/statem/statem_srvr.c | 2 +- ssl/t1_lib.c | 43 +++++++++++------------------- test/ssltest.c | 1 + 8 files changed, 48 insertions(+), 72 deletions(-) diff --git a/CHANGES b/CHANGES index b365cb0ad1..21d95f18c8 100644 --- a/CHANGES +++ b/CHANGES @@ -13,6 +13,10 @@ pages. This work was developed in partnership with Intel Corp. [Matt Caswell] + *) SSL_{CTX}_set_tmp_ecdh() which can set 1 EC curve now internally calls + SSL_{CTX_}set1_curves() which can set a list. + [Kurt Roeckx] + *) Remove support for SSL_{CTX_}set_tmp_ecdh_callback(). You should set the curve you want to support using SSL_{CTX_}set1_curves(). [Kurt Roeckx] diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 0df228e50a..e9a7b47668 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -4072,27 +4072,24 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) #ifndef OPENSSL_NO_EC case SSL_CTRL_SET_TMP_ECDH: { - EC_KEY *ecdh = NULL; + const EC_GROUP *group = NULL; + int nid; if (parg == NULL) { SSLerr(SSL_F_SSL3_CTRL, ERR_R_PASSED_NULL_PARAMETER); - return (ret); + return 0; } - if (!EC_KEY_up_ref((EC_KEY *)parg)) { - SSLerr(SSL_F_SSL3_CTRL, ERR_R_ECDH_LIB); - return (ret); + group = EC_KEY_get0_group((const EC_KEY *)parg); + if (group == NULL) { + SSLerr(SSL_F_SSL3_CTRL, EC_R_MISSING_PARAMETERS); + return 0; } - ecdh = (EC_KEY *)parg; - if (!(s->options & SSL_OP_SINGLE_ECDH_USE)) { - if (!EC_KEY_generate_key(ecdh)) { - EC_KEY_free(ecdh); - SSLerr(SSL_F_SSL3_CTRL, ERR_R_ECDH_LIB); - return (ret); - } - } - EC_KEY_free(s->cert->ecdh_tmp); - s->cert->ecdh_tmp = ecdh; - ret = 1; + nid = EC_GROUP_get_curve_name(group); + if (nid == NID_undef) + return 0; + return tls1_set_curves(&s->tlsext_ellipticcurvelist, + &s->tlsext_ellipticcurvelist_length, + &nid, 1); } break; #endif /* !OPENSSL_NO_EC */ @@ -4522,28 +4519,24 @@ long ssl3_ctx_ctrl(SSL_CTX *ctx, int cmd, long larg, void *parg) #ifndef OPENSSL_NO_EC case SSL_CTRL_SET_TMP_ECDH: { - EC_KEY *ecdh = NULL; + const EC_GROUP *group = NULL; + int nid; if (parg == NULL) { - SSLerr(SSL_F_SSL3_CTX_CTRL, ERR_R_ECDH_LIB); + SSLerr(SSL_F_SSL3_CTX_CTRL, ERR_R_PASSED_NULL_PARAMETER); return 0; } - ecdh = EC_KEY_dup((EC_KEY *)parg); - if (ecdh == NULL) { - SSLerr(SSL_F_SSL3_CTX_CTRL, ERR_R_EC_LIB); + group = EC_KEY_get0_group((const EC_KEY *)parg); + if (group == NULL) { + SSLerr(SSL_F_SSL3_CTX_CTRL, EC_R_MISSING_PARAMETERS); return 0; } - if (!(ctx->options & SSL_OP_SINGLE_ECDH_USE)) { - if (!EC_KEY_generate_key(ecdh)) { - EC_KEY_free(ecdh); - SSLerr(SSL_F_SSL3_CTX_CTRL, ERR_R_ECDH_LIB); - return 0; - } - } - - EC_KEY_free(cert->ecdh_tmp); - cert->ecdh_tmp = ecdh; - return 1; + nid = EC_GROUP_get_curve_name(group); + if (nid == NID_undef) + return 0; + return tls1_set_curves(&ctx->tlsext_ellipticcurvelist, + &ctx->tlsext_ellipticcurvelist_length, + &nid, 1); } /* break; */ #endif /* !OPENSSL_NO_EC */ diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index 45b1d164ba..802b1141c1 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c @@ -232,13 +232,6 @@ CERT *ssl_cert_dup(CERT *cert) #endif #ifndef OPENSSL_NO_EC - if (cert->ecdh_tmp) { - ret->ecdh_tmp = EC_KEY_dup(cert->ecdh_tmp); - if (ret->ecdh_tmp == NULL) { - SSLerr(SSL_F_SSL_CERT_DUP, ERR_R_EC_LIB); - goto err; - } - } ret->ecdh_tmp_auto = cert->ecdh_tmp_auto; #endif @@ -394,9 +387,6 @@ void ssl_cert_free(CERT *c) #ifndef OPENSSL_NO_DH DH_free(c->dh_tmp); #endif -#ifndef OPENSSL_NO_EC - EC_KEY_free(c->ecdh_tmp); -#endif ssl_cert_clear_certs(c); OPENSSL_free(c->conf_sigalgs); diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 9343e7dff2..d598f91eb7 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2037,7 +2037,7 @@ void ssl_set_masks(SSL *s, const SSL_CIPHER *cipher) #endif #ifndef OPENSSL_NO_EC - have_ecdh_tmp = (c->ecdh_tmp || c->ecdh_tmp_auto); + have_ecdh_tmp = c->ecdh_tmp_auto; #endif cpk = &(c->pkeys[SSL_PKEY_RSA_ENC]); rsa_enc = pvalid[SSL_PKEY_RSA_ENC] & CERT_PKEY_VALID; diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 2da47f1d35..a8789325a4 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1569,7 +1569,6 @@ typedef struct cert_st { int dh_tmp_auto; # endif # ifndef OPENSSL_NO_EC - EC_KEY *ecdh_tmp; /* Select ECDH parameters automatically */ int ecdh_tmp_auto; # endif diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index fb64106350..693c265789 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1868,7 +1868,7 @@ int tls_construct_server_key_exchange(SSL *s) if (type & (SSL_kECDHE | SSL_kECDHEPSK)) { const EC_GROUP *group; - ecdhp = cert->ecdh_tmp; + ecdhp = NULL; if (s->cert->ecdh_tmp_auto) { /* Get NID of appropriate shared curve */ int nid = tls1_shared_curve(s, -2); diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 951be10d2d..6a9dc5db28 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -507,8 +507,9 @@ int tls1_check_curve(SSL *s, const unsigned char *p, size_t len) } /*- - * Return |nmatch|th shared curve or NID_undef if there is no match. - * For nmatch == -1, return number of matches + * For nmatch >= 0, return the NID of the |nmatch|th shared curve or NID_undef + * if there is no match. + * For nmatch == -1, return number of matches * For nmatch == -2, return the NID of the curve to use for * an EC tmp key, or NID_undef if there is no match. */ @@ -842,11 +843,18 @@ static int tls1_check_cert_param(SSL *s, X509 *x, int set_ee_md) } # ifndef OPENSSL_NO_EC -/* Check EC temporary key is compatible with client extensions */ +/* + * tls1_check_ec_tmp_key - Check EC temporary key compatiblity + * @s: SSL connection + * @cid: Cipher ID we're considering using + * + * Checks that the kECDHE cipher suite we're considering using + * is compatible with the client extensions. + * + * Returns 0 when the cipher can't be used or 1 when it can. + */ int tls1_check_ec_tmp_key(SSL *s, unsigned long cid) { - unsigned char curve_id[2]; - EC_KEY *ec = s->cert->ecdh_tmp; # ifdef OPENSSL_SSL_DEBUG_BROKEN_PROTOCOL /* Allow any curve: not just those peer supports */ if (s->cert->cert_flags & SSL_CERT_FLAG_BROKEN_PROTOCOL) @@ -857,6 +865,7 @@ int tls1_check_ec_tmp_key(SSL *s, unsigned long cid) * curves permitted. */ if (tls1_suiteb(s)) { + unsigned char curve_id[2]; /* Curve to check determined by ciphersuite */ if (cid == TLS1_CK_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256) curve_id[1] = TLSEXT_curve_P_256; @@ -871,18 +880,8 @@ int tls1_check_ec_tmp_key(SSL *s, unsigned long cid) /* If auto assume OK */ if (s->cert->ecdh_tmp_auto) return 1; - /* Otherwise check curve is acceptable */ - else { - unsigned char curve_tmp[2]; - if (!ec) - return 0; - if (!tls1_set_ec_id(curve_tmp, NULL, ec)) - return 0; - if (!curve_tmp[0] || curve_tmp[1] == curve_id[1]) - return 1; + else return 0; - } - } if (s->cert->ecdh_tmp_auto) { /* Need a shared curve */ @@ -891,17 +890,7 @@ int tls1_check_ec_tmp_key(SSL *s, unsigned long cid) else return 0; } - if (!ec) { - return 0; - } - if (!tls1_set_ec_id(curve_id, NULL, ec)) - return 0; -/* Set this to allow use of invalid curves for testing */ -# if 0 - return 1; -# else - return tls1_check_ec_key(s, curve_id, NULL); -# endif + return 0; } # endif /* OPENSSL_NO_EC */ diff --git a/test/ssltest.c b/test/ssltest.c index 68d48d1d73..6455af3fbf 100644 --- a/test/ssltest.c +++ b/test/ssltest.c @@ -1475,6 +1475,7 @@ int main(int argc, char *argv[]) goto end; } + SSL_CTX_set_ecdh_auto(s_ctx, 1); SSL_CTX_set_tmp_ecdh(s_ctx, ecdh); SSL_CTX_set_options(s_ctx, SSL_OP_SINGLE_ECDH_USE); EC_KEY_free(ecdh);