Change DH_get_nid() to set the value of q if it is not already set

Fixes #11108.

It only sets q if a valid named group is found.
The function signature was recently changed to pass a non const DH pointer
in order to allow the nid to be cached internally. As an extension of this
the value of q can now also be set as q is always known for named groups.
The length field is also set if q is set.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/11114)

7 files changed, 110 insertions, 82 deletions

diff --git a/crypto/dh/dh_group_params.c b/crypto/dh/dh_group_params.c
index d2dd258248..d672ae3034 100644
--- a/crypto/dh/dh_group_params.c
+++ b/crypto/dh/dh_group_params.c
@@ -22,6 +22,33 @@
 #include <openssl/objects.h>
 #include "crypto/bn_dh.h"
 #include "crypto/dh.h"
+#include "crypto/security_bits.h"
+
+
+#define FFDHE(sz) { NID_ffdhe##sz, sz, &_bignum_ffdhe##sz##_p }
+#define MODP(sz)  { NID_modp_##sz, sz, &_bignum_modp_##sz##_p }
+
+typedef struct safe_prime_group_st {
+    int nid;
+    int32_t nbits;
+    const BIGNUM *p;
+} SP_GROUP;
+
+static const SP_GROUP sp_groups[] = {
+    FFDHE(2048),
+    FFDHE(3072),
+    FFDHE(4096),
+    FFDHE(6144),
+    FFDHE(8192),
+#ifndef FIPS_MODE
+    MODP(1536),
+#endif
+    MODP(2048),
+    MODP(3072),
+    MODP(4096),
+    MODP(6144),
+    MODP(8192),
+};
 
 #ifndef FIPS_MODE
 static DH *dh_new_by_nid_with_ctx(OPENSSL_CTX *libctx, int nid);
@@ -54,40 +81,24 @@ static DH *dh_param_init(OPENSSL_CTX *libctx, int nid, const BIGNUM *p,
 
 static DH *dh_new_by_nid_with_ctx(OPENSSL_CTX *libctx, int nid)
 {
-    /*
-     * The last parameter specified in these fields is
-     * 2 * max_target_security_strength.
-     * See SP800-56Ar3 Table(s) 25 & 26.
-     */
-    switch (nid) {
-    case NID_ffdhe2048:
-        return dh_param_init(libctx, nid, &_bignum_ffdhe2048_p, 225);
-    case NID_ffdhe3072:
-        return dh_param_init(libctx, nid, &_bignum_ffdhe3072_p, 275);
-    case NID_ffdhe4096:
-        return dh_param_init(libctx, nid, &_bignum_ffdhe4096_p, 325);
-    case NID_ffdhe6144:
-        return dh_param_init(libctx, nid, &_bignum_ffdhe6144_p, 375);
-    case NID_ffdhe8192:
-        return dh_param_init(libctx, nid, &_bignum_ffdhe8192_p, 400);
-#ifndef FIPS_MODE
-    case NID_modp_1536:
-        return dh_param_init(libctx, nid, &_bignum_modp_1536_p, 190);
-#endif
-    case NID_modp_2048:
-        return dh_param_init(libctx, nid, &_bignum_modp_2048_p, 225);
-    case NID_modp_3072:
-        return dh_param_init(libctx, nid, &_bignum_modp_3072_p, 275);
-    case NID_modp_4096:
-        return dh_param_init(libctx, nid, &_bignum_modp_4096_p, 325);
-    case NID_modp_6144:
-        return dh_param_init(libctx, nid, &_bignum_modp_6144_p, 375);
-    case NID_modp_8192:
-        return dh_param_init(libctx, nid, &_bignum_modp_8192_p, 400);
-    default:
-        DHerr(0, DH_R_INVALID_PARAMETER_NID);
-        return NULL;
+    int i;
+
+    for (i = 0; i < (int)OSSL_NELEM(sp_groups); ++i) {
+        if (sp_groups[i].nid == nid) {
+            int max_target_security_strength =
+                ifc_ffc_compute_security_bits(sp_groups[i].nbits);
+
+            /*
+             * The last parameter specified here is
+             * 2 * max_target_security_strength.
+             * See SP800-56Ar3 Table(s) 25 & 26.
+             */
+            return dh_param_init(libctx, nid, sp_groups[i].p,
+                                 2 * max_target_security_strength);
+        }
     }
+    DHerr(0, DH_R_INVALID_PARAMETER_NID);
+    return NULL;
 }
 
 DH *DH_new_by_nid(int nid)
@@ -98,49 +109,44 @@ DH *DH_new_by_nid(int nid)
 
 int DH_get_nid(DH *dh)
 {
-    int nid = dh->params.nid;
+    BIGNUM *q = NULL;
+    int i, nid;
+
+    if (dh == NULL)
+        return NID_undef;
 
+    nid = dh->params.nid;
+    /* Just return if it is already cached */
     if (nid != NID_undef)
         return nid;
 
     if (BN_get_word(dh->params.g) != 2)
         return NID_undef;
-    if (!BN_cmp(dh->params.p, &_bignum_ffdhe2048_p))
-        nid = NID_ffdhe2048;
-    else if (!BN_cmp(dh->params.p, &_bignum_ffdhe3072_p))
-        nid = NID_ffdhe3072;
-    else if (!BN_cmp(dh->params.p, &_bignum_ffdhe4096_p))
-        nid = NID_ffdhe4096;
-    else if (!BN_cmp(dh->params.p, &_bignum_ffdhe6144_p))
-        nid = NID_ffdhe6144;
-    else if (!BN_cmp(dh->params.p, &_bignum_ffdhe8192_p))
-        nid = NID_ffdhe8192;
-#ifndef FIPS_MODE
-    else if (!BN_cmp(dh->params.p, &_bignum_modp_1536_p))
-        nid = NID_modp_1536;
-#endif
-    else if (!BN_cmp(dh->params.p, &_bignum_modp_2048_p))
-        nid = NID_modp_2048;
-    else if (!BN_cmp(dh->params.p, &_bignum_modp_3072_p))
-        nid = NID_modp_3072;
-    else if (!BN_cmp(dh->params.p, &_bignum_modp_4096_p))
-        nid = NID_modp_4096;
-    else if (!BN_cmp(dh->params.p, &_bignum_modp_6144_p))
-        nid = NID_modp_6144;
-    else if (!BN_cmp(dh->params.p, &_bignum_modp_8192_p))
-        nid = NID_modp_8192;
-    else
-        return NID_undef;
 
-    /* Verify q is correct if it exists - reset the nid if it is not correct */
-    if (dh->params.q != NULL) {
-        BIGNUM *q = BN_dup(dh->params.p);
+    for (i = 0; i < (int)OSSL_NELEM(sp_groups); ++i) {
+        /* If a matching p is found then we will break out of the loop */
+        if (!BN_cmp(dh->params.p, sp_groups[i].p)) {
+            /* Set q = (p - 1) / 2 (p is known to be odd so just shift right ) */
+            q = BN_dup(dh->params.p);
 
-        /* Check q = p * 2 + 1 we already know q is odd, so just shift right */
-        if (q == NULL || !BN_rshift1(q, q) || (BN_cmp(dh->params.q, q) != 0))
-            nid = NID_undef;
-        BN_free(q);
+            if (q == NULL || !BN_rshift1(q, q))
+                break; /* returns nid = NID_undef on failure */
+
+            /* Verify q is correct if it exists */
+            if (dh->params.q != NULL) {
+                if (BN_cmp(dh->params.q, q) != 0)
+                    break;  /* returns nid = NID_undef if q does not match */
+            } else {
+                /* assign the calculated q */
+                dh->params.q = q;
+                q = NULL; /* set to NULL so it is not freed */
+            }
+            dh->params.nid = sp_groups[i].nid; /* cache the nid */
+            dh->length = 2 * ifc_ffc_compute_security_bits(sp_groups[i].nbits);
+            dh->dirty_cnt++;
+            break;
+        }
     }
-    dh->params.nid = nid; /* cache the nid */
+    BN_free(q);
     return nid;
 }
diff --git a/crypto/dh/dh_key.c b/crypto/dh/dh_key.c
index 5748be810f..ab2e25ea87 100644
--- a/crypto/dh/dh_key.c
+++ b/crypto/dh/dh_key.c
@@ -251,8 +251,7 @@ static int generate_key(DH *dh)
              * (where s = max security strength supported).
              * N = dh->length (N = maximum bit length of private key)
              */
-            if (dh->length == 0
-                || dh->params.q == NULL
+            if (dh->params.q == NULL
                 || dh->length > BN_num_bits(dh->params.q))
                 goto err;
             if (!ffc_generate_private_key(ctx, &dh->params, dh->length,
diff --git a/crypto/dh/dh_lib.c b/crypto/dh/dh_lib.c
index d7fe850f58..29152dca4d 100644
--- a/crypto/dh/dh_lib.c
+++ b/crypto/dh/dh_lib.c
@@ -211,11 +211,16 @@ int DH_set0_pqg(DH *dh, BIGNUM *p, BIGNUM *q, BIGNUM *g)
 
     ffc_params_set0_pqg(&dh->params, p, q, g);
     dh->params.nid = NID_undef;
-    DH_get_nid(dh); /* Check if this is a named group and cache it */
-
-    if (q != NULL)
-        dh->length = BN_num_bits(q);
-
+    /*
+     * Check if this is a named group. If it finds a named group then the
+     * 'q' and 'length' value are either already set or are set by the
+     * call.
+     */
+    if (DH_get_nid(dh) == NID_undef) {
+        /* If its not a named group then set the 'length' if q is not NULL */
+        if (q != NULL)
+            dh->length = BN_num_bits(q);
+    }
     dh->dirty_cnt++;
     return 1;
 }
diff --git a/crypto/ffc/ffc_key_generate.c b/crypto/ffc/ffc_key_generate.c
index b8c85480c1..078e8d39a1 100644
--- a/crypto/ffc/ffc_key_generate.c
+++ b/crypto/ffc/ffc_key_generate.c
@@ -36,13 +36,19 @@ int ffc_generate_private_key(BN_CTX *ctx, const FFC_PARAMS *params,
 int ffc_generate_private_key_fips(BN_CTX *ctx, const FFC_PARAMS *params,
                                   int N, int s, BIGNUM *priv)
 {
-    int ret = 0;
+    int ret = 0, qbits = BN_num_bits(params->q);
     BIGNUM *m, *two_powN = NULL;
 
     /* Step (2) : check range of N */
-    if (N < 2 * s || N > BN_num_bits(params->q))
+    if (N < 2 * s || N > qbits)
         return 0;
 
+    /* Deal with the edge case where the value of N is not set */
+    if (N == 0) {
+        N = qbits;
+        s = N / 2;
+    }
+
     two_powN = BN_new();
     /* 2^N */
     if (two_powN == NULL || !BN_lshift(two_powN, BN_value_one(), N))
@@ -50,6 +56,7 @@ int ffc_generate_private_key_fips(BN_CTX *ctx, const FFC_PARAMS *params,
 
     /* Step (5) : M = min(2 ^ N, q) */
     m = (BN_cmp(two_powN, params->q) > 0) ? params->q : two_powN;
+
     do {
         /* Steps (3, 4 & 7) :  c + 1 = 1 + random[0..2^N - 1] */
         if (!BN_priv_rand_range_ex(priv, two_powN, ctx)
diff --git a/crypto/rsa/rsa_lib.c b/crypto/rsa/rsa_lib.c
index 08ce8b4ef8..ada5388bb2 100644
--- a/crypto/rsa/rsa_lib.c
+++ b/crypto/rsa/rsa_lib.c
@@ -23,6 +23,7 @@
 #include "crypto/bn.h"
 #include "crypto/evp.h"
 #include "crypto/rsa.h"
+#include "crypto/security_bits.h"
 #include "rsa_local.h"
 
 static RSA *rsa_new_intern(ENGINE *engine, OPENSSL_CTX *libctx);
@@ -281,11 +282,20 @@ static uint32_t ilog_e(uint64_t v)
  * NIST SP 800-56B rev 2 Appendix D: Maximum Security Strength Estimates for IFC
  * Modulus Lengths.
  *
+ * Note that this formula is also referred to in SP800-56A rev3 Appendix D:
+ * for FFC safe prime groups for modp and ffdhe.
+ * After Table 25 and Table 26 it refers to
+ * "The maximum security strength estimates were calculated using the formula in
+ * Section 7.5 of the FIPS 140 IG and rounded to the nearest multiple of eight
+ * bits".
+ *
+ * The formula is:
+ *
  * E = \frac{1.923 \sqrt[3]{nBits \cdot log_e(2)}
  *           \cdot(log_e(nBits \cdot log_e(2))^{2/3} - 4.69}{log_e(2)}
  * The two cube roots are merged together here.
  */
-uint16_t rsa_compute_security_bits(int n)
+uint16_t ifc_ffc_compute_security_bits(int n)
 {
     uint64_t x;
     uint32_t lx;
@@ -322,6 +332,8 @@ uint16_t rsa_compute_security_bits(int n)
     return (y + 4) & ~7;
 }
 
+
+
 int RSA_security_bits(const RSA *rsa)
 {
     int bits = BN_num_bits(rsa->n);
@@ -335,7 +347,7 @@ int RSA_security_bits(const RSA *rsa)
             return 0;
     }
 #endif
-    return rsa_compute_security_bits(bits);
+    return ifc_ffc_compute_security_bits(bits);
 }
 
 int RSA_set0_key(RSA *r, BIGNUM *n, BIGNUM *e, BIGNUM *d)
diff --git a/crypto/rsa/rsa_local.h b/crypto/rsa/rsa_local.h
index 11d7635c35..ac8856207e 100644
--- a/crypto/rsa/rsa_local.h
+++ b/crypto/rsa/rsa_local.h
@@ -137,8 +137,6 @@ RSA_PRIME_INFO *rsa_multip_info_new(void);
 int rsa_multip_calc_product(RSA *rsa);
 int rsa_multip_cap(int bits);
 
-uint16_t rsa_compute_security_bits(int n);
-
 int rsa_sp800_56b_validate_strength(int nbits, int strength);
 int rsa_check_pminusq_diff(BIGNUM *diff, const BIGNUM *p, const BIGNUM *q,
                            int nbits);
diff --git a/crypto/rsa/rsa_sp800_56b_gen.c b/crypto/rsa/rsa_sp800_56b_gen.c
index 1f8d01d477..a60a428b14 100644
--- a/crypto/rsa/rsa_sp800_56b_gen.c
+++ b/crypto/rsa/rsa_sp800_56b_gen.c
@@ -11,6 +11,7 @@
 #include <openssl/err.h>
 #include <openssl/bn.h>
 #include "crypto/bn.h"
+#include "crypto/security_bits.h"
 #include "rsa_local.h"
 
 #define RSA_FIPS1864_MIN_KEYGEN_KEYSIZE 2048
@@ -144,7 +145,7 @@ err:
  */
 int rsa_sp800_56b_validate_strength(int nbits, int strength)
 {
-    int s = (int)rsa_compute_security_bits(nbits);
+    int s = (int)ifc_ffc_compute_security_bits(nbits);
 
     if (s < RSA_FIPS1864_MIN_KEYGEN_STRENGTH
             || s > RSA_FIPS1864_MAX_KEYGEN_STRENGTH) {