Fix infinite loops in DSA sign code.

Fixes #20268

Values such as q=1 or priv=0 caused infinite loops when calling
DSA_sign() without these changes.

There are other cases where bad domain parameters may have caused
infinite loops where the retry counter has been added. The simpler case
of priv=0 also hits this case. q=1 caused an infinite loop in the setup.

The max retry value has been set to an arbitrary value of 8 (it is
unlikely to ever do a single retry for valid values).

The minimum q bits was set to an arbitrary value of 128 (160 is still
used for legacy reasons when using 512 bit keys).

Thanks @guidovranken for detecting this, and @davidben for his
insightful analysis.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20384)

1 files changed, 110 insertions, 26 deletions

diff --git a/test/dsatest.c b/test/dsatest.c
index 2d34ca4261..5c13a0d825 100644
--- a/test/dsatest.c
+++ b/test/dsatest.c
@@ -32,6 +32,32 @@
 #ifndef OPENSSL_NO_DSA
 static int dsa_cb(int p, int n, BN_GENCB *arg);
 
+static unsigned char out_p[] = {
+    0x8d, 0xf2, 0xa4, 0x94, 0x49, 0x22, 0x76, 0xaa,
+    0x3d, 0x25, 0x75, 0x9b, 0xb0, 0x68, 0x69, 0xcb,
+    0xea, 0xc0, 0xd8, 0x3a, 0xfb, 0x8d, 0x0c, 0xf7,
+    0xcb, 0xb8, 0x32, 0x4f, 0x0d, 0x78, 0x82, 0xe5,
+    0xd0, 0x76, 0x2f, 0xc5, 0xb7, 0x21, 0x0e, 0xaf,
+    0xc2, 0xe9, 0xad, 0xac, 0x32, 0xab, 0x7a, 0xac,
+    0x49, 0x69, 0x3d, 0xfb, 0xf8, 0x37, 0x24, 0xc2,
+    0xec, 0x07, 0x36, 0xee, 0x31, 0xc8, 0x02, 0x91,
+};
+static unsigned char out_q[] = {
+    0xc7, 0x73, 0x21, 0x8c, 0x73, 0x7e, 0xc8, 0xee,
+    0x99, 0x3b, 0x4f, 0x2d, 0xed, 0x30, 0xf4, 0x8e,
+    0xda, 0xce, 0x91, 0x5f,
+};
+static unsigned char out_g[] = {
+    0x62, 0x6d, 0x02, 0x78, 0x39, 0xea, 0x0a, 0x13,
+    0x41, 0x31, 0x63, 0xa5, 0x5b, 0x4c, 0xb5, 0x00,
+    0x29, 0x9d, 0x55, 0x22, 0x95, 0x6c, 0xef, 0xcb,
+    0x3b, 0xff, 0x10, 0xf3, 0x99, 0xce, 0x2c, 0x2e,
+    0x71, 0xcb, 0x9d, 0xe5, 0xfa, 0x24, 0xba, 0xbf,
+    0x58, 0xe5, 0xb7, 0x95, 0x21, 0x92, 0x5c, 0x9c,
+    0xc4, 0x2e, 0x9f, 0x6f, 0x46, 0x4b, 0x08, 0x8c,
+    0xc5, 0x72, 0xaf, 0x53, 0xe6, 0xd7, 0x88, 0x02,
+};
+
 static int dsa_test(void)
 {
     BN_GENCB *cb;
@@ -51,31 +77,6 @@ static int dsa_test(void)
         0xb6, 0x21, 0x1b, 0x40, 0x62, 0xba, 0x32, 0x24,
         0xe0, 0x42, 0x7d, 0xd3,
     };
-    static unsigned char out_p[] = {
-        0x8d, 0xf2, 0xa4, 0x94, 0x49, 0x22, 0x76, 0xaa,
-        0x3d, 0x25, 0x75, 0x9b, 0xb0, 0x68, 0x69, 0xcb,
-        0xea, 0xc0, 0xd8, 0x3a, 0xfb, 0x8d, 0x0c, 0xf7,
-        0xcb, 0xb8, 0x32, 0x4f, 0x0d, 0x78, 0x82, 0xe5,
-        0xd0, 0x76, 0x2f, 0xc5, 0xb7, 0x21, 0x0e, 0xaf,
-        0xc2, 0xe9, 0xad, 0xac, 0x32, 0xab, 0x7a, 0xac,
-        0x49, 0x69, 0x3d, 0xfb, 0xf8, 0x37, 0x24, 0xc2,
-        0xec, 0x07, 0x36, 0xee, 0x31, 0xc8, 0x02, 0x91,
-    };
-    static unsigned char out_q[] = {
-        0xc7, 0x73, 0x21, 0x8c, 0x73, 0x7e, 0xc8, 0xee,
-        0x99, 0x3b, 0x4f, 0x2d, 0xed, 0x30, 0xf4, 0x8e,
-        0xda, 0xce, 0x91, 0x5f,
-    };
-    static unsigned char out_g[] = {
-        0x62, 0x6d, 0x02, 0x78, 0x39, 0xea, 0x0a, 0x13,
-        0x41, 0x31, 0x63, 0xa5, 0x5b, 0x4c, 0xb5, 0x00,
-        0x29, 0x9d, 0x55, 0x22, 0x95, 0x6c, 0xef, 0xcb,
-        0x3b, 0xff, 0x10, 0xf3, 0x99, 0xce, 0x2c, 0x2e,
-        0x71, 0xcb, 0x9d, 0xe5, 0xfa, 0x24, 0xba, 0xbf,
-        0x58, 0xe5, 0xb7, 0x95, 0x21, 0x92, 0x5c, 0x9c,
-        0xc4, 0x2e, 0x9f, 0x6f, 0x46, 0x4b, 0x08, 0x8c,
-        0xc5, 0x72, 0xaf, 0x53, 0xe6, 0xd7, 0x88, 0x02,
-    };
     static const unsigned char str1[] = "12345678901234567890";
 
     if (!TEST_ptr(cb = BN_GENCB_new()))
@@ -114,7 +115,6 @@ static int dsa_test(void)
         goto end;
     if (TEST_int_gt(DSA_verify(0, str1, 20, sig, siglen, dsa), 0))
         ret = 1;
-
  end:
     DSA_free(dsa);
     BN_GENCB_free(cb);
@@ -325,6 +325,89 @@ static int test_dsa_default_paramgen_validate(int i)
     return ret;
 }
 
+static int test_dsa_sig_infinite_loop(void)
+{
+    int ret = 0;
+    DSA *dsa = NULL;
+    BIGNUM *p = NULL, *q = NULL, *g = NULL, *priv = NULL, *pub = NULL, *priv2 = NULL;
+    BIGNUM *badq = NULL, *badpriv = NULL;
+    const unsigned char msg[] = { 0x00 };
+    unsigned int signature_len;
+    unsigned char signature[64];
+
+    static unsigned char out_priv[] = {
+        0x17, 0x00, 0xb2, 0x8d, 0xcb, 0x24, 0xc9, 0x98,
+        0xd0, 0x7f, 0x1f, 0x83, 0x1a, 0xa1, 0xc4, 0xa4,
+        0xf8, 0x0f, 0x7f, 0x12
+    };
+    static unsigned char out_pub[] = {
+        0x04, 0x72, 0xee, 0x8d, 0xaa, 0x4d, 0x89, 0x60,
+        0x0e, 0xb2, 0xd4, 0x38, 0x84, 0xa2, 0x2a, 0x60,
+        0x5f, 0x67, 0xd7, 0x9e, 0x24, 0xdd, 0xe8, 0x50,
+        0xf2, 0x23, 0x71, 0x55, 0x53, 0x94, 0x0d, 0x6b,
+        0x2e, 0xcd, 0x30, 0xda, 0x6f, 0x1e, 0x2c, 0xcf,
+        0x59, 0xbe, 0x05, 0x6c, 0x07, 0x0e, 0xc6, 0x38,
+        0x05, 0xcb, 0x0c, 0x44, 0x0a, 0x08, 0x13, 0xb6,
+        0x0f, 0x14, 0xde, 0x4a, 0xf6, 0xed, 0x4e, 0xc3
+    };
+    if (!TEST_ptr(p = BN_bin2bn(out_p, sizeof(out_p), NULL))
+        || !TEST_ptr(q = BN_bin2bn(out_q, sizeof(out_q), NULL))
+        || !TEST_ptr(g = BN_bin2bn(out_g, sizeof(out_g), NULL))
+        || !TEST_ptr(pub = BN_bin2bn(out_pub, sizeof(out_pub), NULL))
+        || !TEST_ptr(priv = BN_bin2bn(out_priv, sizeof(out_priv), NULL))
+        || !TEST_ptr(priv2 = BN_dup(priv))
+        || !TEST_ptr(badq = BN_new())
+        || !TEST_true(BN_set_word(badq, 1))
+        || !TEST_ptr(badpriv = BN_new())
+        || !TEST_true(BN_set_word(badpriv, 0))
+        || !TEST_ptr(dsa = DSA_new()))
+        goto err;
+
+    if (!TEST_true(DSA_set0_pqg(dsa, p, q, g)))
+        goto err;
+    p = q = g = NULL;
+
+    if (!TEST_true(DSA_set0_key(dsa, pub, priv)))
+        goto err;
+    pub = priv = NULL;
+
+    if (!TEST_int_le(DSA_size(dsa), sizeof(signature)))
+        goto err;
+
+    if (!TEST_true(DSA_sign(0, msg, sizeof(msg), signature, &signature_len, dsa)))
+        goto err;
+
+    /* Test using a private key of zero fails - this causes an infinite loop without the retry test */
+    if (!TEST_true(DSA_set0_key(dsa, NULL, badpriv)))
+        goto err;
+    badpriv = NULL;
+    if (!TEST_false(DSA_sign(0, msg, sizeof(msg), signature, &signature_len, dsa)))
+        goto err;
+
+    /* Restore private and set a bad q - this caused an infinite loop in the setup */
+    if (!TEST_true(DSA_set0_key(dsa, NULL, priv2)))
+        goto err;
+    priv2 = NULL;
+    if (!TEST_true(DSA_set0_pqg(dsa, NULL, badq, NULL)))
+        goto err;
+    badq = NULL;
+    if (!TEST_false(DSA_sign(0, msg, sizeof(msg), signature, &signature_len, dsa)))
+        goto err;
+
+    ret = 1;
+err:
+    BN_free(badq);
+    BN_free(badpriv);
+    BN_free(pub);
+    BN_free(priv);
+    BN_free(priv2);
+    BN_free(g);
+    BN_free(q);
+    BN_free(p);
+    DSA_free(dsa);
+    return ret;
+}
+
 #endif /* OPENSSL_NO_DSA */
 
 int setup_tests(void)
@@ -332,6 +415,7 @@ int setup_tests(void)
 #ifndef OPENSSL_NO_DSA
     ADD_TEST(dsa_test);
     ADD_TEST(dsa_keygen_test);
+    ADD_TEST(test_dsa_sig_infinite_loop);
     ADD_ALL_TESTS(test_dsa_default_paramgen_validate, 2);
 #endif
     return 1;