From a3ae248d6e35515f29a8a97a956d678a5cf9bb30 Mon Sep 17 00:00:00 2001 From: tb <> Date: Fri, 15 Jun 2018 19:24:13 +0000 Subject: Basic cleanup. Handle the possibly NULL ctx_in in ecdsa_sign_setup() with the usual idiom. All the allocations are now handled inside conditionals as is usually done in this part of the tree. Turn a few comments into actual sentences and remove a few self-evident ones. Change outdated or cryptic comments into more helpful annotations. In ecdsa_do_verify(), start calculating only after properly truncating the message digest. More consistent variable names: prefer 'order_bits' and 'point' over 'i' and 'tmp_point'. ok jsing --- src/lib/libcrypto/ecdsa/ecs_ossl.c | 129 ++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 67 deletions(-) diff --git a/src/lib/libcrypto/ecdsa/ecs_ossl.c b/src/lib/libcrypto/ecdsa/ecs_ossl.c index be279b34b6..eff2a5022d 100644 --- a/src/lib/libcrypto/ecdsa/ecs_ossl.c +++ b/src/lib/libcrypto/ecdsa/ecs_ossl.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ecs_ossl.c,v 1.13 2018/06/15 05:00:41 tb Exp $ */ +/* $OpenBSD: ecs_ossl.c,v 1.14 2018/06/15 19:24:13 tb Exp $ */ /* * Written by Nils Larsch for the OpenSSL project */ @@ -88,9 +88,9 @@ ECDSA_OpenSSL(void) static int ecdsa_sign_setup(EC_KEY *eckey, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) { - BN_CTX *ctx = NULL; - BIGNUM *k = NULL, *r = NULL, *order = NULL, *X = NULL; - EC_POINT *tmp_point = NULL; + BN_CTX *ctx = ctx_in; + BIGNUM *k = NULL, *r = NULL, *order = NULL, *X = NULL; + EC_POINT *point = NULL; const EC_GROUP *group; int order_bits, ret = 0; @@ -99,23 +99,19 @@ ecdsa_sign_setup(EC_KEY *eckey, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) return 0; } - if (ctx_in == NULL) { + if (ctx == NULL) { if ((ctx = BN_CTX_new()) == NULL) { ECDSAerror(ERR_R_MALLOC_FAILURE); return 0; } - } else - ctx = ctx_in; - - k = BN_new(); /* this value is later returned in *kinvp */ - r = BN_new(); /* this value is later returned in *rp */ - order = BN_new(); - X = BN_new(); - if (!k || !r || !order || !X) { + } + + if ((k = BN_new()) == NULL || (r = BN_new()) == NULL || + (order = BN_new()) == NULL || (X = BN_new()) == NULL) { ECDSAerror(ERR_R_MALLOC_FAILURE); goto err; } - if ((tmp_point = EC_POINT_new(group)) == NULL) { + if ((point = EC_POINT_new(group)) == NULL) { ECDSAerror(ERR_R_EC_LIB); goto err; } @@ -132,14 +128,13 @@ ecdsa_sign_setup(EC_KEY *eckey, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) goto err; do { - /* get random k */ - do + do { if (!BN_rand_range(k, order)) { ECDSAerror( ECDSA_R_RANDOM_NUMBER_GENERATION_FAILED); goto err; } - while (BN_is_zero(k)); + } while (BN_is_zero(k)); /* * We do not want timing information to leak the length of k, @@ -162,23 +157,23 @@ ecdsa_sign_setup(EC_KEY *eckey, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) BN_set_flags(k, BN_FLG_CONSTTIME); - /* compute r the x-coordinate of generator * k */ - if (!EC_POINT_mul(group, tmp_point, k, NULL, NULL, ctx)) { + /* Compute r, the x-coordinate of G * k. */ + if (!EC_POINT_mul(group, point, k, NULL, NULL, ctx)) { ECDSAerror(ERR_R_EC_LIB); goto err; } if (EC_METHOD_get_field_type(EC_GROUP_method_of(group)) == NID_X9_62_prime_field) { - if (!EC_POINT_get_affine_coordinates_GFp(group, - tmp_point, X, NULL, ctx)) { + if (!EC_POINT_get_affine_coordinates_GFp(group, point, + X, NULL, ctx)) { ECDSAerror(ERR_R_EC_LIB); goto err; } } #ifndef OPENSSL_NO_EC2M else { /* NID_X9_62_characteristic_two_field */ - if (!EC_POINT_get_affine_coordinates_GF2m(group, - tmp_point, X, NULL, ctx)) { + if (!EC_POINT_get_affine_coordinates_GF2m(group, point, + X, NULL, ctx)) { ECDSAerror(ERR_R_EC_LIB); goto err; } @@ -190,15 +185,12 @@ ecdsa_sign_setup(EC_KEY *eckey, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) } } while (BN_is_zero(r)); - /* compute the inverse of k */ if (!BN_mod_inverse_ct(k, k, order, ctx)) { ECDSAerror(ERR_R_BN_LIB); goto err; } - /* clear old values if necessary */ BN_clear_free(*rp); BN_clear_free(*kinvp); - /* save the pre-computed values */ *rp = r; *kinvp = k; ret = 1; @@ -211,7 +203,7 @@ ecdsa_sign_setup(EC_KEY *eckey, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp) if (ctx_in == NULL) BN_CTX_free(ctx); BN_free(order); - EC_POINT_free(tmp_point); + EC_POINT_free(point); BN_clear_free(X); return (ret); } @@ -228,7 +220,7 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, const EC_GROUP *group; ECDSA_SIG *ret; ECDSA_DATA *ecdsa; - int ok = 0, i; + int ok = 0, order_bits; ecdsa = ecdsa_check(eckey); group = EC_KEY_get0_group(eckey); @@ -239,8 +231,7 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, return NULL; } - ret = ECDSA_SIG_new(); - if (!ret) { + if ((ret = ECDSA_SIG_new()) == NULL) { ECDSAerror(ERR_R_MALLOC_FAILURE); return NULL; } @@ -258,18 +249,21 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, ECDSAerror(ERR_R_EC_LIB); goto err; } - i = BN_num_bits(order); + /* Truncate digest if it is too long: first truncate whole bytes. */ - if (8 * dgst_len > i) - dgst_len = (i + 7)/8; + order_bits = BN_num_bits(order); + if (8 * dgst_len > order_bits) + dgst_len = (order_bits + 7) / 8; if (!BN_bin2bn(dgst, dgst_len, m)) { ECDSAerror(ERR_R_BN_LIB); goto err; } /* If it is still too long, truncate the remaining bits with a shift. */ - if ((8 * dgst_len > i) && !BN_rshift(m, m, 8 - (i & 0x7))) { - ECDSAerror(ERR_R_BN_LIB); - goto err; + if (8 * dgst_len > order_bits) { + if (!BN_rshift(m, m, 8 - (order_bits & 0x7))) { + ECDSAerror(ERR_R_BN_LIB); + goto err; + } } do { @@ -359,7 +353,7 @@ ecdsa_do_sign(const unsigned char *dgst, int dgst_len, ok = 1; err: - if (!ok) { + if (ok == 0) { ECDSA_SIG_free(ret); ret = NULL; } @@ -379,22 +373,20 @@ static int ecdsa_do_verify(const unsigned char *dgst, int dgst_len, const ECDSA_SIG *sig, EC_KEY *eckey) { - int ret = -1, i; - BN_CTX *ctx; - BIGNUM *order, *u1, *u2, *m, *X; + BN_CTX *ctx; + BIGNUM *order, *u1, *u2, *m, *X; EC_POINT *point = NULL; const EC_GROUP *group; const EC_POINT *pub_key; + int order_bits, ret = -1; - /* check input values */ if (eckey == NULL || (group = EC_KEY_get0_group(eckey)) == NULL || (pub_key = EC_KEY_get0_public_key(eckey)) == NULL || sig == NULL) { ECDSAerror(ECDSA_R_MISSING_PARAMETERS); return -1; } - ctx = BN_CTX_new(); - if (!ctx) { + if ((ctx = BN_CTX_new()) == NULL) { ECDSAerror(ERR_R_MALLOC_FAILURE); return -1; } @@ -404,7 +396,7 @@ ecdsa_do_verify(const unsigned char *dgst, int dgst_len, const ECDSA_SIG *sig, u2 = BN_CTX_get(ctx); m = BN_CTX_get(ctx); X = BN_CTX_get(ctx); - if (!X) { + if (X == NULL) { ECDSAerror(ERR_R_BN_LIB); goto err; } @@ -414,43 +406,46 @@ ecdsa_do_verify(const unsigned char *dgst, int dgst_len, const ECDSA_SIG *sig, goto err; } - if (BN_is_zero(sig->r) || BN_is_negative(sig->r) || - BN_ucmp(sig->r, order) >= 0 || BN_is_zero(sig->s) || - BN_is_negative(sig->s) || BN_ucmp(sig->s, order) >= 0) { + /* Verify that r and s are in the range [1, order-1]. */ + if (BN_is_zero(sig->r) || BN_is_negative(sig->r) || + BN_ucmp(sig->r, order) >= 0 || + BN_is_zero(sig->s) || BN_is_negative(sig->s) || + BN_ucmp(sig->s, order) >= 0) { ECDSAerror(ECDSA_R_BAD_SIGNATURE); - ret = 0; /* signature is invalid */ - goto err; - } - /* calculate tmp1 = inv(S) mod order */ - if (!BN_mod_inverse_ct(u2, sig->s, order, ctx)) { - ECDSAerror(ERR_R_BN_LIB); + ret = 0; goto err; } - /* digest -> m */ - i = BN_num_bits(order); + /* Truncate digest if it is too long: first truncate whole bytes. */ - if (8 * dgst_len > i) - dgst_len = (i + 7)/8; + order_bits = BN_num_bits(order); + if (8 * dgst_len > order_bits) + dgst_len = (order_bits + 7) / 8; if (!BN_bin2bn(dgst, dgst_len, m)) { ECDSAerror(ERR_R_BN_LIB); goto err; } /* If it is still too long, truncate the remaining bits with a shift. */ - if ((8 * dgst_len > i) && !BN_rshift(m, m, 8 - (i & 0x7))) { + if (8 * dgst_len > order_bits) { + if (!BN_rshift(m, m, 8 - (order_bits & 0x7))) { + ECDSAerror(ERR_R_BN_LIB); + goto err; + } + } + + if (!BN_mod_inverse_ct(u2, sig->s, order, ctx)) { /* w = inv(s) */ ECDSAerror(ERR_R_BN_LIB); goto err; } - /* u1 = m * tmp mod order */ - if (!BN_mod_mul(u1, m, u2, order, ctx)) { + if (!BN_mod_mul(u1, m, u2, order, ctx)) { /* u1 = mw */ ECDSAerror(ERR_R_BN_LIB); goto err; } - /* u2 = r * w mod q */ - if (!BN_mod_mul(u2, sig->r, u2, order, ctx)) { + if (!BN_mod_mul(u2, sig->r, u2, order, ctx)) { /* u2 = rw */ ECDSAerror(ERR_R_BN_LIB); goto err; } + /* Compute the x-coordinate of G * u1 + pub_key * u2. */ if ((point = EC_POINT_new(group)) == NULL) { ECDSAerror(ERR_R_MALLOC_FAILURE); goto err; @@ -461,16 +456,16 @@ ecdsa_do_verify(const unsigned char *dgst, int dgst_len, const ECDSA_SIG *sig, } if (EC_METHOD_get_field_type(EC_GROUP_method_of(group)) == NID_X9_62_prime_field) { - if (!EC_POINT_get_affine_coordinates_GFp(group, - point, X, NULL, ctx)) { + if (!EC_POINT_get_affine_coordinates_GFp(group, point, X, NULL, + ctx)) { ECDSAerror(ERR_R_EC_LIB); goto err; } } #ifndef OPENSSL_NO_EC2M else { /* NID_X9_62_characteristic_two_field */ - if (!EC_POINT_get_affine_coordinates_GF2m(group, - point, X, NULL, ctx)) { + if (!EC_POINT_get_affine_coordinates_GF2m(group, point, X, NULL, + ctx)) { ECDSAerror(ERR_R_EC_LIB); goto err; } @@ -481,7 +476,7 @@ ecdsa_do_verify(const unsigned char *dgst, int dgst_len, const ECDSA_SIG *sig, goto err; } - /* If the signature is correct, then u1 is equal to sig->r. */ + /* If the signature is correct, the x-coordinate is equal to sig->r. */ ret = (BN_ucmp(u1, sig->r) == 0); err: -- cgit v1.2.3-55-g6feb