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 an unsafe prime and verify the
function rejects it appropriately.

Affected function: DoKexDhGexGroup.
Issue: F-1688

Author: ejohnstown

src/internal.c

@@ -6284,6 +6284,143 @@ 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)
+{
+    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)
+        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, 8, &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 +6437,18 @@ 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);
+    }
+
     if (ret == WS_SUCCESS) {
         if (ssh->handshake->primeGroup)
             WFREE(ssh->handshake->primeGroup, ssh->ctx->heap, DYNTYPE_MPINT);
@@ -6340,7 +6482,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)
+{
+    return ValidateKexDhGexGroup(primeGroup, primeGroupSz,
+            generator, generatorSz, minBits, maxBits);
+}
 #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,143 @@ 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)
+
+/*
+ * DH GEX Group Validation Unit Test
+ *
+ * Generate a prime p of bitSz bits where p is prime but (p - 1) / 2
+ * is even, so ValidateKexDhGexGroup must reject it at the safe-prime
+ * check. Any prime p congruent to 1 (mod 4) satisfies this: p - 1 is
+ * then divisible by 4, so (p - 1) / 2 is even, and for p > 5 it is
+ * composite.
+ *
+ * Strategy: draw a random candidate with the top bit forced (to fix the
+ * bit length) and the low two bits forced to 01 (odd, congruent to 1
+ * mod 4), then Miller-Rabin test and step by 4 on failure to preserve
+ * the mod-4 invariant.
+ */
+static int GenerateNonSafePrime(mp_int* p, int bitSz, WC_RNG* rng)
+{
+    byte buf[512];
+    int  bytes     = (bitSz + 7) / 8;
+    int  extraBits = bytes * 8 - bitSz;
+    int  isPrime   = MP_NO;
+    int  ret;
+
+    if (p == NULL || rng == NULL || bitSz < 8
+            || bytes > (int)sizeof(buf))
+        return WS_BAD_ARGUMENT;
+
+    ret = wc_RNG_GenerateBlock(rng, buf, (word32)bytes);
+    if (ret != 0)
+        return ret;
+
+    buf[0] &= (byte)(0xFF >> extraBits);
+    buf[0] |= (byte)(0x80 >> extraBits);
+    buf[bytes - 1] = (byte)((buf[bytes - 1] & 0xFC) | 0x01);
+
+    ret = mp_read_unsigned_bin(p, buf, (word32)bytes);
+    if (ret != MP_OKAY)
+        return ret;
+
+    for (;;) {
+        ret = mp_prime_is_prime(p, 8, &isPrime);
+        if (ret != MP_OKAY)
+            return ret;
+        if (isPrime == MP_YES)
+            break;
+        ret = mp_add_d(p, 4, p);
+        if (ret != MP_OKAY)
+            return ret;
+    }
+
+    return MP_OKAY;
+}
+
+
+static int test_DhGexGroupValidate(void)
+{
+    WC_RNG rng;
+    mp_int p;
+    mp_int q;
+    byte pBuf[256];
+    byte gBuf[1] = { 2 };
+    word32 pSz;
+    int isPrime = MP_NO;
+    int result = 0;
+    int ret;
+
+    if (wc_InitRng(&rng) != 0) {
+        printf("DhGexGroupValidate: wc_InitRng failed\n");
+        return -110;
+    }
+    if (mp_init(&p) != MP_OKAY) {
+        wc_FreeRng(&rng);
+        return -111;
+    }
+    if (mp_init(&q) != MP_OKAY) {
+        mp_clear(&p);
+        wc_FreeRng(&rng);
+        return -112;
+    }
+
+    /* 1024 bits keeps the Miller-Rabin loop quick while still landing
+     * inside the default [dhGexMinSz, dhGexMaxSz] window. */
+    if (GenerateNonSafePrime(&p, 1024, &rng) != MP_OKAY) {
+        printf("DhGexGroupValidate: failed to generate candidate prime\n");
+        result = -113;
+        goto done;
+    }
+
+    /* Sanity check: p is prime. */
+    if (mp_prime_is_prime(&p, 20, &isPrime) != MP_OKAY || isPrime != MP_YES) {
+        printf("DhGexGroupValidate: candidate p is not prime\n");
+        result = -114;
+        goto done;
+    }
+
+    /* Sanity check: (p - 1) / 2 is even (i.e. p is not a safe prime). */
+    if (mp_sub_d(&p, 1, &q) != MP_OKAY || mp_div_2(&q, &q) != MP_OKAY) {
+        result = -115;
+        goto done;
+    }
+    if (mp_isodd(&q)) {
+        printf("DhGexGroupValidate: (p-1)/2 was odd, retry needed\n");
+        result = -116;
+        goto done;
+    }
+
+    pSz = (word32)mp_unsigned_bin_size(&p);
+    if (pSz > sizeof(pBuf)) {
+        result = -117;
+        goto done;
+    }
+    if (mp_to_unsigned_bin(&p, pBuf) != MP_OKAY) {
+        result = -118;
+        goto done;
+    }
+
+    /* The validator must reject the group at the safe-prime check. */
+    ret = wolfSSH_TestValidateKexDhGexGroup(pBuf, pSz, gBuf,
+            (word32)sizeof(gBuf), 1024, 8192);
+    if (ret != WS_CRYPTO_FAILED) {
+        printf("DhGexGroupValidate: validator returned %d, expected %d\n",
+                ret, WS_CRYPTO_FAILED);
+        result = -119;
+        goto done;
+    }
+
+done:
+    mp_clear(&q);
+    mp_clear(&p);
+    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 +660,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);
+#endif /* WOLFSSH_NO_DH_GEX_SHA256 */
+#endif /* WOLFSSH_TEST_INTERNAL */
 
 /* dynamic memory types */
 enum WS_DynamicTypes {