From 5279ef57699d1de10cf287dc243dabd8efef34ce Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 9 Nov 2018 17:36:12 -0600 Subject: [PATCH] Clean up EC_POINT to byte conversions. With the allocations and BN_CTX gone, ECDH and point2oct are much, much shorter. Bug: 242 Change-Id: I3421822e94100f7eb2f5f2373df7fb3b3311365e Reviewed-on: https://boringssl-review.googlesource.com/c/33071 Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org Reviewed-by: Adam Langley --- crypto/ecdh_extra/ecdh_extra.c | 76 +++++++------------------------ crypto/fipsmodule/ec/ec.c | 31 +++++++++++++ crypto/fipsmodule/ec/internal.h | 11 +++++ crypto/fipsmodule/ec/oct.c | 81 +++++++++------------------------ crypto/fipsmodule/ecdh/ecdh.c | 62 ++++--------------------- 5 files changed, 90 insertions(+), 171 deletions(-) diff --git a/crypto/ecdh_extra/ecdh_extra.c b/crypto/ecdh_extra/ecdh_extra.c index eb321f7d..1e080995 100644 --- a/crypto/ecdh_extra/ecdh_extra.c +++ b/crypto/ecdh_extra/ecdh_extra.c @@ -69,7 +69,6 @@ #include #include -#include #include #include #include @@ -78,10 +77,10 @@ #include "../internal.h" -int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key, +int ECDH_compute_key(void *out, size_t out_len, const EC_POINT *pub_key, const EC_KEY *priv_key, void *(*kdf)(const void *in, size_t inlen, void *out, - size_t *outlen)) { + size_t *out_len)) { if (priv_key->priv_key == NULL) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_NO_PRIVATE_VALUE); return -1; @@ -93,74 +92,33 @@ int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key, return -1; } - BN_CTX *ctx = BN_CTX_new(); - if (ctx == NULL) { - return -1; - } - BN_CTX_start(ctx); - - int ret = -1; - size_t buflen = 0; - uint8_t *buf = NULL; - - EC_POINT *tmp = EC_POINT_new(group); - if (tmp == NULL) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); - goto err; - } - - if (!ec_point_mul_scalar(group, &tmp->raw, NULL, &pub_key->raw, priv)) { - OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE); - goto err; - } - - BIGNUM *x = BN_CTX_get(ctx); - if (!x) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); - goto err; - } - - if (!EC_POINT_get_affine_coordinates_GFp(group, tmp, x, NULL, ctx)) { + EC_RAW_POINT shared_point; + uint8_t buf[EC_MAX_BYTES]; + size_t buf_len; + if (!ec_point_mul_scalar(group, &shared_point, NULL, &pub_key->raw, priv) || + !ec_point_get_affine_coordinate_bytes(group, buf, NULL, &buf_len, + sizeof(buf), &shared_point)) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE); - goto err; - } - - buflen = (EC_GROUP_get_degree(group) + 7) / 8; - buf = OPENSSL_malloc(buflen); - if (buf == NULL) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); - goto err; - } - - if (!BN_bn2bin_padded(buf, buflen, x)) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_INTERNAL_ERROR); - goto err; + return -1; } if (kdf != NULL) { - if (kdf(buf, buflen, out, &outlen) == NULL) { + if (kdf(buf, buf_len, out, &out_len) == NULL) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_KDF_FAILED); - goto err; + return -1; } } else { // no KDF, just copy as much as we can - if (buflen < outlen) { - outlen = buflen; + if (buf_len < out_len) { + out_len = buf_len; } - OPENSSL_memcpy(out, buf, outlen); + OPENSSL_memcpy(out, buf, out_len); } - if (outlen > INT_MAX) { + if (out_len > INT_MAX) { OPENSSL_PUT_ERROR(ECDH, ERR_R_OVERFLOW); - goto err; + return -1; } - ret = (int)outlen; - -err: - OPENSSL_free(buf); - EC_POINT_free(tmp); - BN_CTX_end(ctx); - BN_CTX_free(ctx); - return ret; + return (int)out_len; } diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c index 717e0544..bd0662a7 100644 --- a/crypto/fipsmodule/ec/ec.c +++ b/crypto/fipsmodule/ec/ec.c @@ -973,6 +973,37 @@ int ec_get_x_coordinate_as_scalar(const EC_GROUP *group, EC_SCALAR *out, return 1; } +int ec_point_get_affine_coordinate_bytes(const EC_GROUP *group, uint8_t *out_x, + uint8_t *out_y, size_t *out_len, + size_t max_out, + const EC_RAW_POINT *p) { + size_t len = BN_num_bytes(&group->field); + assert(len <= EC_MAX_BYTES); + if (max_out < len) { + OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL); + return 0; + } + + EC_FELEM x, y; + if (!group->meth->point_get_affine_coordinates( + group, p, out_x == NULL ? NULL : &x, out_y == NULL ? NULL : &y)) { + return 0; + } + + if (out_x != NULL) { + for (size_t i = 0; i < len; i++) { + out_x[i] = x.bytes[len - i - 1]; + } + } + if (out_y != NULL) { + for (size_t i = 0; i < len; i++) { + out_y[i] = y.bytes[len - i - 1]; + } + } + *out_len = len; + return 1; +} + void EC_GROUP_set_asn1_flag(EC_GROUP *group, int flag) {} const EC_METHOD *EC_GROUP_method_of(const EC_GROUP *group) { diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index a34ae98d..d78d719c 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h @@ -354,6 +354,17 @@ int ec_cmp_x_coordinate(const EC_GROUP *group, const EC_RAW_POINT *p, int ec_get_x_coordinate_as_scalar(const EC_GROUP *group, EC_SCALAR *out, const EC_RAW_POINT *p); +// ec_point_get_affine_coordinate_bytes writes |p|'s affine coordinates to +// |out_x| and |out_y|, each of which must have at must |max_out| bytes. It sets +// |*out_len| to the number of bytes written in each buffer. Coordinates are +// written big-endian and zero-padded to the size of the field. +// +// Either of |out_x| or |out_y| may be NULL to omit that coordinate. This +// function returns one on success and zero on failure. +int ec_point_get_affine_coordinate_bytes(const EC_GROUP *group, uint8_t *out_x, + uint8_t *out_y, size_t *out_len, + size_t max_out, const EC_RAW_POINT *p); + // ec_field_element_to_scalar reduces |r| modulo |group->order|. |r| must // previously have been reduced modulo |group->field|. int ec_field_element_to_scalar(const EC_GROUP *group, BIGNUM *r); diff --git a/crypto/fipsmodule/ec/oct.c b/crypto/fipsmodule/ec/oct.c index 19e17a7c..04b1f2cc 100644 --- a/crypto/fipsmodule/ec/oct.c +++ b/crypto/fipsmodule/ec/oct.c @@ -74,22 +74,18 @@ static size_t ec_GFp_simple_point2oct(const EC_GROUP *group, - const EC_POINT *point, + const EC_RAW_POINT *point, point_conversion_form_t form, - uint8_t *buf, size_t len, BN_CTX *ctx) { - size_t ret = 0; - BN_CTX *new_ctx = NULL; - int used_ctx = 0; - - if ((form != POINT_CONVERSION_COMPRESSED) && - (form != POINT_CONVERSION_UNCOMPRESSED)) { + uint8_t *buf, size_t len) { + if (form != POINT_CONVERSION_COMPRESSED && + form != POINT_CONVERSION_UNCOMPRESSED) { OPENSSL_PUT_ERROR(EC, EC_R_INVALID_FORM); - goto err; + return 0; } - if (EC_POINT_is_at_infinity(group, point)) { + if (ec_GFp_simple_is_at_infinity(group, point)) { OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY); - goto err; + return 0; } const size_t field_len = BN_num_bytes(&group->field); @@ -103,64 +99,31 @@ static size_t ec_GFp_simple_point2oct(const EC_GROUP *group, if (buf != NULL) { if (len < output_len) { OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL); - goto err; - } - - if (ctx == NULL) { - ctx = new_ctx = BN_CTX_new(); - if (ctx == NULL) { - goto err; - } + return 0; } - BN_CTX_start(ctx); - used_ctx = 1; - BIGNUM *x = BN_CTX_get(ctx); - BIGNUM *y = BN_CTX_get(ctx); - if (y == NULL) { - goto err; + uint8_t y_buf[EC_MAX_BYTES]; + size_t field_len_out; + if (!ec_point_get_affine_coordinate_bytes( + group, buf + 1 /* x */, + form == POINT_CONVERSION_COMPRESSED ? y_buf : buf + 1 + field_len, + &field_len_out, field_len, point)) { + return 0; } - if (!EC_POINT_get_affine_coordinates_GFp(group, point, x, y, ctx)) { - goto err; + if (field_len_out != field_len) { + OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR); + return 0; } - if ((form == POINT_CONVERSION_COMPRESSED) && - BN_is_odd(y)) { - buf[0] = form + 1; + if (form == POINT_CONVERSION_COMPRESSED) { + buf[0] = form + (y_buf[field_len - 1] & 1); } else { buf[0] = form; } - size_t i = 1; - - if (!BN_bn2bin_padded(buf + i, field_len, x)) { - OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR); - goto err; - } - i += field_len; - - if (form == POINT_CONVERSION_UNCOMPRESSED) { - if (!BN_bn2bin_padded(buf + i, field_len, y)) { - OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR); - goto err; - } - i += field_len; - } - - if (i != output_len) { - OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR); - goto err; - } } - ret = output_len; - -err: - if (used_ctx) { - BN_CTX_end(ctx); - } - BN_CTX_free(new_ctx); - return ret; + return output_len; } static int ec_GFp_simple_oct2point(const EC_GROUP *group, EC_POINT *point, @@ -263,7 +226,7 @@ size_t EC_POINT_point2oct(const EC_GROUP *group, const EC_POINT *point, OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS); return 0; } - return ec_GFp_simple_point2oct(group, point, form, buf, len, ctx); + return ec_GFp_simple_point2oct(group, &point->raw, form, buf, len); } int EC_POINT_set_compressed_coordinates_GFp(const EC_GROUP *group, diff --git a/crypto/fipsmodule/ecdh/ecdh.c b/crypto/fipsmodule/ecdh/ecdh.c index 19d12c98..b9dc2374 100644 --- a/crypto/fipsmodule/ecdh/ecdh.c +++ b/crypto/fipsmodule/ecdh/ecdh.c @@ -66,10 +66,8 @@ #include -#include #include -#include #include #include #include @@ -92,49 +90,14 @@ int ECDH_compute_key_fips(uint8_t *out, size_t out_len, const EC_POINT *pub_key, return 0; } - BN_CTX *ctx = BN_CTX_new(); - if (ctx == NULL) { - return 0; - } - BN_CTX_start(ctx); - - int ret = 0; - size_t buflen = 0; - uint8_t *buf = NULL; - - EC_POINT *shared_point = EC_POINT_new(group); - if (shared_point == NULL) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); - goto err; - } - - if (!ec_point_mul_scalar(group, &shared_point->raw, NULL, &pub_key->raw, - priv)) { + EC_RAW_POINT shared_point; + uint8_t buf[EC_MAX_BYTES]; + size_t buflen; + if (!ec_point_mul_scalar(group, &shared_point, NULL, &pub_key->raw, priv) || + !ec_point_get_affine_coordinate_bytes(group, buf, NULL, &buflen, + sizeof(buf), &shared_point)) { OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE); - goto err; - } - - BIGNUM *x = BN_CTX_get(ctx); - if (!x) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); - goto err; - } - - if (!EC_POINT_get_affine_coordinates_GFp(group, shared_point, x, NULL, ctx)) { - OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE); - goto err; - } - - buflen = (EC_GROUP_get_degree(group) + 7) / 8; - buf = OPENSSL_malloc(buflen); - if (buf == NULL) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE); - goto err; - } - - if (!BN_bn2bin_padded(buf, buflen, x)) { - OPENSSL_PUT_ERROR(ECDH, ERR_R_INTERNAL_ERROR); - goto err; + return 0; } switch (out_len) { @@ -152,15 +115,8 @@ int ECDH_compute_key_fips(uint8_t *out, size_t out_len, const EC_POINT *pub_key, break; default: OPENSSL_PUT_ERROR(ECDH, ECDH_R_UNKNOWN_DIGEST_LENGTH); - goto err; + return 0; } - ret = 1; - -err: - OPENSSL_free(buf); - EC_POINT_free(shared_point); - BN_CTX_end(ctx); - BN_CTX_free(ctx); - return ret; + return 1; }