From: Tobias Brunner <tobias@strongswan.org>
Date: Mon, 19 Jul 2021 15:27:53 +0000 (+0200)
Subject: gmp: Move shared secret calculation to get_shared_secret()
X-Git-Tag: 5.9.7dr2~1^2~7
X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=0e82d5cc2cb969744386cca1e02caf7da7f8de63;p=thirdparty%2Fstrongswan.git

gmp: Move shared secret calculation to get_shared_secret()

This avoids doing costly operations when just setting the public key.
For the same reason the optional extended public key check is moved.
---

diff --git a/src/libstrongswan/plugins/gmp/gmp_diffie_hellman.c b/src/libstrongswan/plugins/gmp/gmp_diffie_hellman.c
index 619c6de662..6528495a5e 100644
--- a/src/libstrongswan/plugins/gmp/gmp_diffie_hellman.c
+++ b/src/libstrongswan/plugins/gmp/gmp_diffie_hellman.c
@@ -97,56 +97,30 @@ METHOD(key_exchange_t, set_public_key, bool,
 	mpz_init(p_min_1);
 	mpz_sub_ui(p_min_1, this->p, 1);
 
+	/* this->computed is not reset in order to prevent reuse of this DH
+	 * instance (see below) */
 	mpz_import(this->yb, value.len, 1, 1, 1, 0, value.ptr);
 
-	/* check public value:
-	 * 1. 0 or 1 is invalid as 0^a = 0 and 1^a = 1
-	 * 2. a public value larger or equal the modulus is invalid */
-	if (mpz_cmp_ui(this->yb, 1) > 0 &&
-		mpz_cmp(this->yb, p_min_1) < 0)
-	{
-#ifdef EXTENDED_DH_TEST
-		/* 3. test if y ^ q mod p = 1, where q = (p - 1)/2. */
-		mpz_t q, one;
-		diffie_hellman_params_t *params;
-
-		mpz_init(q);
-		mpz_init(one);
-
-		params = diffie_hellman_get_params(this->group);
-		if (!params->subgroup.len)
-		{
-			mpz_fdiv_q_2exp(q, p_min_1, 1);
-		}
-		else
-		{
-			mpz_import(q, params->subgroup.len, 1, 1, 1, 0, params->subgroup.ptr);
-		}
-		mpz_powm(one, this->yb, q, this->p);
-		mpz_clear(q);
-		if (mpz_cmp_ui(one, 1) == 0)
-		{
-			mpz_powm(this->zz, this->yb, this->xa, this->p);
-			this->computed = TRUE;
-		}
-		else
-		{
-			DBG1(DBG_LIB, "public DH value verification failed:"
-				 " y ^ q mod p != 1");
-		}
-		mpz_clear(one);
-#else
-		mpz_powm(this->zz, this->yb, this->xa, this->p);
-		this->computed = TRUE;
-#endif
-	}
-	else
+	/* check that the public value y satisfies 1 < y < p-1.
+	 * according to RFC 6989, section 2.1, this is enough for the common safe-
+	 * prime DH groups (i.e. with q=(p-1)/2 being prime), only for those with
+	 * small subgroups (22, 23, 24) does the RFC require the extended test but
+	 * only if private keys are reused. we never do that anyway and it's
+	 * explicitly prevented in this implementation. so the extended test, which
+	 * optionally happens in get_shared_secret(), is really only useful for full
+	 * NIST SP 800-56A compliance, which only allows the partial check for
+	 * safe-prime groups.
+	 */
+	if (mpz_cmp_ui(this->yb, 1) <= 0 ||
+		mpz_cmp(this->yb, p_min_1) >= 0)
 	{
-		DBG1(DBG_LIB, "public DH value verification failed:"
-			 " y < 2 || y > p - 1 ");
+		DBG1(DBG_LIB, "public DH value verification failed: "
+			 "y <= 1 || y >= p - 1");
+		mpz_clear(p_min_1);
+		return FALSE;
 	}
 	mpz_clear(p_min_1);
-	return this->computed;
+	return TRUE;
 }
 
 METHOD(key_exchange_t, get_public_key, bool,
@@ -175,7 +149,41 @@ METHOD(key_exchange_t, get_shared_secret, bool,
 {
 	if (!this->computed)
 	{
-		return FALSE;
+#ifdef EXTENDED_DH_TEST
+		/* test if y ^ q mod p = 1, where q = (p - 1)/2 or the actual size of
+		 * the subgroup.  as noted above, this check is not really necessary as
+		 * the plugin does not reuse private keys */
+		mpz_t q, one, p_min_1;
+		diffie_hellman_params_t *params;
+
+		mpz_init(q);
+		mpz_init(one);
+
+		params = diffie_hellman_get_params(this->group);
+		if (!params->subgroup.len)
+		{
+			mpz_init(p_min_1);
+			mpz_sub_ui(p_min_1, this->p, 1);
+			mpz_fdiv_q_2exp(q, p_min_1, 1);
+			mpz_clear(p_min_1);
+		}
+		else
+		{
+			mpz_import(q, params->subgroup.len, 1, 1, 1, 0, params->subgroup.ptr);
+		}
+		mpz_powm(one, this->yb, q, this->p);
+		mpz_clear(q);
+		if (mpz_cmp_ui(one, 1) != 0)
+		{
+			DBG1(DBG_LIB, "public DH value verification failed: "
+				 "y ^ q mod p != 1");
+			mpz_clear(one);
+			return FALSE;
+		}
+		mpz_clear(one);
+#endif
+		mpz_powm(this->zz, this->yb, this->xa, this->p);
+		this->computed = TRUE;
 	}
 	secret->len = this->p_len;
 	secret->ptr = mpz_export(NULL, NULL, 1, secret->len, 1, 0, this->zz);