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

1 files changed, 62 insertions, 67 deletions

diff --git a/lib/libcrypto/ecdsa/ecs_ossl.c b/lib/libcrypto/ecdsa/ecs_ossl.c
index be279b34b6a..eff2a5022df 100644
--- a/lib/libcrypto/ecdsa/ecs_ossl.c
+++ b/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: