Clean up and simplify BIGNUM handling in DSA code.

This adds missing BN_CTX_start()/BN_CTX_end() calls, removes NULL checks
before BN_CTX_end()/BN_CTX_free() (since they're NULL safe) and calls
BN_free() instead of BN_clear_free() (which does the same thing).

Also replace stack allocated BIGNUMs with calls to BN_CTX_get(), using the
BN_CTX that is already available.

ok tb@

3 files changed, 116 insertions, 96 deletions

diff --git a/src/lib/libcrypto/dsa/dsa_ameth.c b/src/lib/libcrypto/dsa/dsa_ameth.c
index fb333dda0f..0d3333d92c 100644
--- a/src/lib/libcrypto/dsa/dsa_ameth.c
+++ b/src/lib/libcrypto/dsa/dsa_ameth.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: dsa_ameth.c,v 1.38 2022/11/26 16:08:52 tb Exp $ */
+/* $OpenBSD: dsa_ameth.c,v 1.39 2023/01/11 04:39:42 jsing Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2006.
  */
@@ -192,7 +192,6 @@ dsa_priv_decode(EVP_PKEY *pkey, const PKCS8_PRIV_KEY_INFO *p8)
         ASN1_INTEGER *privkey = NULL;
         BN_CTX *ctx = NULL;
         DSA *dsa = NULL;
-
         int ret = 0;
 
         if (!PKCS8_pkey_get0(NULL, &p, &pklen, &palg, p8))
@@ -221,11 +220,14 @@ dsa_priv_decode(EVP_PKEY *pkey, const PKCS8_PRIV_KEY_INFO *p8)
                 DSAerror(ERR_R_MALLOC_FAILURE);
                 goto dsaerr;
         }
-        if (!(ctx = BN_CTX_new())) {
+
+        if ((ctx = BN_CTX_new()) == NULL) {
                 DSAerror(ERR_R_MALLOC_FAILURE);
                 goto dsaerr;
         }
 
+        BN_CTX_start(ctx);
+
         if (!BN_mod_exp_ct(dsa->pub_key, dsa->g, dsa->priv_key, dsa->p, ctx)) {
                 DSAerror(DSA_R_BN_ERROR);
                 goto dsaerr;
@@ -242,8 +244,10 @@ decerr:
 dsaerr:
         DSA_free(dsa);
 done:
+        BN_CTX_end(ctx);
         BN_CTX_free(ctx);
         ASN1_INTEGER_free(privkey);
+
         return ret;
 }
 
@@ -511,26 +515,31 @@ old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen)
                 goto err;
         }
 
-        ctx = BN_CTX_new();
-        if (ctx == NULL)
+        if ((ctx = BN_CTX_new()) == NULL)
                 goto err;
 
+        BN_CTX_start(ctx);
+
         /*
          * Check that p and q are consistent with each other.
          */
-
-        j = BN_CTX_get(ctx);
-        p1 = BN_CTX_get(ctx);
-        newp1 = BN_CTX_get(ctx);
-        powg = BN_CTX_get(ctx);
-        if (j == NULL || p1 == NULL || newp1 == NULL || powg == NULL)
+        if ((j = BN_CTX_get(ctx)) == NULL)
                 goto err;
+        if ((p1 = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((newp1 = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((powg = BN_CTX_get(ctx)) == NULL)
+                goto err;
+
         /* p1 = p - 1 */
         if (BN_sub(p1, dsa->p, BN_value_one()) == 0)
                 goto err;
+
         /* j = (p - 1) / q */
         if (BN_div_ct(j, NULL, p1, dsa->q, ctx) == 0)
                 goto err;
+
         /* q * j should == p - 1 */
         if (BN_mul(newp1, dsa->q, j, ctx) == 0)
                 goto err;
@@ -561,12 +570,14 @@ old_dsa_priv_decode(EVP_PKEY *pkey, const unsigned char **pder, int derlen)
                 goto err;
         }
 
+        BN_CTX_end(ctx);
         BN_CTX_free(ctx);
 
         EVP_PKEY_assign_DSA(pkey, dsa);
         return 1;
 
  err:
+        BN_CTX_end(ctx);
         BN_CTX_free(ctx);
         DSA_free(dsa);
         return 0;
diff --git a/src/lib/libcrypto/dsa/dsa_gen.c b/src/lib/libcrypto/dsa/dsa_gen.c
index 9c2b9cfd70..1f91894100 100644
--- a/src/lib/libcrypto/dsa/dsa_gen.c
+++ b/src/lib/libcrypto/dsa/dsa_gen.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: dsa_gen.c,v 1.26 2022/11/26 16:08:52 tb Exp $ */
+/* $OpenBSD: dsa_gen.c,v 1.27 2023/01/11 04:39:42 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -142,11 +142,12 @@ dsa_builtin_paramgen(DSA *ret, size_t bits, size_t qbits, const EVP_MD *evpmd,
         else if (seed_len != 0)
                 goto err;
 
-        if ((mont=BN_MONT_CTX_new()) == NULL)
+        if ((mont = BN_MONT_CTX_new()) == NULL)
                 goto err;
 
-        if ((ctx=BN_CTX_new()) == NULL)
+        if ((ctx = BN_CTX_new()) == NULL)
                 goto err;
+
         BN_CTX_start(ctx);
 
         if ((r0 = BN_CTX_get(ctx)) == NULL)
@@ -348,11 +349,10 @@ err:
                 if (seed_out != NULL)
                         memcpy(seed_out, seed, qsize);
         }
-        if (ctx) {
-                BN_CTX_end(ctx);
-                BN_CTX_free(ctx);
-        }
+        BN_CTX_end(ctx);
+        BN_CTX_free(ctx);
         BN_MONT_CTX_free(mont);
+
         return ok;
 }
 #endif
diff --git a/src/lib/libcrypto/dsa/dsa_ossl.c b/src/lib/libcrypto/dsa/dsa_ossl.c
index 102bc44cd7..a242291a62 100644
--- a/src/lib/libcrypto/dsa/dsa_ossl.c
+++ b/src/lib/libcrypto/dsa/dsa_ossl.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: dsa_ossl.c,v 1.46 2022/11/26 16:08:52 tb Exp $ */
+/* $OpenBSD: dsa_ossl.c,v 1.47 2023/01/11 04:39:42 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -95,28 +95,35 @@ DSA_OpenSSL(void)
 static DSA_SIG *
 dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa)
 {
-        BIGNUM b, bm, bxr, binv, m, *kinv = NULL, *r = NULL, *s = NULL;
+        BIGNUM *b = NULL, *bm = NULL, *bxr = NULL, *binv = NULL, *m = NULL;
+        BIGNUM *kinv = NULL, *r = NULL, *s = NULL;
         BN_CTX *ctx = NULL;
         int reason = ERR_R_BN_LIB;
         DSA_SIG *ret = NULL;
         int noredo = 0;
 
-        BN_init(&b);
-        BN_init(&binv);
-        BN_init(&bm);
-        BN_init(&bxr);
-        BN_init(&m);
-
-        if (!dsa->p || !dsa->q || !dsa->g) {
+        if (dsa->p == NULL || dsa->q == NULL || dsa->g == NULL) {
                 reason = DSA_R_MISSING_PARAMETERS;
                 goto err;
         }
 
-        s = BN_new();
-        if (s == NULL)
+        if ((s = BN_new()) == NULL)
                 goto err;
-        ctx = BN_CTX_new();
-        if (ctx == NULL)
+
+        if ((ctx = BN_CTX_new()) == NULL)
+                goto err;
+
+        BN_CTX_start(ctx);
+
+        if ((b = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((binv = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((bm = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((bxr = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((m = BN_CTX_get(ctx)) == NULL)
                 goto err;
 
         /*
@@ -126,7 +133,7 @@ dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa)
          */
         if (dlen > BN_num_bytes(dsa->q))
                 dlen = BN_num_bytes(dsa->q);
-        if (BN_bin2bn(dgst, dlen, &m) == NULL)
+        if (BN_bin2bn(dgst, dlen, m) == NULL)
                 goto err;
 
  redo:
@@ -153,22 +160,22 @@ dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa)
          *
          * Where b is a random value in the range [1, q).
          */
-        if (!bn_rand_interval(&b, BN_value_one(), dsa->q))
+        if (!bn_rand_interval(b, BN_value_one(), dsa->q))
                 goto err;
-        if (BN_mod_inverse_ct(&binv, &b, dsa->q, ctx) == NULL)
+        if (BN_mod_inverse_ct(binv, b, dsa->q, ctx) == NULL)
                 goto err;
 
-        if (!BN_mod_mul(&bxr, &b, dsa->priv_key, dsa->q, ctx))  /* bx */
+        if (!BN_mod_mul(bxr, b, dsa->priv_key, dsa->q, ctx))    /* bx */
                 goto err;
-        if (!BN_mod_mul(&bxr, &bxr, r, dsa->q, ctx))    /* bxr */
+        if (!BN_mod_mul(bxr, bxr, r, dsa->q, ctx))      /* bxr */
                 goto err;
-        if (!BN_mod_mul(&bm, &b, &m, dsa->q, ctx))      /* bm */
+        if (!BN_mod_mul(bm, b, m, dsa->q, ctx))         /* bm */
                 goto err;
-        if (!BN_mod_add(s, &bxr, &bm, dsa->q, ctx))     /* s = bm + bxr */
+        if (!BN_mod_add(s, bxr, bm, dsa->q, ctx))       /* s = bm + bxr */
                 goto err;
         if (!BN_mod_mul(s, s, kinv, dsa->q, ctx))       /* s = b(m + xr)k^-1 */
                 goto err;
-        if (!BN_mod_mul(s, s, &binv, dsa->q, ctx))      /* s = (m + xr)k^-1 */
+        if (!BN_mod_mul(s, s, binv, dsa->q, ctx))       /* s = (m + xr)k^-1 */
                 goto err;
 
         /*
@@ -196,13 +203,9 @@ dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa)
                 BN_free(r);
                 BN_free(s);
         }
+        BN_CTX_end(ctx);
         BN_CTX_free(ctx);
-        BN_clear_free(&b);
-        BN_clear_free(&bm);
-        BN_clear_free(&bxr);
-        BN_clear_free(&binv);
-        BN_clear_free(&m);
-        BN_clear_free(kinv);
+        BN_free(kinv);
 
         return ret;
 }
@@ -210,39 +213,44 @@ dsa_do_sign(const unsigned char *dgst, int dlen, DSA *dsa)
 static int
 dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp)
 {
-        BN_CTX *ctx;
-        BIGNUM k, l, m, *kinv = NULL, *r = NULL;
-        int q_bits, ret = 0;
+        BIGNUM *k = NULL, *l = NULL, *m = NULL, *kinv = NULL, *r = NULL;
+        BN_CTX *ctx = NULL;
+        int q_bits;
+        int ret = 0;
 
-        if (!dsa->p || !dsa->q || !dsa->g) {
+        if (dsa->p == NULL || dsa->q == NULL || dsa->g == NULL) {
                 DSAerror(DSA_R_MISSING_PARAMETERS);
                 return 0;
         }
 
-        BN_init(&k);
-        BN_init(&l);
-        BN_init(&m);
+        if ((r = BN_new()) == NULL)
+                goto err;
 
-        if (ctx_in == NULL) {
-                if ((ctx = BN_CTX_new()) == NULL)
-                        goto err;
-        } else
-                ctx = ctx_in;
+        if ((ctx = ctx_in) == NULL)
+                ctx = BN_CTX_new();
+        if (ctx == NULL)
+                goto err;
 
-        if ((r = BN_new()) == NULL)
+        BN_CTX_start(ctx);
+
+        if ((k = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((l = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((m = BN_CTX_get(ctx)) == NULL)
                 goto err;
 
         /* Preallocate space */
         q_bits = BN_num_bits(dsa->q);
-        if (!BN_set_bit(&k, q_bits) ||
-            !BN_set_bit(&l, q_bits) ||
-            !BN_set_bit(&m, q_bits))
+        if (!BN_set_bit(k, q_bits) ||
+            !BN_set_bit(l, q_bits) ||
+            !BN_set_bit(m, q_bits))
                 goto err;
 
-        if (!bn_rand_interval(&k, BN_value_one(), dsa->q))
+        if (!bn_rand_interval(k, BN_value_one(), dsa->q))
                 goto err;
 
-        BN_set_flags(&k, BN_FLG_CONSTTIME);
+        BN_set_flags(k, BN_FLG_CONSTTIME);
 
         if (dsa->flags & DSA_FLAG_CACHE_MONT_P) {
                 if (!BN_MONT_CTX_set_locked(&dsa->method_mont_p,
@@ -265,17 +273,17 @@ dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp)
          * conditional copy.
          */
 
-        if (!BN_add(&l, &k, dsa->q) ||
-            !BN_add(&m, &l, dsa->q) ||
-            !BN_copy(&k, BN_num_bits(&l) > q_bits ? &l : &m))
+        if (!BN_add(l, k, dsa->q) ||
+            !BN_add(m, l, dsa->q) ||
+            !BN_copy(k, BN_num_bits(l) > q_bits ? l : m))
                 goto err;
 
         if (dsa->meth->bn_mod_exp != NULL) {
-                if (!dsa->meth->bn_mod_exp(dsa, r, dsa->g, &k, dsa->p, ctx,
+                if (!dsa->meth->bn_mod_exp(dsa, r, dsa->g, k, dsa->p, ctx,
                     dsa->method_mont_p))
                         goto err;
         } else {
-                if (!BN_mod_exp_mont_ct(r, dsa->g, &k, dsa->p, ctx,
+                if (!BN_mod_exp_mont_ct(r, dsa->g, k, dsa->p, ctx,
                     dsa->method_mont_p))
                         goto err;
         }
@@ -284,13 +292,14 @@ dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp)
                 goto err;
 
         /* Compute  part of 's = inv(k) (m + xr) mod q' */
-        if ((kinv = BN_mod_inverse_ct(NULL, &k, dsa->q, ctx)) == NULL)
+        if ((kinv = BN_mod_inverse_ct(NULL, k, dsa->q, ctx)) == NULL)
                 goto err;
 
-        BN_clear_free(*kinvp);
+        BN_free(*kinvp);
         *kinvp = kinv;
         kinv = NULL;
-        BN_clear_free(*rp);
+
+        BN_free(*rp);
         *rp = r;
 
         ret = 1;
@@ -298,13 +307,11 @@ dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp)
  err:
         if (!ret) {
                 DSAerror(ERR_R_BN_LIB);
-                BN_clear_free(r);
+                BN_free(r);
         }
-        if (ctx_in == NULL)
+        BN_CTX_end(ctx);
+        if (ctx != ctx_in)
                 BN_CTX_free(ctx);
-        BN_clear_free(&k);
-        BN_clear_free(&l);
-        BN_clear_free(&m);
 
         return ret;
 }
@@ -312,13 +319,13 @@ dsa_sign_setup(DSA *dsa, BN_CTX *ctx_in, BIGNUM **kinvp, BIGNUM **rp)
 static int
 dsa_do_verify(const unsigned char *dgst, int dgst_len, DSA_SIG *sig, DSA *dsa)
 {
-        BN_CTX *ctx;
-        BIGNUM u1, u2, t1;
+        BIGNUM *u1 = NULL, *u2 = NULL, *t1 = NULL;
+        BN_CTX *ctx = NULL;
         BN_MONT_CTX *mont = NULL;
         int qbits;
         int ret = -1;
 
-        if (!dsa->p || !dsa->q || !dsa->g) {
+        if (dsa->p == NULL || dsa->q == NULL || dsa->g == NULL) {
                 DSAerror(DSA_R_MISSING_PARAMETERS);
                 return -1;
         }
@@ -334,13 +341,18 @@ dsa_do_verify(const unsigned char *dgst, int dgst_len, DSA_SIG *sig, DSA *dsa)
                 return -1;
         }
 
-        BN_init(&u1);
-        BN_init(&u2);
-        BN_init(&t1);
-
         if ((ctx = BN_CTX_new()) == NULL)
                 goto err;
 
+        BN_CTX_start(ctx);
+
+        if ((u1 = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((u2 = BN_CTX_get(ctx)) == NULL)
+                goto err;
+        if ((t1 = BN_CTX_get(ctx)) == NULL)
+                goto err;
+
         if (BN_is_zero(sig->r) || BN_is_negative(sig->r) ||
             BN_ucmp(sig->r, dsa->q) >= 0) {
                 ret = 0;
@@ -353,7 +365,7 @@ dsa_do_verify(const unsigned char *dgst, int dgst_len, DSA_SIG *sig, DSA *dsa)
         }
 
         /* Calculate w = inv(s) mod q, saving w in u2. */
-        if ((BN_mod_inverse_ct(&u2, sig->s, dsa->q, ctx)) == NULL)
+        if ((BN_mod_inverse_ct(u2, sig->s, dsa->q, ctx)) == NULL)
                 goto err;
 
         /*
@@ -364,15 +376,15 @@ dsa_do_verify(const unsigned char *dgst, int dgst_len, DSA_SIG *sig, DSA *dsa)
                 dgst_len = (qbits >> 3);
 
         /* Save m in u1. */
-        if (BN_bin2bn(dgst, dgst_len, &u1) == NULL)
+        if (BN_bin2bn(dgst, dgst_len, u1) == NULL)
                 goto err;
 
         /* u1 = m * w mod q */
-        if (!BN_mod_mul(&u1, &u1, &u2, dsa->q, ctx))
+        if (!BN_mod_mul(u1, u1, u2, dsa->q, ctx))
                 goto err;
 
         /* u2 = r * w mod q */
-        if (!BN_mod_mul(&u2, sig->r, &u2, dsa->q, ctx))
+        if (!BN_mod_mul(u2, sig->r, u2, dsa->q, ctx))
                 goto err;
 
         if (dsa->flags & DSA_FLAG_CACHE_MONT_P) {
@@ -383,30 +395,27 @@ dsa_do_verify(const unsigned char *dgst, int dgst_len, DSA_SIG *sig, DSA *dsa)
         }
 
         if (dsa->meth->dsa_mod_exp != NULL) {
-                if (!dsa->meth->dsa_mod_exp(dsa, &t1, dsa->g, &u1, dsa->pub_key,
-                    &u2, dsa->p, ctx, mont))
+                if (!dsa->meth->dsa_mod_exp(dsa, t1, dsa->g, u1, dsa->pub_key,
+                    u2, dsa->p, ctx, mont))
                         goto err;
         } else {
-                if (!BN_mod_exp2_mont(&t1, dsa->g, &u1, dsa->pub_key, &u2,
+                if (!BN_mod_exp2_mont(t1, dsa->g, u1, dsa->pub_key, u2,
                     dsa->p, ctx, mont))
                         goto err;
         }
 
-        /* BN_copy(&u1,&t1); */
         /* let u1 = u1 mod q */
-        if (!BN_mod_ct(&u1, &t1, dsa->q, ctx))
+        if (!BN_mod_ct(u1, t1, dsa->q, ctx))
                 goto err;
 
         /* v is in u1 - if the signature is correct, it will be equal to r. */
-        ret = BN_ucmp(&u1, sig->r) == 0;
+        ret = BN_ucmp(u1, sig->r) == 0;
 
  err:
         if (ret < 0)
                 DSAerror(ERR_R_BN_LIB);
+        BN_CTX_end(ctx);
         BN_CTX_free(ctx);
-        BN_free(&u1);
-        BN_free(&u2);
-        BN_free(&t1);
 
         return ret;
 }