From: tb <tb@openbsd.org>
Date: Thu, 7 Apr 2022 17:38:24 +0000 (+0000)
Subject: Avoid infinite loop on parsing DSA private keys
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=5c3491c88525c7dd851c2219135004522a168fe4;p=openbsd

Avoid infinite loop on parsing DSA private keys

DSA private keys with ill-chosen g could cause an infinite
loop on deserializing. Add a few sanity checks that ensure
that g is according to the FIPS 186-4: check 1 < g < p and
g^q == 1 (mod p). This is enough to ascertain that g is a
generator of a multiplicative group of order q once we know
that q is prime (which is checked a bit later).

Issue reported with reproducers by Hanno Boeck.
Additional variants and analysis by David Benjamin.

ok beck jsing
---

diff --git a/lib/libcrypto/dsa/dsa_ameth.c b/lib/libcrypto/dsa/dsa_ameth.c
index 07773ad44f6..9b8f09d969f 100644
--- a/lib/libcrypto/dsa/dsa_ameth.c
+++ b/lib/libcrypto/dsa/dsa_ameth.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: dsa_ameth.c,v 1.34 2022/02/24 21:07:03 tb Exp $ */
+/* $OpenBSD: dsa_ameth.c,v 1.35 2022/04/07 17:38:24 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2006.
  */
@@ -479,7 +479,7 @@ old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen)
 {
 	DSA *dsa;
 	BN_CTX *ctx = NULL;
-	BIGNUM *j, *p1, *newp1;
+	BIGNUM *j, *p1, *newp1, *powg;
 	int qbits;
 
 	if (!(dsa = d2i_DSAPrivateKey(NULL, pder, derlen))) {
@@ -498,6 +498,13 @@ old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen)
 		goto err;
 	}
 
+	/* Check that 1 < g < p. */
+	if (BN_cmp(dsa->g, BN_value_one()) <= 0 ||
+	    BN_cmp(dsa->g, dsa->p) >= 0) {
+		DSAerror(DSA_R_PARAMETER_ENCODING_ERROR); /* XXX */
+		goto err;
+	}
+
 	ctx = BN_CTX_new();
 	if (ctx == NULL)
 		goto err;
@@ -509,7 +516,8 @@ old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen)
 	j = BN_CTX_get(ctx);
 	p1 = BN_CTX_get(ctx);
 	newp1 = BN_CTX_get(ctx);
-	if (j == NULL || p1 == NULL || newp1 == NULL)
+	powg = BN_CTX_get(ctx);
+	if (j == NULL || p1 == NULL || newp1 == NULL || powg == NULL)
 		goto err;
 	/* p1 = p - 1 */
 	if (BN_sub(p1, dsa->p, BN_value_one()) == 0)
@@ -525,6 +533,19 @@ old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen)
 		goto err;
 	}
 
+	/*
+	 * Check that g generates a multiplicative subgroup of order q.
+	 * We only check that g^q == 1, so the order is a divisor of q.
+	 * Once we know that q is prime, this is enough.
+	 */
+
+	if (!BN_mod_exp_ct(powg, dsa->g, dsa->q, dsa->p, ctx))
+		goto err;
+	if (BN_cmp(powg, BN_value_one()) != 0) {
+		DSAerror(DSA_R_PARAMETER_ENCODING_ERROR); /* XXX */
+		goto err;
+	}
+
 	/*
 	 * Check that q is not a composite number.
 	 */