From: jsing <jsing@openbsd.org>
Date: Wed, 8 Mar 2023 05:35:51 +0000 (+0000)
Subject: Stop trying to use EC_GFp_nist_method().
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=a318884f78c5622f958105f6b7ce2a3dda3bac4c;p=openbsd

Stop trying to use EC_GFp_nist_method().

Currently, if compiled without OPENSSL_BN_ASM_MONT,
EC_GROUP_new_curve_GFp() tries to use EC_GFp_nist_method(), falling back to
EC_GFp_mont_method() if it is not a NIST curve (if OPENSSL_BN_ASM_MONT is
defined we use EC_GFp_mont_method() unconditionally).

Now that we have a reasonable non-assembly Montgomery implementation, the
performance of EC_GFp_nist_method() is either similar or slower than
EC_GFp_mont_method() (the exception being P-521, however if you're using
that you're not doing it for performance reasons anyway).

The EC_GFp_nist_method() uses rather scary BN NIST code (which would
probably already be removed, if not for the BN and EC public APIs), it uses
code paths that are currently less constant time, and there is additional
overhead in checking to see if the curve is actually supported.

Stop trying to use EC_GFp_nist_method() and unconditionally use
EC_GFp_mont_method() in all cases. While here, factor out the common
setup code and call it from both EC_GROUP_new_curve_GFp() and
EC_GROUP_new_curve_GF2m().

ok beck@ tb@
---

diff --git a/lib/libcrypto/ec/ec_cvt.c b/lib/libcrypto/ec/ec_cvt.c
index 365ca1aa7e3..fff9ab99cbd 100644
--- a/lib/libcrypto/ec/ec_cvt.c
+++ b/lib/libcrypto/ec/ec_cvt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ec_cvt.c,v 1.8 2022/11/26 16:08:52 tb Exp $ */
+/* $OpenBSD: ec_cvt.c,v 1.9 2023/03/08 05:35:51 jsing Exp $ */
 /*
  * Originally written by Bodo Moeller for the OpenSSL project.
  */
@@ -74,74 +74,31 @@
 #include <openssl/err.h>
 #include "ec_local.h"
 
-EC_GROUP *
-EC_GROUP_new_curve_GFp(const BIGNUM *p, const BIGNUM *a, const BIGNUM *b,
-    BN_CTX *ctx)
+static EC_GROUP *
+ec_group_new_curve(const EC_METHOD *method, const BIGNUM *p, const BIGNUM *a,
+    const BIGNUM *b, BN_CTX *ctx)
 {
-	const EC_METHOD *meth;
-	EC_GROUP *ret;
-
-#if defined(OPENSSL_BN_ASM_MONT)
-	/*
-	 * This might appear controversial, but the fact is that generic
-	 * prime method was observed to deliver better performance even
-	 * for NIST primes on a range of platforms, e.g.: 60%-15%
-	 * improvement on IA-64, ~25% on ARM, 30%-90% on P4, 20%-25%
-	 * in 32-bit build and 35%--12% in 64-bit build on Core2...
-	 * Coefficients are relative to optimized bn_nist.c for most
-	 * intensive ECDSA verify and ECDH operations for 192- and 521-
-	 * bit keys respectively. Choice of these boundary values is
-	 * arguable, because the dependency of improvement coefficient
-	 * from key length is not a "monotone" curve. For example while
-	 * 571-bit result is 23% on ARM, 384-bit one is -1%. But it's
-	 * generally faster, sometimes "respectfully" faster, sometimes
-	 * "tolerably" slower... What effectively happens is that loop
-	 * with bn_mul_add_words is put against bn_mul_mont, and the
-	 * latter "wins" on short vectors. Correct solution should be
-	 * implementing dedicated NxN multiplication subroutines for
-	 * small N. But till it materializes, let's stick to generic
-	 * prime method...
-	 *						<appro>
-	 */
-	meth = EC_GFp_mont_method();
-#else
-	meth = EC_GFp_nist_method();
-#endif
-
-	ret = EC_GROUP_new(meth);
-	if (ret == NULL)
-		return NULL;
-
-	if (!EC_GROUP_set_curve(ret, p, a, b, ctx)) {
-		unsigned long err;
-
-		err = ERR_peek_last_error();
+	EC_GROUP *group;
 
-		if (!(ERR_GET_LIB(err) == ERR_LIB_EC &&
-		    ((ERR_GET_REASON(err) == EC_R_NOT_A_NIST_PRIME) ||
-		    (ERR_GET_REASON(err) == EC_R_NOT_A_SUPPORTED_NIST_PRIME)))) {
-			/* real error */
+	if ((group = EC_GROUP_new(method)) == NULL)
+		goto err;
 
-			EC_GROUP_clear_free(ret);
-			return NULL;
-		}
-		/* not an actual error, we just cannot use EC_GFp_nist_method */
+	if (!EC_GROUP_set_curve(group, p, a, b, ctx))
+		goto err;
 
-		ERR_clear_error();
+	return group;
 
-		EC_GROUP_clear_free(ret);
-		meth = EC_GFp_mont_method();
+ err:
+	EC_GROUP_clear_free(group);
 
-		ret = EC_GROUP_new(meth);
-		if (ret == NULL)
-			return NULL;
+	return NULL;
+}
 
-		if (!EC_GROUP_set_curve(ret, p, a, b, ctx)) {
-			EC_GROUP_clear_free(ret);
-			return NULL;
-		}
-	}
-	return ret;
+EC_GROUP *
+EC_GROUP_new_curve_GFp(const BIGNUM *p, const BIGNUM *a, const BIGNUM *b,
+    BN_CTX *ctx)
+{
+	return ec_group_new_curve(EC_GFp_mont_method(), p, a, b, ctx);
 }
 
 #ifndef OPENSSL_NO_EC2M
@@ -149,19 +106,6 @@ EC_GROUP *
 EC_GROUP_new_curve_GF2m(const BIGNUM *p, const BIGNUM *a, const BIGNUM *b,
     BN_CTX *ctx)
 {
-	const EC_METHOD *meth;
-	EC_GROUP *ret;
-
-	meth = EC_GF2m_simple_method();
-
-	ret = EC_GROUP_new(meth);
-	if (ret == NULL)
-		return NULL;
-
-	if (!EC_GROUP_set_curve(ret, p, a, b, ctx)) {
-		EC_GROUP_clear_free(ret);
-		return NULL;
-	}
-	return ret;
+	return ec_group_new_curve(EC_GF2m_simple_method(), p, a, b, ctx);
 }
 #endif