libgcrypt, mbedtls, openssl: validate DH public value

Also add tests.

Reported-by: yaotushaozhu on github
Ref: GHSA-m9rg-h9jv-9gvh

Closes #2239

src/crypto.h

@@ -371,6 +371,7 @@ int ssh2_bn_to_bin(const ssh2_bn *bn, unsigned char *bin);
 void ssh2_dh_init(ssh2_dh_ctx *dhctx);
 int ssh2_dh_key_pair(ssh2_dh_ctx *dhctx, ssh2_bn *pub, ssh2_bn *g,
                      ssh2_bn *p, int group_order, ssh2_bn_ctx *bnctx);
+int ssh2_dh_is_valid(ssh2_bn *f, ssh2_bn *p); /* for unit tests */
 int ssh2_dh_secret(ssh2_dh_ctx *dhctx, ssh2_bn *secret, ssh2_bn *f,
                    ssh2_bn *p, ssh2_bn_ctx *bnctx);
 void ssh2_dh_dtor(ssh2_dh_ctx *dhctx);

src/libgcrypt.c

@@ -788,10 +788,42 @@ int ssh2_dh_key_pair(ssh2_dh_ctx *dhctx, ssh2_bn *pub, ssh2_bn *g,
     return 0;
 }
 
+int ssh2_dh_is_valid(ssh2_bn *f, ssh2_bn *p)
+{
+    gcry_mpi_t tmp;
+    unsigned int n, i, bits_set;
+
+    if(gcry_mpi_cmp_ui(f, 1) <= 0)
+        return -1;  /* f <= 1 */
+
+    tmp = gcry_mpi_new(0);
+    if(!tmp)
+        return -4;
+    gcry_mpi_sub_ui(tmp, p, 1);
+    if(gcry_mpi_cmp(f, tmp) >= 0) {
+        gcry_mpi_release(tmp);
+        return -2;  /* f >= p - 1 (== f > p - 2) */
+    }
+    gcry_mpi_release(tmp);
+
+    for(i = 0, n = gcry_mpi_get_nbits(f), bits_set = 0; i < n; ++i)
+        if(gcry_mpi_test_bit(f, i))
+            ++bits_set;
+
+    if(bits_set < 4)
+        return -3;
+
+    return 0;
+}
+
 int ssh2_dh_secret(ssh2_dh_ctx *dhctx, ssh2_bn *secret, ssh2_bn *f,
                    ssh2_bn *p, ssh2_bn_ctx *bnctx)
 {
     (void)bnctx;
+
+    if(ssh2_dh_is_valid(f, p))  /* Verify if parameters are valid */
+        return -1;
+
     /* Compute the shared secret */
     gcry_mpi_powm(secret, f, *dhctx, p);
     return 0;

src/mbedtls.c

@@ -777,10 +777,56 @@ int ssh2_dh_key_pair(ssh2_dh_ctx *dhctx, ssh2_bn *pub, ssh2_bn *g,
     return 0;
 }
 
+int ssh2_dh_is_valid(ssh2_bn *f, ssh2_bn *p)
+{
+    mbedtls_mpi one, tmp;
+    size_t n, i, bits_set;
+
+    /* Verify if valid */
+    mbedtls_mpi_init(&one);
+    if(mbedtls_mpi_lset(&one, 1)) {
+        mbedtls_mpi_free(&one);
+        return -4;
+    }
+    if(mbedtls_mpi_cmp_mpi(f, &one) != 1) {
+        mbedtls_mpi_free(&one);
+        return -1;  /* f <= 1 */
+    }
+
+    mbedtls_mpi_init(&tmp);
+    if(mbedtls_mpi_copy(&tmp, p) ||  /* tmp = p */
+       mbedtls_mpi_sub_int(&tmp, &tmp, 2)) {  /* tmp -= 2 */
+        mbedtls_mpi_free(&tmp);
+        mbedtls_mpi_free(&one);
+        return -5;
+    }
+    if(mbedtls_mpi_cmp_mpi(f, &tmp) == 1) {
+        mbedtls_mpi_free(&tmp);
+        mbedtls_mpi_free(&one);
+        return -2;  /* f >= p - 1 (== f > p - 2) */
+    }
+
+    mbedtls_mpi_free(&tmp);
+    mbedtls_mpi_free(&one);
+
+    for(i = 0, n = mbedtls_mpi_bitlen(f), bits_set = 0; i < n; ++i)
+        if(mbedtls_mpi_get_bit(f, i))
+            ++bits_set;
+
+    if(bits_set < 4)
+        return -3;
+
+    return 0;
+}
+
 int ssh2_dh_secret(ssh2_dh_ctx *dhctx, ssh2_bn *secret, ssh2_bn *f,
                    ssh2_bn *p, ssh2_bn_ctx *bnctx)
 {
     (void)bnctx;
+
+    if(ssh2_dh_is_valid(f, p))  /* Verify if parameters are valid */
+        return -1;
+
     /* Compute the shared secret */
     if(mbedtls_mpi_exp_mod(secret, f, *dhctx, p, NULL))
         return -1;

src/openssl.c

@@ -4283,9 +4283,43 @@ int ssh2_dh_key_pair(ssh2_dh_ctx *dhctx, ssh2_bn *pub, ssh2_bn *g,
     return 0;
 }
 
+int ssh2_dh_is_valid(ssh2_bn *f, ssh2_bn *p)
+{
+    BIGNUM *tmp;
+    int n, i, bits_set;
+
+    if(BN_cmp(f, BN_value_one()) != 1)
+        return -1;  /* f <= 1 */
+
+    tmp = BN_new();
+    if(!tmp)
+        return -4;
+    if(!BN_sub(tmp, p, BN_value_one())) {
+        BN_clear_free(tmp);
+        return -4;
+    }
+    if(BN_cmp(f, tmp) != -1) {
+        BN_clear_free(tmp);
+        return -2;  /* f >= p - 1 (== f > p - 2) */
+    }
+    BN_clear_free(tmp);
+
+    for(i = 0, n = BN_num_bits(f), bits_set = 0; i < n; ++i)
+        if(BN_is_bit_set(f, i))
+            ++bits_set;
+
+    if(bits_set < 4)
+        return -3;
+
+    return 0;
+}
+
 int ssh2_dh_secret(ssh2_dh_ctx *dhctx, ssh2_bn *secret, ssh2_bn *f,
                    ssh2_bn *p, ssh2_bn_ctx *bnctx)
 {
+    if(ssh2_dh_is_valid(f, p))  /* Verify if parameters are valid */
+        return -1;
+
     /* Compute the shared secret */
     if(!BN_mod_exp(secret, f, *dhctx, p, bnctx))
         return -1;

src/userauth_kbd_packet.h

@@ -34,6 +34,6 @@
 #define LIBSSH2_USERAUTH_KBD_PACKET_H
 
 int userauth_keyboard_interactive_decode_info_request(
-    LIBSSH2_SESSION *session);
+    LIBSSH2_SESSION *session); /* for unit tests */
 
 #endif /* LIBSSH2_USERAUTH_KBD_PACKET_H */

tests/test_simple.c

@@ -34,7 +34,7 @@
 #include "libssh2_priv.h"
 
 #include <stdio.h>
-#include <stdlib.h>
+#include <stdlib.h>  /* for atoi() */
 
 static int test_ssh2_base64_decode(LIBSSH2_SESSION *session)
 {
@@ -58,6 +58,72 @@ static int test_ssh2_base64_decode(LIBSSH2_SESSION *session)
     return 0;
 }
 
+static int test_ssh2_dh_is_valid(void)
+{
+    struct tbn {
+        const char *f; const char *p; int expected;
+    };
+    static const struct tbn tests[] = {
+        {  "-1",  "10", -1 },
+        {   "2",  "10", -3 },
+        {   "1",  "10", -1 },
+        {   "0",  "10", -1 },
+        {   "9",  "10", -2 }, /* f=9; p=10 => p-2=8, f>8 */
+        {   "6",  "20", -3 }, /* f=6 (110); p=20 => p-2=18, 6<=18 */
+        {  "27", "100",  0 }, /* f=27 (11011); p=100 => p-2=98, 27<=98 */
+        { "240", "242",  0 }, /* f=p-2, f=240 (11110000); p=f+2=242 */
+    };
+
+    size_t i;
+    int err = 0;
+
+    for(i = 0; i < SSH2_ARRAYSIZE(tests); i++) {
+        int got;
+#ifdef LIBSSH2_LIBGCRYPT
+        int fi = atoi(tests[i].f);
+        gcry_mpi_t f = gcry_mpi_set_ui(NULL, fi >= 0 ? fi : -fi);
+        gcry_mpi_t p = gcry_mpi_set_ui(NULL, atoi(tests[i].p));
+        if(tests[i].f[0] == '-')
+            gcry_mpi_neg(f, f);
+        got = ssh2_dh_is_valid(f, p);
+        gcry_mpi_release(f);
+        gcry_mpi_release(p);
+#elif defined(LIBSSH2_MBEDTLS)
+        mbedtls_mpi f, p;
+        mbedtls_mpi_init(&f);
+        mbedtls_mpi_init(&p);
+        if(mbedtls_mpi_read_string(&f, 10, tests[i].f) ||
+           mbedtls_mpi_read_string(&p, 10, tests[i].p))
+            got = -9;
+        else
+            got = ssh2_dh_is_valid(&f, &p);
+        mbedtls_mpi_free(&f);
+        mbedtls_mpi_free(&p);
+#elif defined(LIBSSH2_OPENSSL) || \
+    (defined(LIBSSH2_WOLFSSL) && LIBWOLFSSL_VERSION_HEX >= 0x05006000)
+        BIGNUM *f = BN_new(), *p = BN_new();
+        if(!BN_dec2bn(&f, tests[i].f) ||
+           !BN_dec2bn(&p, tests[i].p))
+            got = -9;
+        else
+            got = ssh2_dh_is_valid(f, p);
+        BN_free(f);
+        BN_free(p);
+#else
+        got = tests[i].expected;
+#endif
+        if(got != tests[i].expected) {
+            fprintf(stderr,
+                    "ssh2_dh_is_valid/%lu: f=%s p=%s: expected %d got %d\n",
+                    (unsigned long)i,
+                    tests[i].f, tests[i].p, tests[i].expected, got);
+            err++;
+        }
+    }
+
+    return err > 0;
+}
+
 int main(int argc, char *argv[])
 {
     LIBSSH2_SESSION *session;
@@ -65,7 +131,7 @@ int main(int argc, char *argv[])
     (void)argv;
     (void)argc;
 
-    rc = libssh2_init(LIBSSH2_INIT_NO_CRYPTO);
+    rc = libssh2_init(0);
     if(rc) {
         fprintf(stderr, "libssh2_init() failed: %d\n", rc);
         return 1;
@@ -78,6 +144,7 @@ int main(int argc, char *argv[])
     }
 
     rc = test_ssh2_base64_decode(session);
+    rc |= test_ssh2_dh_is_valid();
 
     libssh2_session_free(session);