Simplify server-side ECDH curve selection.
There's multiple sets of APIs for selecting the curve. Fold away
SSL_OP_SINGLE_ECDH_USE as failing to set it is either a no-op or a bug. With
that gone, the consumer only needs to control the selection of a curve, with
key generation from then on being uniform. Also clean up the interaction
between the three API modes in s3_srvr.c; they were already mutually exclusive
due to tls1_check_ec_tmp_key.
This also removes all callers of EC_KEY_dup (and thus CRYPTO_dup_ex_data)
within the library.
Change-Id: I477b13bd9e77eb03d944ef631dd521639968dc8c
Reviewed-on: https://boringssl-review.googlesource.com/4200
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index d67d381..fe0e760 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -753,26 +753,14 @@
return ret;
case SSL_CTRL_SET_TMP_ECDH: {
- EC_KEY *ecdh = NULL;
-
- if (parg == NULL) {
+ /* For historical reasons, this API expects an |EC_KEY|, but only the
+ * group is used. */
+ EC_KEY *ec_key = (EC_KEY *)parg;
+ if (ec_key == NULL || EC_KEY_get0_group(ec_key) == NULL) {
OPENSSL_PUT_ERROR(SSL, ssl3_ctrl, ERR_R_PASSED_NULL_PARAMETER);
return ret;
}
- if (!EC_KEY_up_ref((EC_KEY *)parg)) {
- OPENSSL_PUT_ERROR(SSL, ssl3_ctrl, ERR_R_ECDH_LIB);
- return ret;
- }
- ecdh = (EC_KEY *)parg;
- if (!(s->options & SSL_OP_SINGLE_ECDH_USE) && !EC_KEY_generate_key(ecdh)) {
- EC_KEY_free(ecdh);
- OPENSSL_PUT_ERROR(SSL, ssl3_ctrl, ERR_R_ECDH_LIB);
- return ret;
- }
- if (s->cert->ecdh_tmp != NULL) {
- EC_KEY_free(s->cert->ecdh_tmp);
- }
- s->cert->ecdh_tmp = ecdh;
+ s->cert->ecdh_nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec_key));
ret = 1;
break;
}
@@ -1034,28 +1022,14 @@
return 0;
case SSL_CTRL_SET_TMP_ECDH: {
- EC_KEY *ecdh = NULL;
-
- if (parg == NULL) {
- OPENSSL_PUT_ERROR(SSL, ssl3_ctx_ctrl, ERR_R_ECDH_LIB);
+ /* For historical reasons, this API expects an |EC_KEY|, but only the
+ * group is used. */
+ EC_KEY *ec_key = (EC_KEY *)parg;
+ if (ec_key == NULL || EC_KEY_get0_group(ec_key) == NULL) {
+ OPENSSL_PUT_ERROR(SSL, ssl3_ctx_ctrl, ERR_R_PASSED_NULL_PARAMETER);
return 0;
}
- ecdh = EC_KEY_dup((EC_KEY *)parg);
- if (ecdh == NULL) {
- OPENSSL_PUT_ERROR(SSL, ssl3_ctx_ctrl, ERR_R_EC_LIB);
- return 0;
- }
- if (!(ctx->options & SSL_OP_SINGLE_ECDH_USE) &&
- !EC_KEY_generate_key(ecdh)) {
- EC_KEY_free(ecdh);
- OPENSSL_PUT_ERROR(SSL, ssl3_ctx_ctrl, ERR_R_ECDH_LIB);
- return 0;
- }
-
- if (cert->ecdh_tmp != NULL) {
- EC_KEY_free(cert->ecdh_tmp);
- }
- cert->ecdh_tmp = ecdh;
+ ctx->cert->ecdh_nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec_key));
return 1;
}
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index a3256d7..25482a2 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -1331,7 +1331,7 @@
int ssl3_send_server_key_exchange(SSL *s) {
DH *dh = NULL, *dhp;
- EC_KEY *ecdh = NULL, *ecdhp;
+ EC_KEY *ecdh = NULL;
uint8_t *encodedPoint = NULL;
int encodedlen = 0;
uint16_t curve_id = 0;
@@ -1415,19 +1415,21 @@
r[1] = dh->g;
r[2] = dh->pub_key;
} else if (alg_k & SSL_kECDHE) {
- const EC_GROUP *group;
-
- ecdhp = cert->ecdh_tmp;
- if (s->cert->ecdh_tmp_auto) {
- /* Get NID of appropriate shared curve */
- int nid = tls1_get_shared_curve(s);
- if (nid != NID_undef) {
- ecdhp = EC_KEY_new_by_curve_name(nid);
+ /* Determine the curve to use. */
+ int nid = NID_undef;
+ if (cert->ecdh_tmp_auto) {
+ nid = tls1_get_shared_curve(s);
+ } else if (cert->ecdh_nid != NID_undef) {
+ nid = cert->ecdh_nid;
+ } else if (cert->ecdh_tmp_cb != NULL) {
+ /* Note: |ecdh_tmp_cb| does NOT pass ownership of the result
+ * to the caller. */
+ EC_KEY *template = s->cert->ecdh_tmp_cb(s, 0, 1024);
+ if (template != NULL && EC_KEY_get0_group(template) != NULL) {
+ nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(template));
}
- } else if (ecdhp == NULL && s->cert->ecdh_tmp_cb) {
- ecdhp = s->cert->ecdh_tmp_cb(s, 0, 1024);
}
- if (ecdhp == NULL) {
+ if (nid == NID_undef) {
al = SSL_AD_HANDSHAKE_FAILURE;
OPENSSL_PUT_ERROR(SSL, ssl3_send_server_key_exchange,
SSL_R_MISSING_TMP_ECDH_KEY);
@@ -1439,42 +1441,19 @@
ERR_R_INTERNAL_ERROR);
goto err;
}
-
- /* Duplicate the ECDH structure. */
- if (ecdhp == NULL) {
- OPENSSL_PUT_ERROR(SSL, ssl3_send_server_key_exchange, ERR_R_ECDH_LIB);
+ ecdh = EC_KEY_new_by_curve_name(nid);
+ if (ecdh == NULL) {
goto err;
}
-
- if (s->cert->ecdh_tmp_auto) {
- ecdh = ecdhp;
- } else {
- ecdh = EC_KEY_dup(ecdhp);
- if (ecdh == NULL) {
- OPENSSL_PUT_ERROR(SSL, ssl3_send_server_key_exchange, ERR_R_ECDH_LIB);
- goto err;
- }
- }
-
s->s3->tmp.ecdh = ecdh;
- if (EC_KEY_get0_public_key(ecdh) == NULL ||
- EC_KEY_get0_private_key(ecdh) == NULL ||
- (s->options & SSL_OP_SINGLE_ECDH_USE)) {
- if (!EC_KEY_generate_key(ecdh)) {
- OPENSSL_PUT_ERROR(SSL, ssl3_send_server_key_exchange, ERR_R_ECDH_LIB);
- goto err;
- }
- }
- group = EC_KEY_get0_group(ecdh);
- if (group == NULL ||
- EC_KEY_get0_public_key(ecdh) == NULL ||
- EC_KEY_get0_private_key(ecdh) == NULL) {
+ if (!EC_KEY_generate_key(ecdh)) {
OPENSSL_PUT_ERROR(SSL, ssl3_send_server_key_exchange, ERR_R_ECDH_LIB);
goto err;
}
/* We only support ephemeral ECDH keys over named (not generic) curves. */
+ const EC_GROUP *group = EC_KEY_get0_group(ecdh);
if (!tls1_ec_nid2curve_id(&curve_id, EC_GROUP_get_curve_name(group))) {
OPENSSL_PUT_ERROR(SSL, ssl3_send_server_key_exchange,
SSL_R_UNSUPPORTED_ELLIPTIC_CURVE);
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index fbe38ec..185c12b 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -212,13 +212,7 @@
}
ret->dh_tmp_cb = cert->dh_tmp_cb;
- if (cert->ecdh_tmp) {
- ret->ecdh_tmp = EC_KEY_dup(cert->ecdh_tmp);
- if (ret->ecdh_tmp == NULL) {
- OPENSSL_PUT_ERROR(SSL, ssl_cert_dup, ERR_R_EC_LIB);
- goto err;
- }
- }
+ ret->ecdh_nid = cert->ecdh_nid;
ret->ecdh_tmp_cb = cert->ecdh_tmp_cb;
ret->ecdh_tmp_auto = cert->ecdh_tmp_auto;
@@ -324,9 +318,6 @@
if (c->dh_tmp) {
DH_free(c->dh_tmp);
}
- if (c->ecdh_tmp) {
- EC_KEY_free(c->ecdh_tmp);
- }
ssl_cert_clear_certs(c);
if (c->peer_sigalgs) {
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c81e061..b21b521 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2011,7 +2011,8 @@
dh_tmp = (c->dh_tmp != NULL || c->dh_tmp_cb != NULL);
- have_ecdh_tmp = (c->ecdh_tmp || c->ecdh_tmp_cb || c->ecdh_tmp_auto);
+ have_ecdh_tmp = (c->ecdh_nid != NID_undef || c->ecdh_tmp_cb != NULL ||
+ c->ecdh_tmp_auto);
rsa_enc = ssl_has_key(s, SSL_PKEY_RSA_ENC);
rsa_sign = ssl_has_key(s, SSL_PKEY_RSA_SIGN);
have_ecc_cert = ssl_has_key(s, SSL_PKEY_ECC);
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 13fdd91..ed62fab 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -434,7 +434,9 @@
DH *dh_tmp;
DH *(*dh_tmp_cb)(SSL *ssl, int is_export, int keysize);
- EC_KEY *ecdh_tmp;
+ /* ecdh_nid, if not |NID_undef|, is the NID of the curve to use for ephemeral
+ * ECDH keys. */
+ int ecdh_nid;
/* Callback for generating ephemeral ECDH keys */
EC_KEY *(*ecdh_tmp_cb)(SSL *ssl, int is_export, int keysize);
/* Select ECDH parameters automatically */
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 01d9875..0009d13 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -606,23 +606,22 @@
}
int tls1_check_ec_tmp_key(SSL *s) {
- uint16_t curve_id;
- EC_KEY *ec = s->cert->ecdh_tmp;
-
if (s->cert->ecdh_tmp_auto) {
- /* Need a shared curve */
+ /* If using |ecdh_tmp_auto|, ECDH is acceptable if there is a shared
+ * curve. */
return tls1_get_shared_curve(s) != NID_undef;
}
- if (!ec) {
- if (s->cert->ecdh_tmp_cb) {
- return 1;
- }
- return 0;
+ if (s->cert->ecdh_nid != NID_undef) {
+ /* If the curve is preconfigured, ECDH is acceptable if the peer supports
+ * the curve. */
+ uint16_t curve_id;
+ return tls1_ec_nid2curve_id(&curve_id, s->cert->ecdh_nid) &&
+ tls1_check_curve_id(s, curve_id);
}
- return tls1_curve_params_from_ec_key(&curve_id, NULL, ec) &&
- tls1_check_curve_id(s, curve_id);
+ /* Otherwise, assume the callback will provide an acceptable curve. */
+ return s->cert->ecdh_tmp_cb != NULL;
}
/* List of supported signature algorithms and hashes. Should make this
