Rewrite EC_GROUP_cmp()

Use better variable names (cf. https://jmilne.org/math/tips.html#4) and
avoid the weird style of assigning to r (what does r stand for anyway?)
and short circuiting subsequent tests using if (r || ...). Also, do not
reuse the variables for order and cofactor that were previously used for
the curve coefficients.

ok jsing

1 files changed, 75 insertions, 51 deletions

diff --git a/lib/libcrypto/ec/ec_lib.c b/lib/libcrypto/ec/ec_lib.c
index d61dea9f125..01e21e9eb83 100644
--- a/lib/libcrypto/ec/ec_lib.c
+++ b/lib/libcrypto/ec/ec_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec_lib.c,v 1.85 2024/11/08 13:55:45 tb Exp $ */
+/* $OpenBSD: ec_lib.c,v 1.86 2024/11/17 06:33:35 tb Exp $ */
 /*
  * Originally written by Bodo Moeller for the OpenSSL project.
  */
@@ -662,77 +662,101 @@ EC_GROUP_check(const EC_GROUP *group, BN_CTX *ctx_in)
 }
 LCRYPTO_ALIAS(EC_GROUP_check);
 
+/*
+ * Returns -1 on error, 0 if the groups are equal, 1 if they are distinct.
+ */
 int
-EC_GROUP_cmp(const EC_GROUP *a, const EC_GROUP *b, BN_CTX *ctx)
+EC_GROUP_cmp(const EC_GROUP *group1, const EC_GROUP *group2, BN_CTX *ctx_in)
 {
-	int r = 0;
-	BIGNUM *a1, *a2, *a3, *b1, *b2, *b3;
-	BN_CTX *ctx_new = NULL;
-
-	/* compare the field types */
-	if (ec_group_get_field_type(a) != ec_group_get_field_type(b))
-		return 1;
-	/* compare the curve name (if present in both) */
-	if (EC_GROUP_get_curve_name(a) && EC_GROUP_get_curve_name(b) &&
-	    EC_GROUP_get_curve_name(a) != EC_GROUP_get_curve_name(b))
-		return 1;
+	BN_CTX *ctx = NULL;
+	BIGNUM *p1, *a1, *b1, *p2, *a2, *b2;
+	const EC_POINT *generator1, *generator2;
+	const BIGNUM *order1, *order2, *cofactor1, *cofactor2;
+	int nid1, nid2;
+	int cmp = 1;
+	int ret = -1;
 
-	if (!ctx)
-		ctx_new = ctx = BN_CTX_new();
-	if (!ctx)
-		return -1;
+	if ((ctx = ctx_in) == NULL)
+		ctx = BN_CTX_new();
+	if (ctx == NULL)
+		goto err;
 
 	BN_CTX_start(ctx);
+
+	if (ec_group_get_field_type(group1) != ec_group_get_field_type(group2))
+		goto distinct;
+	if ((nid1 = EC_GROUP_get_curve_name(group1)) != NID_undef &&
+	    (nid2 = EC_GROUP_get_curve_name(group2)) != NID_undef) {
+		if (nid1 != nid2)
+			goto distinct;
+	}
+
+	if ((p1 = BN_CTX_get(ctx)) == NULL)
+		goto err;
 	if ((a1 = BN_CTX_get(ctx)) == NULL)
 		goto err;
-	if ((a2 = BN_CTX_get(ctx)) == NULL)
+	if ((b1 = BN_CTX_get(ctx)) == NULL)
 		goto err;
-	if ((a3 = BN_CTX_get(ctx)) == NULL)
+	if ((p2 = BN_CTX_get(ctx)) == NULL)
 		goto err;
-	if ((b1 = BN_CTX_get(ctx)) == NULL)
+	if ((a2 = BN_CTX_get(ctx)) == NULL)
 		goto err;
 	if ((b2 = BN_CTX_get(ctx)) == NULL)
 		goto err;
-	if ((b3 = BN_CTX_get(ctx)) == NULL)
+
+	/*
+	 * If we ever support curves in non-Weierstrass form, this check needs
+	 * to be adjusted. The comparison of the generators will fail anyway.
+	 */
+	if (!EC_GROUP_get_curve(group1, p1, a1, b1, ctx))
+		goto err;
+	if (!EC_GROUP_get_curve(group2, p2, a2, b2, ctx))
+		goto err;
+
+	if (BN_cmp(p1, p2) != 0 || BN_cmp(a1, a2) != 0 || BN_cmp(b1, b2) != 0)
+		goto distinct;
+
+	if ((generator1 = EC_GROUP_get0_generator(group1)) == NULL)
+		goto err;
+	if ((generator2 = EC_GROUP_get0_generator(group2)) == NULL)
 		goto err;
 
 	/*
-	 * XXX This approach assumes that the external representation of
-	 * curves over the same field type is the same.
+	 * It does not matter whether group1 or group2 is used: both points must
+	 * have a matching method for this to succeed.
 	 */
-	if (!a->meth->group_get_curve(a, a1, a2, a3, ctx) ||
-	    !b->meth->group_get_curve(b, b1, b2, b3, ctx))
-		r = 1;
-
-	if (r || BN_cmp(a1, b1) || BN_cmp(a2, b2) || BN_cmp(a3, b3))
-		r = 1;
-
-	/* XXX EC_POINT_cmp() assumes that the methods are equal */
-	if (r || EC_POINT_cmp(a, EC_GROUP_get0_generator(a),
-		EC_GROUP_get0_generator(b), ctx))
-		r = 1;
-
-	if (!r) {
-		/* compare the order and cofactor */
-		if (!EC_GROUP_get_order(a, a1, ctx) ||
-		    !EC_GROUP_get_order(b, b1, ctx) ||
-		    !EC_GROUP_get_cofactor(a, a2, ctx) ||
-		    !EC_GROUP_get_cofactor(b, b2, ctx))
-			goto err;
-		if (BN_cmp(a1, b1) || BN_cmp(a2, b2))
-			r = 1;
-	}
-	BN_CTX_end(ctx);
-	if (ctx_new)
-		BN_CTX_free(ctx);
+	if ((cmp = EC_POINT_cmp(group1, generator1, generator2, ctx)) < 0)
+		goto err;
+	if (cmp == 1)
+		goto distinct;
+	cmp = 1;
+
+	if ((order1 = EC_GROUP_get0_order(group1)) == NULL)
+		goto err;
+	if ((order2 = EC_GROUP_get0_order(group2)) == NULL)
+		goto err;
+
+	if ((cofactor1 = EC_GROUP_get0_cofactor(group1)) == NULL)
+		goto err;
+	if ((cofactor2 = EC_GROUP_get0_cofactor(group2)) == NULL)
+		goto err;
+
+	if (BN_cmp(order1, order2) != 0 || BN_cmp(cofactor1, cofactor2) != 0)
+		goto distinct;
 
-	return r;
+	/* All parameters match: the groups are equal. */
+	cmp = 0;
+
+ distinct:
+	ret = cmp;
 
  err:
 	BN_CTX_end(ctx);
-	if (ctx_new)
+
+	if (ctx != ctx_in)
 		BN_CTX_free(ctx);
-	return -1;
+
+	return ret;
 }
 LCRYPTO_ALIAS(EC_GROUP_cmp);