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)

crypto/dsa/dsa_err.c

@@ -1,6 +1,6 @@
 /*
  * Generated by util/mkerr.pl DO NOT EDIT
- * Copyright 1995-2021 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2023 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the Apache License 2.0 (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -36,6 +36,7 @@ static const ERR_STRING_DATA DSA_str_reasons[] = {
     {ERR_PACK(ERR_LIB_DSA, 0, DSA_R_Q_NOT_PRIME), "q not prime"},
     {ERR_PACK(ERR_LIB_DSA, 0, DSA_R_SEED_LEN_SMALL),
     "seed_len is less than the length of q"},
+    {ERR_PACK(ERR_LIB_DSA, 0, DSA_R_TOO_MANY_RETRIES), "too many retries"},
     {0, NULL}
 };
 

crypto/dsa/dsa_ossl.c

@@ -22,6 +22,9 @@
 #include <openssl/asn1.h>
 #include "internal/deterministic_nonce.h"
 
+#define MIN_DSA_SIGN_QBITS   128
+#define MAX_DSA_SIGN_RETRIES 8
+
 static DSA_SIG *dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa);
 static int dsa_sign_setup_no_digest(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp,
                                     BIGNUM **rp);
@@ -80,6 +83,7 @@ DSA_SIG *ossl_dsa_do_sign_int(const unsigned char *dgst, int dlen, DSA *dsa,
     int reason = ERR_R_BN_LIB;
     DSA_SIG *ret = NULL;
     int rv = 0;
+    int retries = 0;
 
     if (dsa->params.p == NULL
         || dsa->params.q == NULL
@@ -135,7 +139,10 @@ DSA_SIG *ossl_dsa_do_sign_int(const unsigned char *dgst, int dlen, DSA *dsa,
      *   s := blind^-1 * k^-1 * (blind * m + blind * r * priv_key) mod q
      */
 
-    /* Generate a blinding value */
+    /*
+     * Generate a blinding value
+     * The size of q is tested in dsa_sign_setup() so there should not be an infinite loop here.
+     */
     do {
         if (!BN_priv_rand_ex(blind, BN_num_bits(dsa->params.q) - 1,
                              BN_RAND_TOP_ANY, BN_RAND_BOTTOM_ANY, 0, ctx))
@@ -170,14 +177,19 @@ DSA_SIG *ossl_dsa_do_sign_int(const unsigned char *dgst, int dlen, DSA *dsa,
         goto err;
 
     /*
-     * Redo if r or s is zero as required by FIPS 186-3: this is very
-     * unlikely.
+     * Redo if r or s is zero as required by FIPS 186-4: Section 4.6
+     * This is very unlikely.
+     * Limit the retries so there is no possibility of an infinite
+     * loop for bad domain parameter values.
      */
-    if (BN_is_zero(ret->r) || BN_is_zero(ret->s))
+    if (BN_is_zero(ret->r) || BN_is_zero(ret->s)) {
+        if (retries++ > MAX_DSA_SIGN_RETRIES) {
+            reason = DSA_R_TOO_MANY_RETRIES;
+            goto err;
+        }
         goto redo;
-
+    }
     rv = 1;
-
  err:
     if (rv == 0) {
         ERR_raise(ERR_LIB_DSA, reason);
@@ -230,7 +242,6 @@ static int dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in,
         ERR_raise(ERR_LIB_DSA, DSA_R_MISSING_PRIVATE_KEY);
         return 0;
     }
-
     k = BN_new();
     l = BN_new();
     if (k == NULL || l == NULL)
@@ -246,7 +257,8 @@ static int dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in,
     /* Preallocate space */
     q_bits = BN_num_bits(dsa->params.q);
     q_words = bn_get_top(dsa->params.q);
-    if (!bn_wexpand(k, q_words + 2)
+    if (q_bits < MIN_DSA_SIGN_QBITS
+        || !bn_wexpand(k, q_words + 2)
         || !bn_wexpand(l, q_words + 2))
         goto err;
 

crypto/err/openssl.txt

@@ -1,4 +1,4 @@
-# Copyright 1999-2022 The OpenSSL Project Authors. All Rights Reserved.
+# Copyright 1999-2023 The OpenSSL Project Authors. All Rights Reserved.
 #
 # Licensed under the Apache License 2.0 (the "License").  You may not use
 # this file except in compliance with the License.  You can obtain a copy
@@ -545,6 +545,7 @@ DSA_R_PARAMETER_ENCODING_ERROR:105:parameter encoding error
 DSA_R_P_NOT_PRIME:115:p not prime
 DSA_R_Q_NOT_PRIME:113:q not prime
 DSA_R_SEED_LEN_SMALL:110:seed_len is less than the length of q
+DSA_R_TOO_MANY_RETRIES:116:too many retries
 DSO_R_CTRL_FAILED:100:control command failed
 DSO_R_DSO_ALREADY_LOADED:110:dso already loaded
 DSO_R_EMPTY_FILE_STRUCTURE:113:empty file structure

include/crypto/dsaerr.h

@@ -1,6 +1,6 @@
 /*
  * Generated by util/mkerr.pl DO NOT EDIT
- * Copyright 2020-2021 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 2020-2023 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the Apache License 2.0 (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy

include/openssl/dsaerr.h

@@ -1,6 +1,6 @@
 /*
  * Generated by util/mkerr.pl DO NOT EDIT
- * Copyright 1995-2021 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2023 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the Apache License 2.0 (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -38,6 +38,7 @@
 #  define DSA_R_P_NOT_PRIME                                115
 #  define DSA_R_Q_NOT_PRIME                                113
 #  define DSA_R_SEED_LEN_SMALL                             110
+#  define DSA_R_TOO_MANY_RETRIES                           116
 
 # endif
 #endif

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;