From: djm@openbsd.org <djm@openbsd.org>
Date: Fri, 7 Nov 2025 04:11:59 +0000 (+0000)
Subject: upstream: Remove some unnecessary checks in
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9bea081888fa659b964e6bfa41caca2b5def98c2;p=thirdparty%2Fopenssh-portable.git

upstream: Remove some unnecessary checks in

sshkey_ec_validate_public()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Checking nQ == infinity is not needed for cofactor 1 curves.
Checking x and y coordinates against order is not needed either.

patch from Szilárd Pfeiffer, with further refinement by tb@
ok tb@

OpenBSD-Commit-ID: ef985e2be7c64e215d064757d3fc65eb181e8ede
---

diff --git a/sshkey.c b/sshkey.c
index afd7822c4..6e47362fc 100644
--- a/sshkey.c
+++ b/sshkey.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: sshkey.c,v 1.155 2025/10/03 00:08:02 djm Exp $ */
+/* $OpenBSD: sshkey.c,v 1.156 2025/11/07 04:11:59 djm Exp $ */
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
  * Copyright (c) 2008 Alexander von Gernler.  All rights reserved.
@@ -2672,64 +2672,54 @@ int
 sshkey_ec_validate_public(const EC_GROUP *group, const EC_POINT *public)
 {
 	EC_POINT *nq = NULL;
-	BIGNUM *order = NULL, *x = NULL, *y = NULL, *tmp = NULL;
+	BIGNUM *order = NULL, *cofactor = NULL;
 	int ret = SSH_ERR_KEY_INVALID_EC_VALUE;
 
 	/*
 	 * NB. This assumes OpenSSL has already verified that the public
-	 * point lies on the curve. This is done by EC_POINT_oct2point()
-	 * implicitly calling EC_POINT_is_on_curve(). If this code is ever
-	 * reachable with public points not unmarshalled using
-	 * EC_POINT_oct2point then the caller will need to explicitly check.
+	 * point lies on the curve and that its coordinates are in [0, p).
+	 * This is done by EC_POINT_oct2point() on at least OpenSSL >= 1.1,
+	 * LibreSSL and BoringSSL.
 	 */
 
 	/* Q != infinity */
 	if (EC_POINT_is_at_infinity(group, public))
 		goto out;
 
-	if ((x = BN_new()) == NULL ||
-	    (y = BN_new()) == NULL ||
-	    (order = BN_new()) == NULL ||
-	    (tmp = BN_new()) == NULL) {
+	if ((cofactor = BN_new()) == NULL) {
 		ret = SSH_ERR_ALLOC_FAIL;
 		goto out;
 	}
-
-	/* log2(x) > log2(order)/2, log2(y) > log2(order)/2 */
-	if (EC_GROUP_get_order(group, order, NULL) != 1 ||
-	    EC_POINT_get_affine_coordinates(group, public, x, y, NULL) != 1) {
-		ret = SSH_ERR_LIBCRYPTO_ERROR;
-		goto out;
-	}
-	if (BN_num_bits(x) <= BN_num_bits(order) / 2 ||
-	    BN_num_bits(y) <= BN_num_bits(order) / 2)
+	if (EC_GROUP_get_cofactor(group, cofactor, NULL) != 1)
 		goto out;
 
-	/* nQ == infinity (n == order of subgroup) */
-	if ((nq = EC_POINT_new(group)) == NULL) {
-		ret = SSH_ERR_ALLOC_FAIL;
-		goto out;
-	}
-	if (EC_POINT_mul(group, nq, NULL, public, order, NULL) != 1) {
-		ret = SSH_ERR_LIBCRYPTO_ERROR;
-		goto out;
+	/*
+	 * Verify nQ == infinity (n == order of subgroup)
+	 * This check may be skipped for curves with cofactor 1, as per
+	 * NIST SP 800-56A, 5.6.2.3. 
+	 */
+	if (!BN_is_one(cofactor)) {
+		if ((order = BN_new()) == NULL) {
+			ret = SSH_ERR_ALLOC_FAIL;
+			goto out;
+		}
+		if ((nq = EC_POINT_new(group)) == NULL) {
+			ret = SSH_ERR_ALLOC_FAIL;
+			goto out;
+		}
+		if (EC_POINT_mul(group, nq, NULL, public, order, NULL) != 1) {
+			ret = SSH_ERR_LIBCRYPTO_ERROR;
+			goto out;
+		}
+		if (EC_POINT_is_at_infinity(group, nq) != 1)
+			goto out;
 	}
-	if (EC_POINT_is_at_infinity(group, nq) != 1)
-		goto out;
 
-	/* x < order - 1, y < order - 1 */
-	if (!BN_sub(tmp, order, BN_value_one())) {
-		ret = SSH_ERR_LIBCRYPTO_ERROR;
-		goto out;
-	}
-	if (BN_cmp(x, tmp) >= 0 || BN_cmp(y, tmp) >= 0)
-		goto out;
+	/* success */
 	ret = 0;
  out:
-	BN_clear_free(x);
-	BN_clear_free(y);
+	BN_clear_free(cofactor);
 	BN_clear_free(order);
-	BN_clear_free(tmp);
 	EC_POINT_free(nq);
 	return ret;
 }