Reject a negative DH e or f

Classic DH e and f are mpints (RFC 4251 section 5), so a value with
the top bit set and no leading zero is negative. KeyAgreeDh_client()
and KeyAgreeDh_server() reject one before the range check, and the key
exchange ends with the usual KEY_EXCHANGE_FAILED disconnect.

- add test_KeyAgreeDh_rejectsNegativePeer() for both sides
- pad e in test_DhGexGroup16KeyAgree() the way SendKexDhInit() does

Issue: F-15532

Author: ejohnstown

src/internal.c

@@ -8410,8 +8410,12 @@ static int KeyAgreeDh_client(WOLFSSH* ssh, byte hashId,
 
     /* Reject a peer public value outside the safe range [2, p-2] before key
      * agreement. Keeps wolfSSH independent of whether the linked wolfSSL
-     * validates the peer key inside wc_DhAgree. */
-    ret = wc_DhCheckPubKey(&ssh->handshake->privKey.dh, f, fSz);
+     * validates the peer key inside wc_DhAgree. f is an mpint, so a set
+     * top bit makes it negative (RFC 4251 section 5). */
+    if (fSz > 0 && (f[0] & 0x80) != 0)
+        ret = WS_PARSE_E;
+    else
+        ret = wc_DhCheckPubKey(&ssh->handshake->privKey.dh, f, fSz);
     if (ret != 0) {
         /* Peer-driven reject; log at DEBUG like other peer-input rejections
          * so a remote peer cannot flood the error log. */
@@ -17352,10 +17356,24 @@ static int KeyAgreeDh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz)
             PRIVATE_KEY_LOCK();
         }
         /* Reject a peer public value outside the safe range [2, p-2] before
-         * key agreement, independent of the linked wolfSSL's own check. */
-        if (ret == 0)
-            ret = wc_DhCheckPubKey(privKey, ssh->handshake->e,
-                    ssh->handshake->eSz);
+         * key agreement, independent of the linked wolfSSL's own check.
+         * e is an mpint, so a set top bit makes it negative (RFC 4251
+         * section 5). */
+        if (ret == 0) {
+            if (ssh->handshake->eSz > 0
+                    && (ssh->handshake->e[0] & 0x80) != 0)
+                ret = WS_PARSE_E;
+            else
+                ret = wc_DhCheckPubKey(privKey, ssh->handshake->e,
+                        ssh->handshake->eSz);
+            if (ret != 0) {
+                /* Peer-driven reject; log at DEBUG like other peer-input
+                 * rejections so a remote peer cannot flood the error log. */
+                WLOG(WS_LOG_DEBUG,
+                        "Peer DH public key out of range, %d", ret);
+                ret = WS_CRYPTO_FAILED;
+            }
+        }
         if (ret == 0) {
             PRIVATE_KEY_UNLOCK();
             ret = wc_DhAgree(privKey, ssh->k, &ssh->kSz, y_ptr, ySz,

tests/unit.c

@@ -3540,9 +3540,13 @@ static int test_DhGexGroup16KeyAgree(void)
         goto out;
     }
 
-    /* Hand the client's public value to the server and run the agreement. */
-    WMEMCPY(ssh->handshake->e, cPub, cPubSz);
-    ssh->handshake->eSz = cPubSz;
+    /* Hand the client's public value to the server as the mpint
+     * SendKexDhInit() would send, and run the agreement. */
+    ssh->handshake->eSz = 0;
+    if (cPub[0] & 0x80)
+        ssh->handshake->e[ssh->handshake->eSz++] = 0;
+    WMEMCPY(ssh->handshake->e + ssh->handshake->eSz, cPub, cPubSz);
+    ssh->handshake->eSz += cPubSz;
     ssh->kSz = MAX_KEX_KEY_SZ;
 
     ret = wolfSSH_TestKeyAgreeDh_server(ssh, WC_HASH_TYPE_SHA256, f, &fSz);
@@ -20678,7 +20682,7 @@ static int test_KeyAgreeDh_server_rejectsOutOfRangePeer(void)
         hs->eSz = 1;
         fSz = (word32)sizeof(f);
         ret = wolfSSH_TestKeyAgreeDh_server(ssh, WC_HASH_TYPE_SHA256, f, &fSz);
-        if (ret == WS_SUCCESS) {
+        if (ret != WS_CRYPTO_FAILED) {
             result = -743;
             break;
         }
@@ -20750,6 +20754,111 @@ static int test_KeyAgreeDh_client_rejectsOutOfRangePeer(void)
     return result;
 }
 #endif /* HAVE_FFDHE_2048 */
+
+/* Verify both DH key agreement sides reject a peer public value encoded
+ * as a negative mpint (RFC 4251 section 5). 0x80 is 128 read unsigned,
+ * inside [2, p-2], so only the sign check can reject it; 0x00 0x80 is
+ * the same value encoded correctly and must be accepted. */
+static int test_KeyAgreeDh_rejectsNegativePeer(void)
+{
+    WOLFSSH_CTX* ctx = NULL;
+    WOLFSSH* ssh = NULL;
+    HandshakeInfo* hs = NULL;
+    byte f[MAX_KEX_KEY_SZ];
+    word32 fSz;
+    int ret;
+    int result = 0;
+    static const byte negPeer[1] = { 0x80 };
+    static const byte posPeer[2] = { 0x00, 0x80 };
+    byte kexId;
+
+    /* server: the peer value is the client's e */
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
+    if (ctx == NULL)
+        return -760;
+    ssh = wolfSSH_new(ctx);
+    if (ssh == NULL || ssh->handshake == NULL) {
+        result = -761;
+        goto cleanup;
+    }
+#ifndef WOLFSSH_NO_DH_GROUP14_SHA256
+    kexId = ID_DH_GROUP14_SHA256;
+#elif !defined(WOLFSSH_NO_DH_GROUP16_SHA512)
+    kexId = ID_DH_GROUP16_SHA512;
+#elif !defined(WOLFSSH_NO_DH_GROUP14_SHA1)
+    kexId = ID_DH_GROUP14_SHA1;
+#elif !defined(WOLFSSH_NO_DH_GROUP1_SHA1)
+    kexId = ID_DH_GROUP1_SHA1;
+#else
+    /* No fixed DH group enabled; fail rather than silently pass. */
+    result = -762;
+    goto cleanup;
+#endif
+    hs = ssh->handshake;
+    hs->kexId = kexId;
+
+    WMEMCPY(hs->e, negPeer, sizeof(negPeer));
+    hs->eSz = (word32)sizeof(negPeer);
+    fSz = (word32)sizeof(f);
+    ret = wolfSSH_TestKeyAgreeDh_server(ssh, WC_HASH_TYPE_SHA256, f, &fSz);
+    if (ret != WS_CRYPTO_FAILED) {
+        result = -763;
+        goto cleanup;
+    }
+
+    WMEMCPY(hs->e, posPeer, sizeof(posPeer));
+    hs->eSz = (word32)sizeof(posPeer);
+    fSz = (word32)sizeof(f);
+    ret = wolfSSH_TestKeyAgreeDh_server(ssh, WC_HASH_TYPE_SHA256, f, &fSz);
+    if (ret != WS_SUCCESS) {
+        result = -764;
+        goto cleanup;
+    }
+
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+
+    /* client: the peer value is the server's f */
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    if (ctx == NULL)
+        return -765;
+    ssh = wolfSSH_new(ctx);
+    if (ssh == NULL || ssh->handshake == NULL) {
+        result = -766;
+        goto cleanup;
+    }
+    ssh->handshake->kexId = kexId;
+
+    /* The hook frees the DH key on return, so set it up for each call. */
+    if (wolfSSH_TestSetDhKexKey(ssh) != 0) {
+        result = -767;
+        goto cleanup;
+    }
+    ret = wolfSSH_TestKeyAgreeDh_client(ssh, WC_HASH_TYPE_SHA256,
+            negPeer, (word32)sizeof(negPeer));
+    if (ret != WS_CRYPTO_FAILED) {
+        result = -768;
+        goto cleanup;
+    }
+
+    if (wolfSSH_TestSetDhKexKey(ssh) != 0) {
+        result = -769;
+        goto cleanup;
+    }
+    ret = wolfSSH_TestKeyAgreeDh_client(ssh, WC_HASH_TYPE_SHA256,
+            posPeer, (word32)sizeof(posPeer));
+    if (ret != WS_SUCCESS) {
+        result = -770;
+        goto cleanup;
+    }
+
+cleanup:
+    if (ssh != NULL)
+        wolfSSH_free(ssh);
+    if (ctx != NULL)
+        wolfSSH_CTX_free(ctx);
+    return result;
+}
 #endif /* !WOLFSSH_NO_DH */
 
 #if !defined(WOLFSSH_NO_ECDH) && !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256)
@@ -26512,6 +26621,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
             (unitResult == 0 ? "SUCCESS" : "FAILED"));
     testResult = testResult || unitResult;
 #endif
+
+    unitResult = test_KeyAgreeDh_rejectsNegativePeer();
+    printf("KeyAgreeDh_rejectsNegativePeer: %s (result=%d)\n",
+            (unitResult == 0 ? "SUCCESS" : "FAILED"), unitResult);
+    testResult = testResult || unitResult;
 #endif /* !WOLFSSH_NO_DH */
 
 #if !defined(WOLFSSH_NO_ECDH) && !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256)