Validate server's group

The client wasn't validating the DH group parameters in the KEX DH GEX
Group message. This adds a function to perform the validation of the
prime `p` to verify it is safe. (Prime and that ((p - 1) / 2) is
prime.) Also adds a test to generate a known unsafe prime to verify the
function rejects it appropriately.

Affected function: DoKexDhGexGroup.
Issue: F-1688

Author: ejohnstown

src/internal.c

@@ -6284,6 +6284,144 @@ static int DoKexDhGexRequest(WOLFSSH* ssh,
 }
 
 
+/*
+ * Validate a DH GEX group received from the server per RFC 4419 sec. 3:
+ *   - p's bit length falls within the client's requested [minBits, maxBits]
+ *   - p is odd and probably prime
+ *   - (p-1)/2 is also probably prime, i.e. p is a safe prime, which bounds
+ *     the order of g to q or 2q and closes the small-subgroup attack
+ *   - 2 <= g <= p - 2
+ * Returns WS_SUCCESS if the group is acceptable.
+ */
+static int ValidateKexDhGexGroup(const byte* primeGroup, word32 primeGroupSz,
+        const byte* generator, word32 generatorSz,
+        word32 minBits, word32 maxBits, WC_RNG* rng)
+{
+    mp_int p;
+    mp_int g;
+    mp_int q;
+    int pInit = 0, gInit = 0, qInit = 0;
+    int bits;
+    int isPrime = MP_NO;
+    int ret = WS_SUCCESS;
+
+    if (primeGroup == NULL || primeGroupSz == 0
+            || generator == NULL || generatorSz == 0
+            || rng == NULL)
+        ret = WS_BAD_ARGUMENT;
+
+    /*
+     * Check the bounds on the size of the flat prime group and generator
+     * values. The prime group shall be LE maxBits. The generator size
+     * shall be LE prime group size. This is a check on the sizes of values
+     * sent by the peer before reading them in and checking them as mp_ints.
+     */
+    if (ret == WS_SUCCESS) {
+        word32 maxBytes = (maxBits / 8) + ((maxBits % 8) ? 1 : 0);
+        /* Adjust the sizes for signed padding. */
+        word32 adjPrimeGroupSz = primeGroupSz - ((primeGroup[0] == 0) ? 1 : 0);
+        word32 adjGeneratorSz = generatorSz - ((generator[0] == 0) ? 1 : 0);
+
+        if (adjPrimeGroupSz > maxBytes || adjGeneratorSz > adjPrimeGroupSz) {
+            ret = WS_DH_SIZE_E;
+        }
+    }
+
+    if (ret == WS_SUCCESS) {
+        if (mp_init(&p) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+        else
+            pInit = 1;
+    }
+    if (ret == WS_SUCCESS) {
+        if (mp_init(&g) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+        else
+            gInit = 1;
+    }
+    if (ret == WS_SUCCESS) {
+        if (mp_init(&q) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+        else
+            qInit = 1;
+    }
+
+    if (ret == WS_SUCCESS) {
+        if (mp_read_unsigned_bin(&p, primeGroup, primeGroupSz) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+    }
+    if (ret == WS_SUCCESS) {
+        if (mp_read_unsigned_bin(&g, generator, generatorSz) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+    }
+
+    if (ret == WS_SUCCESS) {
+        bits = mp_count_bits(&p);
+        if (bits < (int)minBits || bits > (int)maxBits) {
+            WLOG(WS_LOG_DEBUG,
+                    "DH GEX: prime size %d outside requested [%u, %u]",
+                    bits, minBits, maxBits);
+            ret = WS_DH_SIZE_E;
+        }
+    }
+
+    if (ret == WS_SUCCESS) {
+        if (!mp_isodd(&p)) {
+            WLOG(WS_LOG_DEBUG, "DH GEX: prime is even");
+            ret = WS_CRYPTO_FAILED;
+        }
+    }
+
+    /* 2 <= g: reject g == 0 and g == 1. */
+    if (ret == WS_SUCCESS) {
+        if (mp_cmp_d(&g, 1) != MP_GT) {
+            WLOG(WS_LOG_DEBUG, "DH GEX: generator < 2");
+            ret = WS_CRYPTO_FAILED;
+        }
+    }
+
+    /* g <= p - 2: reject g == p - 1 (order 2) and anything larger. */
+    if (ret == WS_SUCCESS) {
+        if (mp_sub_d(&p, 1, &q) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+        else if (mp_cmp(&g, &q) != MP_LT) {
+            WLOG(WS_LOG_DEBUG, "DH GEX: generator >= p - 1");
+            ret = WS_CRYPTO_FAILED;
+        }
+    }
+
+    /* Miller-Rabin: p must be prime. */
+    if (ret == WS_SUCCESS) {
+        isPrime = MP_NO;
+        if (mp_prime_is_prime(&p, WOLFSSH_MR_ROUNDS, &isPrime) != MP_OKAY
+                || isPrime == MP_NO) {
+            WLOG(WS_LOG_DEBUG, "DH GEX: p is not prime");
+            ret = WS_CRYPTO_FAILED;
+        }
+    }
+
+    /* Safe prime check: q = (p - 1) / 2 must also be prime. */
+    if (ret == WS_SUCCESS) {
+        if (mp_sub_d(&p, 1, &q) != MP_OKAY || mp_div_2(&q, &q) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+    }
+    if (ret == WS_SUCCESS) {
+        isPrime = MP_NO;
+        if (mp_prime_is_prime(&q, WOLFSSH_MR_ROUNDS, &isPrime) != MP_OKAY
+                || isPrime == MP_NO) {
+            WLOG(WS_LOG_DEBUG, "DH GEX: (p-1)/2 is not prime, p is not safe");
+            ret = WS_CRYPTO_FAILED;
+        }
+    }
+
+    if (qInit) mp_clear(&q);
+    if (gInit) mp_clear(&g);
+    if (pInit) mp_clear(&p);
+
+    return ret;
+}
+
+
 static int DoKexDhGexGroup(WOLFSSH* ssh,
                            byte* buf, word32 len, word32* idx)
 {
@@ -6300,13 +6438,19 @@ static int DoKexDhGexGroup(WOLFSSH* ssh,
     if (ret == WS_SUCCESS) {
         begin = *idx;
         ret = GetMpint(&primeGroupSz, &primeGroup, buf, len, &begin);
-        if (ret == WS_SUCCESS && primeGroupSz > (MAX_KEX_KEY_SZ + 1))
-            ret = WS_DH_SIZE_E;
     }
 
     if (ret == WS_SUCCESS)
         ret = GetMpint(&generatorSz, &generator, buf, len, &begin);
 
+    if (ret == WS_SUCCESS) {
+        ret = ValidateKexDhGexGroup(primeGroup, primeGroupSz,
+                generator, generatorSz,
+                ssh->handshake->dhGexMinSz,
+                ssh->handshake->dhGexMaxSz,
+                ssh->rng);
+    }
+
     if (ret == WS_SUCCESS) {
         if (ssh->handshake->primeGroup)
             WFREE(ssh->handshake->primeGroup, ssh->ctx->heap, DYNTYPE_MPINT);
@@ -6340,7 +6484,17 @@ static int DoKexDhGexGroup(WOLFSSH* ssh,
 
     return ret;
 }
+
+#ifdef WOLFSSH_TEST_INTERNAL
+int wolfSSH_TestValidateKexDhGexGroup(const byte* primeGroup,
+        word32 primeGroupSz, const byte* generator, word32 generatorSz,
+        word32 minBits, word32 maxBits, WC_RNG* rng)
+{
+    return ValidateKexDhGexGroup(primeGroup, primeGroupSz,
+            generator, generatorSz, minBits, maxBits, rng);
+}
 #endif
+#endif /* !WOLFSSH_NO_DH_GEX_SHA256 */
 
 
 static int DoIgnore(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)

tests/unit.c

@@ -31,7 +31,10 @@
 #include <stdio.h>
 #include <wolfssh/ssh.h>
 #include <wolfssh/keygen.h>
+#include <wolfssh/error.h>
 #include <wolfssh/internal.h>
+#include <wolfssl/wolfcrypt/random.h>
+#include <wolfssl/wolfcrypt/integer.h>
 #include <wolfssl/wolfcrypt/hmac.h>
 
 #define WOLFSSH_TEST_HEX2BIN
@@ -439,6 +442,57 @@ static int test_DoReceive_VerifyMacFailure(void)
 #endif /* WOLFSSH_TEST_INTERNAL && any HMAC SHA variant enabled */
 
 
+#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_DH_GEX_SHA256)
+
+/*
+ * For testing the ValidateKexDhGexGroup() function, we need to verify that
+ * that the function detects unsafe primes. The following unsafePrime is
+ * the prime used with GOST-ECC. It is fine for that. For DH, it isn't safe.
+ */
+
+const char unsafePrime[] =
+    "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"
+    "fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffdc7";
+
+
+static int test_DhGexGroupValidate(void)
+{
+    WC_RNG rng;
+    byte* pFlat = NULL;
+    word32 pFlatSz = 0;
+    byte gFlat = 2;
+    int result = 0, ret;
+
+    if (wc_InitRng(&rng) != 0) {
+        printf("DhGexGroupValidate: wc_InitRng failed\n");
+        result = -110;
+        goto cleanup;
+    }
+
+    ret = ConvertHexToBin(unsafePrime, &pFlat, &pFlatSz,
+            NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL);
+    if (ret != 0) {
+        result = -113;
+        goto cleanup;
+    }
+
+    ret = wolfSSH_TestValidateKexDhGexGroup(pFlat, pFlatSz, &gFlat, 1,
+            512, 8192, &rng);
+    if (ret != WS_CRYPTO_FAILED) {
+        printf("DhGexGroupValidate: validator returned %d, expected %d\n",
+                ret, WS_CRYPTO_FAILED);
+        result = -121;
+    }
+
+cleanup:
+    FreeBins(pFlat, NULL, NULL, NULL);
+    wc_FreeRng(&rng);
+    return result;
+}
+
+#endif /* WOLFSSH_TEST_INTERNAL && !WOLFSSH_NO_DH_GEX_SHA256 */
+
+
 /* Error Code And Message Test */
 
 static int test_Errors(void)
@@ -520,6 +574,13 @@ int wolfSSH_UnitTest(int argc, char** argv)
     testResult = testResult || unitResult;
 #endif
 
+#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_DH_GEX_SHA256)
+    unitResult = test_DhGexGroupValidate();
+    printf("DhGexGroupValidate: %s\n",
+            (unitResult == 0 ? "SUCCESS" : "FAILED"));
+    testResult = testResult || unitResult;
+#endif
+
 #ifdef WOLFSSH_KEYGEN
 #ifndef WOLFSSH_NO_RSA
     unitResult = test_RsaKeyGen();

wolfssh/internal.h

@@ -476,6 +476,9 @@ enum NameIdType {
         #define MAX_KEX_KEY_SZ (WOLFSSH_DEFAULT_GEXDH_MAX / 8)
     #endif
 #endif
+#ifndef WOLFSSH_MR_ROUNDS
+    #define WOLFSSH_MR_ROUNDS 8
+#endif
 #ifndef WOLFSSH_MAX_FILE_SIZE
     #define WOLFSSH_MAX_FILE_SIZE (1024ul * 1024ul * 4)
 #endif
@@ -1324,7 +1327,12 @@ enum WS_MessageIdLimits {
     WOLFSSH_API int wolfSSH_TestIsMessageAllowed(WOLFSSH* ssh, byte msg,
             byte state);
     WOLFSSH_API int wolfSSH_TestDoReceive(WOLFSSH* ssh);
-#endif
+#ifndef WOLFSSH_NO_DH_GEX_SHA256
+    WOLFSSH_API int wolfSSH_TestValidateKexDhGexGroup(const byte* primeGroup,
+            word32 primeGroupSz, const byte* generator, word32 generatorSz,
+            word32 minBits, word32 maxBits, WC_RNG* rng);
+#endif /* !WOLFSSH_NO_DH_GEX_SHA256 */
+#endif /* WOLFSSH_TEST_INTERNAL */
 
 /* dynamic memory types */
 enum WS_DynamicTypes {