From 70bd80a2367fc00e579be6ab041048b8ec930b1f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 27 Dec 2014 03:06:46 -0500 Subject: [PATCH] Remove constraints on curve ID values. The under 32 constraint is silly; it's to check for duplicate curves in library-supplied configuration. That API is new as of 1.0.2. It doesn't seem worth bothering; if the caller supplies a repeated value, may as well emit a repeated one and so be it. (Probably no one will ever call that function outside of maybe test code anyway.) While I'm here, remove the 0 constraint too. It's not likely to change, but removing the return value overload seems easier than keeping comments about it comments about it. Change-Id: I01d36dba1855873875bb5a0ec84b040199e0e9bc Reviewed-on: https://boringssl-review.googlesource.com/2844 Reviewed-by: Adam Langley --- ssl/s3_srvr.c | 24 +++++++++--------------- ssl/ssl_locl.h | 2 +- ssl/t1_lib.c | 33 ++++++--------------------------- 3 files changed, 16 insertions(+), 43 deletions(-) diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 88f462b2..b346d144 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -1377,7 +1377,7 @@ int ssl3_send_server_key_exchange(SSL *s) { EC_KEY *ecdh = NULL, *ecdhp; uint8_t *encodedPoint = NULL; int encodedlen = 0; - int curve_id = 0; + uint16_t curve_id = 0; BN_CTX *bn_ctx = NULL; const char *psk_identity_hint = NULL; size_t psk_identity_hint_len = 0; @@ -1517,10 +1517,8 @@ int ssl3_send_server_key_exchange(SSL *s) { goto err; } - /* We only support ephemeral ECDH keys over named (not generic) curves. - * For supported named curves, curve_id is non-zero. */ - curve_id = tls1_ec_nid2curve_id(EC_GROUP_get_curve_name(group)); - if (curve_id == 0) { + /* We only support ephemeral ECDH keys over named (not generic) curves. */ + 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); goto err; @@ -1617,18 +1615,14 @@ int ssl3_send_server_key_exchange(SSL *s) { * [1 byte CurveType], [2 byte CurveName] * [1 byte length of encoded point], followed by * the actual encoded point itself. */ - *p = NAMED_CURVE_TYPE; - p += 1; - *p = 0; - p += 1; - *p = curve_id; - p += 1; - *p = encodedlen; - p += 1; - memcpy((uint8_t *)p, (uint8_t *)encodedPoint, encodedlen); + *(p++) = NAMED_CURVE_TYPE; + *(p++) = (uint8_t)(curve_id >> 8); + *(p++) = (uint8_t)(curve_id & 0xff); + *(p++) = encodedlen; + memcpy(p, encodedPoint, encodedlen); + p += encodedlen; OPENSSL_free(encodedPoint); encodedPoint = NULL; - p += encodedlen; } /* not anonymous */ diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 7d02a2d7..a21d2401 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -919,7 +919,7 @@ int ssl_check_srvr_ecc_cert_and_alg(X509 *x, SSL *s); char ssl_early_callback_init(struct ssl_early_callback_ctx *ctx); int tls1_ec_curve_id2nid(uint16_t curve_id); -uint16_t tls1_ec_nid2curve_id(int nid); +int tls1_ec_nid2curve_id(uint16_t *out_curve_id, int nid); /* tls1_check_curve parses ECParameters out of |cbs|, modifying it. It * checks the curve is one of our preferences and writes the diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 224d636e..339c765f 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -354,16 +354,7 @@ struct tls_curve { int nid; }; -/* ECC curves from RFC4492. - * - * NOTE: tls1_ec_curve_id2nid and tls1_set_curves assume that - * - * (a) 0 is not a valid curve ID. - * - * (b) The largest curve ID is 31. - * - * Those implementations must be revised before adding support for curve IDs - * that break these assumptions. */ +/* ECC curves from RFC4492. */ static const struct tls_curve tls_curves[] = { {21, NID_secp224r1}, {23, NID_X9_62_prime256v1}, @@ -391,16 +382,14 @@ int tls1_ec_curve_id2nid(uint16_t curve_id) { return NID_undef; } -uint16_t tls1_ec_nid2curve_id(int nid) { +int tls1_ec_nid2curve_id(uint16_t *out_curve_id, int nid) { size_t i; for (i = 0; i < sizeof(tls_curves) / sizeof(tls_curves[0]); i++) { if (nid == tls_curves[i].nid) { - return tls_curves[i].curve_id; + *out_curve_id = tls_curves[i].curve_id; + return 1; } } - - /* Use 0 for a non-existent curve ID. Note: this assumes that curve - * ID 0 will never be allocated. */ return 0; } @@ -479,25 +468,16 @@ int tls1_set_curves(uint16_t **out_curve_ids, size_t *out_curve_ids_len, uint16_t *curve_ids; size_t i; - /* Bitmap of curves included to detect duplicates: only works - * while curve ids < 32. */ - uint32_t dup_list = 0; curve_ids = (uint16_t *)OPENSSL_malloc(ncurves * sizeof(uint16_t)); if (curve_ids == NULL) { return 0; } for (i = 0; i < ncurves; i++) { - uint32_t idmask; - uint16_t id; - id = tls1_ec_nid2curve_id(curves[i]); - idmask = ((uint32_t)1) << id; - if (!id || (dup_list & idmask)) { + if (!tls1_ec_nid2curve_id(&curve_ids[i], curves[i])) { OPENSSL_free(curve_ids); return 0; } - dup_list |= idmask; - curve_ids[i] = id; } if (*out_curve_ids) { @@ -529,8 +509,7 @@ static int tls1_curve_params_from_ec_key(uint16_t *out_curve_id, /* Determine curve ID */ nid = EC_GROUP_get_curve_name(grp); - id = tls1_ec_nid2curve_id(nid); - if (!id) { + if (!tls1_ec_nid2curve_id(&id, nid)) { return 0; }