From ca3ec7ec889b015ce1120cfafc7eaff9d23713e1 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 23 Jun 2026 15:36:35 -0700 Subject: [PATCH 1/4] Discard guessed KEX packet in DoKexDhGexGroup - DoKexDhGexGroup didn't check ignoreNextKexMsg, so a wrong first_kex_packet_follows guess was parsed as a real group and errored instead of being silently discarded (RFC 4253 7.1). - Factor the discard the Do* KEX handlers share into SkipGuessedKexMsg and route DoKexDhInit / DoKexDhReply / DoKexDhGexRequest / DoKexDhGexGroup through it, so GEX_GROUP gets the same guard. - The helper also re-arms handshake->expectMsgId so the real message following a discarded guess is still accepted. - Extend TestFirstPacketFollowsSkipped via a new wolfSSH_TestDoKexDhGexGroup wrapper. Issue: F-2866 --- src/internal.c | 61 +++++++++++++++++++++++++++++----------------- tests/regress.c | 48 ++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 2 ++ 3 files changed, 89 insertions(+), 22 deletions(-) diff --git a/src/internal.c b/src/internal.c index 47d40a1eb..81bdb36a3 100644 --- a/src/internal.c +++ b/src/internal.c @@ -5533,6 +5533,23 @@ static const byte dhPrimeGroup16[] = { static const word32 dhPrimeGroup16Sz = (word32)sizeof(dhPrimeGroup16); #endif +/* RFC 4253 sec. 7.1: discard the KEX packet a peer sent after a wrong + * first_packet_follows guess. Leave expectMsgId at MSGID_NONE -- the real + * follow-up's ID comes from the negotiated KEX, not the guess, and may cross + * the GEX boundary. Returns 1 if consumed, else 0. Caller validates handshake. */ +static int SkipGuessedKexMsg(WOLFSSH* ssh, const char* what, + word32 len, word32* idx) +{ + WOLFSSH_UNUSED(what); + if (!ssh->handshake->ignoreNextKexMsg) + return 0; + WLOG(WS_LOG_DEBUG, "Skipping %s due to first_packet_follows guess mismatch.", + what); + ssh->handshake->ignoreNextKexMsg = 0; + *idx += len; + return 1; +} + static int DoKexDhInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { /* First get the length of the MP_INT, and then add in the hash of the @@ -5554,14 +5571,9 @@ static int DoKexDhInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) ret = WS_BAD_ARGUMENT; if (ret == WS_SUCCESS) { - if (ssh->handshake->ignoreNextKexMsg) { - /* skip this message. */ - WLOG(WS_LOG_DEBUG, "Skipping client's KEXDH_INIT message due to " - "first_packet_follows guess mismatch."); - ssh->handshake->ignoreNextKexMsg = 0; - *idx += len; + if (SkipGuessedKexMsg(ssh, "client's KEXDH_INIT message", + len, idx)) return WS_SUCCESS; - } begin = *idx; ret = GetStringRef(&eSz, &e, buf, len, &begin); @@ -6781,14 +6793,9 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) } if (ret == WS_SUCCESS) { - if (ssh->handshake->ignoreNextKexMsg) { - /* skip this message. */ - WLOG(WS_LOG_DEBUG, "Skipping server's KEXDH_REPLY message due to " - "first_packet_follows guess mismatch."); - ssh->handshake->ignoreNextKexMsg = 0; - *idx += len; + if (SkipGuessedKexMsg(ssh, "server's KEXDH_REPLY message", + len, idx)) return WS_SUCCESS; - } } if (ret == WS_SUCCESS && len < LENGTH_SZ*2 + *idx) { @@ -7232,14 +7239,9 @@ static int DoKexDhGexRequest(WOLFSSH* ssh, ret = WS_BAD_ARGUMENT; if (ret == WS_SUCCESS) { - if (ssh->handshake->ignoreNextKexMsg) { - /* skip this message. */ - WLOG(WS_LOG_DEBUG, "Skipping client's KEXDH_GEX_REQUEST message " - "due to first_packet_follows guess mismatch."); - ssh->handshake->ignoreNextKexMsg = 0; - *idx += len; + if (SkipGuessedKexMsg(ssh, "client's KEXDH_GEX_REQUEST message", + len, idx)) return WS_SUCCESS; - } begin = *idx; ret = GetUint32(&ssh->handshake->dhGexMinSz, buf, len, &begin); @@ -7411,10 +7413,19 @@ static int DoKexDhGexGroup(WOLFSSH* ssh, word32 begin; int ret = WS_SUCCESS; - if (ssh == NULL || buf == NULL || len == 0 || idx == NULL) + if (ssh == NULL || ssh->handshake == NULL || buf == NULL || len == 0 || + idx == NULL) ret = WS_BAD_ARGUMENT; if (ret == WS_SUCCESS) { + /* A conformant server sends GROUP only in response to the client's + * REQUEST, so it should never set first_packet_follows here. Discard + * the message defensively if a peer sets it anyway, mirroring the other + * Do* handlers. */ + if (SkipGuessedKexMsg(ssh, "server's KEXDH_GEX_GROUP message", + len, idx)) + return WS_SUCCESS; + begin = *idx; ret = GetMpint(&primeGroupSz, &primeGroup, buf, len, &begin); if (ret == WS_SUCCESS && primeGroupSz > (MAX_KEX_KEY_SZ + 1)) { @@ -20097,6 +20108,12 @@ int wolfSSH_TestDoKexDhGexRequest(WOLFSSH* ssh, byte* buf, word32 len, return DoKexDhGexRequest(ssh, buf, len, idx); } +int wolfSSH_TestDoKexDhGexGroup(WOLFSSH* ssh, byte* buf, word32 len, + word32* idx) +{ + return DoKexDhGexGroup(ssh, buf, len, idx); +} + int wolfSSH_TestValidateKexDhGexGroup(const byte* primeGroup, word32 primeGroupSz, const byte* generator, word32 generatorSz, word32 minBits, word32 maxBits, WC_RNG* rng) diff --git a/tests/regress.c b/tests/regress.c index 18cbb2cea..48fd83fd1 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3203,6 +3203,47 @@ static void RunFirstPacketFollowsSkipCase(FirstPacketFollowsSkipFn fn, wolfSSH_CTX_free(ctx); } +#ifndef WOLFSSH_NO_DH_GEX_SHA256 +/* After skipping a wrong-guess packet, the real first packet of the negotiated + * KEX must still pass the message gate even when it sits across the GEX + * boundary from the guess. Drives skip-then-real through IsMessageAllowed and + * checks the skip did not pin expectMsgId to the discarded message's ID. */ +static void RunFirstPacketFollowsCrossBoundaryCase(FirstPacketFollowsSkipFn fn, + const char* label, byte realMsg) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + byte payload[8]; + word32 idx = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertNotNull(ssh->handshake); + + ssh->clientState = CLIENT_KEXINIT_DONE; + ssh->isKeying |= WOLFSSH_PEER_IS_KEYING; + ssh->handshake->ignoreNextKexMsg = 1; + ssh->handshake->expectMsgId = MSGID_NONE; + + WMEMSET(payload, 0xAB, sizeof(payload)); + + ret = fn(ssh, payload, sizeof(payload), &idx); + if (ret != WS_SUCCESS) + Fail(("%s returns WS_SUCCESS when skipping", label), ("%d", ret)); + + /* Gate must not be pinned to the discarded guess's ID. */ + AssertIntEQ(ssh->handshake->expectMsgId, MSGID_NONE); + /* The negotiated first-KEX packet must be accepted. */ + AssertIntEQ(wolfSSH_TestIsMessageAllowed(ssh, realMsg, 0), 1); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} +#endif /* WOLFSSH_NO_DH_GEX_SHA256 */ + static void TestFirstPacketFollowsSkipped(void) { RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhInit, @@ -3210,6 +3251,13 @@ static void TestFirstPacketFollowsSkipped(void) #ifndef WOLFSSH_NO_DH_GEX_SHA256 RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhGexRequest, "DoKexDhGexRequest", WOLFSSH_ENDPOINT_SERVER, CLIENT_KEXINIT_DONE); + RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhGexGroup, + "DoKexDhGexGroup", WOLFSSH_ENDPOINT_CLIENT, SERVER_KEXINIT_DONE); + /* Guess/negotiation straddling the GEX boundary, both directions. */ + RunFirstPacketFollowsCrossBoundaryCase(wolfSSH_TestDoKexDhInit, + "DoKexDhInit->GEX_REQUEST", MSGID_KEXDH_GEX_REQUEST); + RunFirstPacketFollowsCrossBoundaryCase(wolfSSH_TestDoKexDhGexRequest, + "DoKexDhGexRequest->KEXDH_INIT", MSGID_KEXDH_INIT); #endif RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhReply, "DoKexDhReply", WOLFSSH_ENDPOINT_CLIENT, SERVER_KEXINIT_DONE); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index dba5dbf32..a95098648 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1499,6 +1499,8 @@ enum WS_MessageIdLimits { #ifndef WOLFSSH_NO_DH_GEX_SHA256 WOLFSSH_API int wolfSSH_TestDoKexDhGexRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); + WOLFSSH_API int wolfSSH_TestDoKexDhGexGroup(WOLFSSH* ssh, byte* buf, + word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestValidateKexDhGexGroup(const byte* primeGroup, word32 primeGroupSz, const byte* generator, word32 generatorSz, word32 minBits, word32 maxBits, WC_RNG* rng); From d06f4797d517476e93ef7da4bd8869a858623a2f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 23 Jun 2026 15:43:33 -0700 Subject: [PATCH 2/4] Select DH GEX group from client size window - Server GEX ignored the client's min/preferred/max and always sent group 14, silently downgrading a 4096-min client (RFC 4419). - Add SelectKexDhGexGroup(): pick the built-in group within [min, max] closest to preferred, ties favoring the smaller. - Reject with WS_DH_SIZE_E when no built-in group fits. - GetDHPrimeGroup() now takes the WOLFSSH so the group sent, the exchange hash, and the shared secret all reselect consistently. - Add a unit test (default, max-cap, out-of-window, 4096-min, 1024-only). Issue: F-55 --- src/internal.c | 143 +++++++++++++-- tests/unit.c | 432 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 6 + 3 files changed, 570 insertions(+), 11 deletions(-) diff --git a/src/internal.c b/src/internal.c index 81bdb36a3..80682b25a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -12778,11 +12778,77 @@ static int BuildRFC6187Info(WOLFSSH* ssh, int pubKeyID, #endif /* WOLFSSH_CERTS */ +#ifndef WOLFSSH_NO_DH_GEX_SHA256 +/* Pick the in-window group closest to preferredBits, ties favoring the + * smaller. WS_DH_SIZE_E if none fits. */ +static int SelectKexDhGexGroup(word32 minBits, word32 preferredBits, + word32 maxBits, const byte** primeGroup, word32* primeGroupSz) +{ + static const struct { + word32 bits; + const byte* group; + const word32* groupSz; + } candidates[] = { + #ifndef WOLFSSH_NO_DH_GROUP1_SHA1 + { 1024, dhPrimeGroup1, &dhPrimeGroup1Sz }, + #endif + { 2048, dhPrimeGroup14, &dhPrimeGroup14Sz }, + #ifndef WOLFSSH_NO_DH_GROUP16_SHA512 + { 4096, dhPrimeGroup16, &dhPrimeGroup16Sz }, + #endif + }; + word32 i; + word32 best = 0; + word32 bestDelta = 0; + int haveBest = 0; + int ret = WS_SUCCESS; + + if (primeGroup == NULL || primeGroupSz == NULL) + return WS_BAD_ARGUMENT; + + for (i = 0; i < (word32)(sizeof(candidates) / sizeof(candidates[0])); i++) { + word32 bits = candidates[i].bits; + word32 delta; + + if (bits < minBits || bits > maxBits) + continue; + delta = (bits > preferredBits) ? (bits - preferredBits) + : (preferredBits - bits); + /* Ascending scan with a strict '<' keeps the smaller group on a tie. */ + if (!haveBest || delta < bestDelta) { + best = i; + bestDelta = delta; + haveBest = 1; + } + } + + if (!haveBest) { + WLOG(WS_LOG_DEBUG, + "DH GEX: no built-in group within client window [%u, %u]", + minBits, maxBits); + ret = WS_DH_SIZE_E; + } + else { + *primeGroup = candidates[best].group; + *primeGroupSz = *candidates[best].groupSz; + } + + return ret; +} +#endif /* !WOLFSSH_NO_DH_GEX_SHA256 */ + + #ifndef WOLFSSH_NO_DH -static int GetDHPrimeGroup(int kexId, const byte** primeGroup, +static int GetDHPrimeGroup(WOLFSSH* ssh, const byte** primeGroup, word32* primeGroupSz, const byte** generator, word32* generatorSz) { int ret = WS_SUCCESS; + int kexId; + + if (ssh == NULL || ssh->handshake == NULL) + return WS_BAD_ARGUMENT; + + kexId = ssh->handshake->kexId; switch (kexId) { #ifndef WOLFSSH_NO_DH_GROUP1_SHA1 @@ -12819,10 +12885,27 @@ static int GetDHPrimeGroup(int kexId, const byte** primeGroup, #endif #ifndef WOLFSSH_NO_DH_GEX_SHA256 case ID_DH_GEX_SHA256: - *primeGroup = dhPrimeGroup14; - *primeGroupSz = dhPrimeGroup14Sz; - *generator = dhGenerator; - *generatorSz = dhGeneratorSz; + /* Reuse the group SendKexDhGexGroup cached on the handshake so the + * exchange hash and the shared secret match the group that was put + * on the wire. Fall back to selecting from the client's window if + * the cache is somehow unset, so this path can never desynchronize + * from the wire group. */ + if (ssh->handshake->primeGroup != NULL) { + *primeGroup = ssh->handshake->primeGroup; + *primeGroupSz = ssh->handshake->primeGroupSz; + *generator = dhGenerator; + *generatorSz = dhGeneratorSz; + } + else { + ret = SelectKexDhGexGroup(ssh->handshake->dhGexMinSz, + ssh->handshake->dhGexPreferredSz, + ssh->handshake->dhGexMaxSz, + primeGroup, primeGroupSz); + if (ret == WS_SUCCESS) { + *generator = dhGenerator; + *generatorSz = dhGeneratorSz; + } + } break; #endif default: @@ -13236,7 +13319,7 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, if (ssh->handshake->kexId == ID_DH_GEX_SHA256) { byte primeGroupPad = 0, generatorPad = 0; - if (GetDHPrimeGroup(ssh->handshake->kexId, &primeGroup, + if (GetDHPrimeGroup(ssh, &primeGroup, &primeGroupSz, &generator, &generatorSz) != WS_SUCCESS) { ret = WS_BAD_ARGUMENT; } @@ -13476,7 +13559,7 @@ static int KeyAgreeDh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz) ret = WS_MEMORY_E; #else DhKey privKey[1]; - byte y_s[MAX_KEX_KEY_SZ]; + byte y_s[MAX_KEX_KEY_SZ] = {0}; y_ptr = y_s; #endif @@ -13484,7 +13567,7 @@ static int KeyAgreeDh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz) WOLFSSH_UNUSED(hashId); if (ret == WS_SUCCESS) { - ret = GetDHPrimeGroup(ssh->handshake->kexId, &primeGroup, + ret = GetDHPrimeGroup(ssh, &primeGroup, &primeGroupSz, &generator, &generatorSz); if (ret == WS_SUCCESS) { @@ -14988,8 +15071,8 @@ int SendKexDhGexGroup(WOLFSSH* ssh) byte* output; word32 idx = 0; word32 payloadSz; - const byte* primeGroup = dhPrimeGroup14; - word32 primeGroupSz = dhPrimeGroup14Sz; + const byte* primeGroup = NULL; + word32 primeGroupSz = 0; const byte* generator = dhGenerator; word32 generatorSz = dhGeneratorSz; byte primePad = 0; @@ -15000,6 +15083,30 @@ int SendKexDhGexGroup(WOLFSSH* ssh) if (ssh == NULL || ssh->handshake == NULL) ret = WS_BAD_ARGUMENT; + /* Pick a group that satisfies the client's requested min/preferred/max + * rather than always sending the 2048-bit group 14. */ + if (ret == WS_SUCCESS) { + ret = SelectKexDhGexGroup(ssh->handshake->dhGexMinSz, + ssh->handshake->dhGexPreferredSz, + ssh->handshake->dhGexMaxSz, + &primeGroup, &primeGroupSz); + } + + /* Cache the selected group so the exchange hash and shared secret + * (GetDHPrimeGroup) reuse exactly what goes on the wire here. */ + if (ret == WS_SUCCESS) { + if (ssh->handshake->primeGroup != NULL) + WFREE(ssh->handshake->primeGroup, ssh->ctx->heap, DYNTYPE_MPINT); + ssh->handshake->primeGroup = + (byte*)WMALLOC(primeGroupSz, ssh->ctx->heap, DYNTYPE_MPINT); + if (ssh->handshake->primeGroup == NULL) + ret = WS_MEMORY_E; + else { + WMEMCPY(ssh->handshake->primeGroup, primeGroup, primeGroupSz); + ssh->handshake->primeGroupSz = primeGroupSz; + } + } + if (ret == WS_SUCCESS) { if (primeGroup[0] & 0x80) primePad = 1; @@ -20054,7 +20161,7 @@ int wolfSSH_TestSetDhKexKey(WOLFSSH* ssh) word32 eSz = (word32)sizeof(e); int keyInited = 0; - ret = GetDHPrimeGroup(ssh->handshake->kexId, &primeGroup, &primeGroupSz, + ret = GetDHPrimeGroup(ssh, &primeGroup, &primeGroupSz, &generator, &generatorSz); if (ret == WS_SUCCESS) { ret = wc_InitDhKey(privKey); @@ -20084,6 +20191,13 @@ int wolfSSH_TestSetDhKexKey(WOLFSSH* ssh) return ret; } +int wolfSSH_TestGetDHPrimeGroup(WOLFSSH* ssh, const byte** primeGroup, + word32* primeGroupSz, const byte** generator, word32* generatorSz) +{ + return GetDHPrimeGroup(ssh, primeGroup, primeGroupSz, + generator, generatorSz); +} + #endif /* !WOLFSSH_NO_DH */ #ifndef WOLFSSH_NO_ECDH @@ -20122,6 +20236,13 @@ int wolfSSH_TestValidateKexDhGexGroup(const byte* primeGroup, generator, generatorSz, minBits, maxBits, rng); } +int wolfSSH_TestSelectKexDhGexGroup(word32 minBits, word32 preferredBits, + word32 maxBits, const byte** primeGroup, word32* primeGroupSz) +{ + return SelectKexDhGexGroup(minBits, preferredBits, maxBits, + primeGroup, primeGroupSz); +} + #endif /* !WOLFSSH_NO_DH_GEX_SHA256 */ #ifndef WOLFSSH_NO_RSA diff --git a/tests/unit.c b/tests/unit.c index 6be8faed1..f7d324a68 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -1269,6 +1269,418 @@ static int test_DhGexGroupValidate(void) return result; } +/* Server-side group selection for DH GEX. Confirms the server honors the + * client's requested min/preferred/max window instead of always handing back + * the 2048-bit group 14, and rejects windows no built-in group can satisfy. */ +static int test_DhGexGroupSelect(void) +{ + const byte* group; + word32 groupSz; + int ret; + + /* Default-ish window: with group 16 enabled, preferred 3072 is equidistant + * from 2048 and 4096 and the tie favors the smaller group; without group 16, + * 2048 is simply the closest in-window candidate. Either way a stock client + * still receives the historical 2048-bit group (256 bytes). */ + group = NULL; groupSz = 0; + ret = wolfSSH_TestSelectKexDhGexGroup(1024, 3072, 8192, &group, &groupSz); + if (ret != WS_SUCCESS || groupSz != 256) { + printf("DhGexGroupSelect: default window got ret %d sz %u\n", + ret, groupSz); + return -200; + } + + /* A max below 4096 must cap the choice even when preferred is huge. */ + group = NULL; groupSz = 0; + ret = wolfSSH_TestSelectKexDhGexGroup(1024, 8192, 2048, &group, &groupSz); + if (ret != WS_SUCCESS || groupSz != 256) { + printf("DhGexGroupSelect: max-cap window got ret %d sz %u\n", + ret, groupSz); + return -201; + } + + /* A window no built-in group falls inside must be rejected, not silently + * served a group outside it. */ + group = NULL; groupSz = 0; + ret = wolfSSH_TestSelectKexDhGexGroup(3000, 3000, 3500, &group, &groupSz); + if (ret != WS_DH_SIZE_E) { + printf("DhGexGroupSelect: impossible window got ret %d\n", ret); + return -202; + } + +#ifndef WOLFSSH_NO_DH_GROUP16_SHA512 + /* A client demanding a 4096-bit minimum gets the 4096-bit group (512 + * bytes), never a silent downgrade to 2048. */ + group = NULL; groupSz = 0; + ret = wolfSSH_TestSelectKexDhGexGroup(4096, 4096, 8192, &group, &groupSz); + if (ret != WS_SUCCESS || groupSz != 512) { + printf("DhGexGroupSelect: 4096 min got ret %d sz %u\n", ret, groupSz); + return -203; + } +#else + /* Without group 16 there is nothing >= 4096, so the request is rejected + * rather than downgraded. */ + group = NULL; groupSz = 0; + ret = wolfSSH_TestSelectKexDhGexGroup(4096, 4096, 8192, &group, &groupSz); + if (ret != WS_DH_SIZE_E) { + printf("DhGexGroupSelect: 4096 min (no group16) got ret %d\n", ret); + return -203; + } +#endif + +#ifndef WOLFSSH_NO_DH_GROUP1_SHA1 + /* A window only the 1024-bit group fits returns group 1 (128 bytes). */ + group = NULL; groupSz = 0; + ret = wolfSSH_TestSelectKexDhGexGroup(1024, 1024, 1536, &group, &groupSz); + if (ret != WS_SUCCESS || groupSz != 128) { + printf("DhGexGroupSelect: 1024 window got ret %d sz %u\n", + ret, groupSz); + return -204; + } +#endif + + return 0; +} + +/* Send sink so SendKexDhGexGroup can run the real server path (select the + * group, build the KEXDH_GEX_GROUP packet, drain it) without a live transport. + * We only care that the group it selects gets cached on the handshake. */ +static int GexSinkSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) +{ + (void)ssh; + (void)buf; + (void)ctx; + return (int)sz; +} + +/* Write a big-endian uint32 without the internal-only c32toa(). */ +static void PutU32BE(byte* out, word32 v) +{ + out[0] = (byte)(v >> 24); + out[1] = (byte)(v >> 16); + out[2] = (byte)(v >> 8); + out[3] = (byte)(v); +} + +/* One send/hash consistency case. Runs the real server request path for the + * given client window, then confirms GetDHPrimeGroup hands the exchange-hash + * and key-agreement path the exact group SendKexDhGexGroup cached and put on + * the wire. A zero expectSz skips the size assertion. Failure codes are + * offset from base so each caller reports distinctly. */ +static int DhGexSendHashConsistencyCase(word32 minBits, word32 prefBits, + word32 maxBits, word32 expectSz, int base) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte request[UINT32_SZ * 3]; + word32 idx = 0; + const byte* hashGroup = NULL; + word32 hashGroupSz = 0; + const byte* generator = NULL; + word32 generatorSz = 0; + int ret; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return base; + wolfSSH_SetIOSend(ctx, GexSinkSend); + ssh = wolfSSH_new(ctx); + if (ssh == NULL || ssh->handshake == NULL) { + result = base - 1; + goto out; + } + ssh->handshake->kexId = ID_DH_GEX_SHA256; + + PutU32BE(request, minBits); + PutU32BE(request + UINT32_SZ, prefBits); + PutU32BE(request + UINT32_SZ * 2, maxBits); + + /* Real server entry point: parse the window, select the group, send it on + * the wire, and cache it on the handshake. */ + ret = wolfSSH_TestDoKexDhGexRequest(ssh, request, sizeof(request), &idx); + if (ret != WS_SUCCESS) { + printf("DhGexSendHashConsistency: request ret %d\n", ret); + result = base - 2; + goto out; + } + if (ssh->handshake->primeGroup == NULL || + (expectSz != 0 && ssh->handshake->primeGroupSz != expectSz)) { + printf("DhGexSendHashConsistency: wire group sz %u want %u\n", + ssh->handshake->primeGroupSz, expectSz); + result = base - 3; + goto out; + } + + /* The exchange-hash / key-agreement path must hand back the identical group + * -- same pointer, same size -- not an independently re-selected one. */ + ret = wolfSSH_TestGetDHPrimeGroup(ssh, &hashGroup, &hashGroupSz, + &generator, &generatorSz); + if (ret != WS_SUCCESS) { + printf("DhGexSendHashConsistency: hash group ret %d\n", ret); + result = base - 4; + goto out; + } + if (hashGroup != ssh->handshake->primeGroup || + hashGroupSz != ssh->handshake->primeGroupSz) { + printf("DhGexSendHashConsistency: hash group sz %u != wire %u\n", + hashGroupSz, ssh->handshake->primeGroupSz); + result = base - 5; + goto out; + } + +out: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + +/* End-to-end consistency check for DH GEX server group selection: the group + * SendKexDhGexGroup puts on the wire must be the exact group GetDHPrimeGroup + * hands the exchange hash. */ +static int test_DhGexGroupSendHashConsistency(void) +{ + int ret; + + /* Default client window. Covers the cache plumbing on every build, + * including group14-only ones. */ + ret = DhGexSendHashConsistencyCase(WOLFSSH_DEFAULT_GEXDH_MIN, + WOLFSSH_DEFAULT_GEXDH_PREFERRED, WOLFSSH_DEFAULT_GEXDH_MAX, + 0, -210); + if (ret != 0) + return ret; + +#ifndef WOLFSSH_NO_DH_GROUP16_SHA512 + /* Force the 4096-bit group 16 (512 bytes) so a send/hash divergence can't + * hide behind both paths independently landing on the default group 14. */ + ret = DhGexSendHashConsistencyCase(4096, 4096, 8192, 512, -230); + if (ret != 0) + return ret; +#endif + + return 0; +} + +/* Drive a successful client-side GEX GROUP accept and confirm the client honors + * a peer's non-default generator (g != 2). DoKexDhGexGroup caches the wire prime + * and generator on the handshake, then GetDHPrimeGroup must hand both back to + * key agreement -- the generator-honoring branch that only the client reaches. + * The server-side tests always leave handshake->generator NULL, so without this + * that branch has zero positive coverage. Mirrors how test_DhGexGroup16KeyAgree + * sources a real safe prime, but exercises the accept side instead of select. */ +static int test_DhGexGroupAcceptHonorsGenerator(void) +{ +#if WOLFSSH_DH_GEX_MIN_BITS <= 2048 + WOLFSSH_CTX* sctx = NULL; /* server: source a genuine 2048-bit safe prime */ + WOLFSSH* sssh = NULL; + WOLFSSH_CTX* cctx = NULL; /* client: drive the accept under test */ + WOLFSSH* cssh = NULL; + byte request[UINT32_SZ * 3]; + const byte* srcGroup = NULL; + word32 srcGroupSz = 0; + const byte* srcGen = NULL; + word32 srcGenSz = 0; + byte wire[UINT32_SZ + 1 + 512 + UINT32_SZ + 1]; + word32 wIdx = 0; + word32 idx = 0; + const byte* group = NULL; + word32 groupSz = 0; + const byte* generator = NULL; + word32 generatorSz = 0; + byte pad; + int ret; + int result = 0; + + /* Select group 14 on a server so we have a real 2048-bit safe prime to put + * on the wire; a window pinned to 2048 leaves group 14 the only fit. */ + sctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (sctx == NULL) + return -300; + wolfSSH_SetIOSend(sctx, GexSinkSend); + sssh = wolfSSH_new(sctx); + if (sssh == NULL || sssh->handshake == NULL) { + result = -301; + goto out; + } + sssh->handshake->kexId = ID_DH_GEX_SHA256; + PutU32BE(request, 2048); + PutU32BE(request + UINT32_SZ, 2048); + PutU32BE(request + UINT32_SZ * 2, 2048); + ret = wolfSSH_TestDoKexDhGexRequest(sssh, request, sizeof(request), &idx); + if (ret != WS_SUCCESS) { + printf("DhGexGroupAcceptHonorsGenerator: request ret %d\n", ret); + result = -302; + goto out; + } + ret = wolfSSH_TestGetDHPrimeGroup(sssh, &srcGroup, &srcGroupSz, + &srcGen, &srcGenSz); + if (ret != WS_SUCCESS || srcGroup == NULL || srcGroupSz == 0) { + result = -303; + goto out; + } + + /* Build a GROUP message: mpint prime (0x00 sign pad when the top bit is + * set), then mpint generator 5 -- deliberately not the built-in 2, so a + * passing accept can only come from caching and honoring the wire value. */ + pad = (srcGroup[0] & 0x80) ? 1 : 0; + if ((word32)(UINT32_SZ + pad + srcGroupSz + UINT32_SZ + 1) > sizeof(wire)) { + result = -304; + goto out; + } + PutU32BE(wire + wIdx, srcGroupSz + pad); + wIdx += UINT32_SZ; + if (pad) + wire[wIdx++] = 0x00; + WMEMCPY(wire + wIdx, srcGroup, srcGroupSz); + wIdx += srcGroupSz; + PutU32BE(wire + wIdx, 1); + wIdx += UINT32_SZ; + wire[wIdx++] = 0x05; + + cctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (cctx == NULL) { + result = -305; + goto out; + } + wolfSSH_SetIOSend(cctx, GexSinkSend); + cssh = wolfSSH_new(cctx); + if (cssh == NULL || cssh->handshake == NULL) { + result = -306; + goto out; + } + cssh->handshake->kexId = ID_DH_GEX_SHA256; + cssh->handshake->dhGexMinSz = 2048; + cssh->handshake->dhGexMaxSz = 8192; + + idx = 0; + ret = wolfSSH_TestDoKexDhGexGroup(cssh, wire, wIdx, &idx); + if (ret != WS_SUCCESS) { + printf("DhGexGroupAcceptHonorsGenerator: accept ret %d\n", ret); + result = -307; + goto out; + } + /* The accept must have cached the wire generator, not defaulted to g = 2. */ + if (cssh->handshake->generator == NULL || + cssh->handshake->generatorSz != 1 || + cssh->handshake->generator[0] != 0x05) { + result = -308; + goto out; + } + + /* GetDHPrimeGroup must return the cached prime AND the cached non-default + * generator -- same pointers -- so key agreement runs over the peer's g. */ + ret = wolfSSH_TestGetDHPrimeGroup(cssh, &group, &groupSz, + &generator, &generatorSz); + if (ret != WS_SUCCESS) { + result = -309; + goto out; + } + if (group != cssh->handshake->primeGroup || + groupSz != cssh->handshake->primeGroupSz) { + result = -310; + goto out; + } + if (generator != cssh->handshake->generator || + generatorSz != 1 || generator[0] != 0x05) { + printf("DhGexGroupAcceptHonorsGenerator: generator not honored\n"); + result = -311; + goto out; + } + +out: + if (cssh != NULL) + wolfSSH_free(cssh); + if (cctx != NULL) + wolfSSH_CTX_free(cctx); + if (sssh != NULL) + wolfSSH_free(sssh); + if (sctx != NULL) + wolfSSH_CTX_free(sctx); + return result; +#else + return 0; +#endif +} + +/* SendKexDhGexGroup frees any previously cached group before caching the freshly + * selected one. A first handshake entry always has primeGroup == NULL, so the + * free-before-realloc branch is never reached by an ordinary handshake. Seed a + * stale cached group, then drive the server request path and confirm the seed + * is replaced by a real modulus (not leaked or left stale). */ +static int test_DhGexGroupSendRecache(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte request[UINT32_SZ * 3]; + word32 idx = 0; + const byte* group = NULL; + word32 groupSz = 0; + const byte* generator = NULL; + word32 generatorSz = 0; + int ret; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -320; + wolfSSH_SetIOSend(ctx, GexSinkSend); + ssh = wolfSSH_new(ctx); + if (ssh == NULL || ssh->handshake == NULL) { + result = -321; + goto out; + } + ssh->handshake->kexId = ID_DH_GEX_SHA256; + + /* Seed a stale, WMALLOC'd 4-byte group so SendKexDhGexGroup must free it + * before caching the selection. The distinctive size proves replacement; + * a leaked seed is caught by the leak checkers CI runs the suite under. */ + ssh->handshake->primeGroup = + (byte*)WMALLOC(4, ssh->ctx->heap, DYNTYPE_MPINT); + if (ssh->handshake->primeGroup == NULL) { + result = -322; + goto out; + } + ssh->handshake->primeGroupSz = 4; + + PutU32BE(request, WOLFSSH_DEFAULT_GEXDH_MIN); + PutU32BE(request + UINT32_SZ, WOLFSSH_DEFAULT_GEXDH_PREFERRED); + PutU32BE(request + UINT32_SZ * 2, WOLFSSH_DEFAULT_GEXDH_MAX); + + ret = wolfSSH_TestDoKexDhGexRequest(ssh, request, sizeof(request), &idx); + if (ret != WS_SUCCESS || ssh->handshake->primeGroup == NULL) { + printf("DhGexGroupSendRecache: request ret %d\n", ret); + result = -323; + goto out; + } + /* The 4-byte seed must be gone, replaced by a real group modulus. */ + if (ssh->handshake->primeGroupSz == 4 || + ssh->handshake->primeGroupSz == 0) { + printf("DhGexGroupSendRecache: cache not replaced, sz %u\n", + ssh->handshake->primeGroupSz); + result = -324; + goto out; + } + /* The exchange-hash path must hand back that same cached group. */ + ret = wolfSSH_TestGetDHPrimeGroup(ssh, &group, &groupSz, + &generator, &generatorSz); + if (ret != WS_SUCCESS || group != ssh->handshake->primeGroup || + groupSz != ssh->handshake->primeGroupSz) { + printf("DhGexGroupSendRecache: hash group desync sz %u vs %u\n", + groupSz, ssh->handshake->primeGroupSz); + result = -325; + goto out; + } + +out: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + #endif /* WOLFSSH_TEST_INTERNAL && !WOLFSSH_NO_DH_GEX_SHA256 */ @@ -7753,6 +8165,26 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("DhGexGroupValidate: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + + unitResult = test_DhGexGroupSelect(); + printf("DhGexGroupSelect: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_DhGexGroupSendHashConsistency(); + printf("DhGexGroupSendHashConsistency: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_DhGexGroupAcceptHonorsGenerator(); + printf("DhGexGroupAcceptHonorsGenerator: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_DhGexGroupSendRecache(); + printf("DhGexGroupSendRecache: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; #endif #ifdef WOLFSSH_TEST_INTERNAL diff --git a/wolfssh/internal.h b/wolfssh/internal.h index a95098648..d3f97ea1f 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1489,6 +1489,9 @@ enum WS_MessageIdLimits { WOLFSSH_API int wolfSSH_TestKeyAgreeDh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz); WOLFSSH_API int wolfSSH_TestSetDhKexKey(WOLFSSH* ssh); + WOLFSSH_API int wolfSSH_TestGetDHPrimeGroup(WOLFSSH* ssh, + const byte** primeGroup, word32* primeGroupSz, + const byte** generator, word32* generatorSz); #endif /* !WOLFSSH_NO_DH */ #ifndef WOLFSSH_NO_ECDH WOLFSSH_API int wolfSSH_TestKeyAgreeEcdh_server(WOLFSSH* ssh, byte hashId, @@ -1504,6 +1507,9 @@ enum WS_MessageIdLimits { WOLFSSH_API int wolfSSH_TestValidateKexDhGexGroup(const byte* primeGroup, word32 primeGroupSz, const byte* generator, word32 generatorSz, word32 minBits, word32 maxBits, WC_RNG* rng); + WOLFSSH_API int wolfSSH_TestSelectKexDhGexGroup(word32 minBits, + word32 preferredBits, word32 maxBits, const byte** primeGroup, + word32* primeGroupSz); #endif /* !WOLFSSH_NO_DH_GEX_SHA256 */ #ifndef WOLFSSH_NO_RSA WOLFSSH_API int wolfSSH_TestRsaVerify(const byte* sig, word32 sigSz, From 2b65dc8028ba4862ff86e387d7639b3c3b58dadd Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 14 Jul 2026 16:47:19 -0700 Subject: [PATCH 3/4] Enforce DH GEX floor on client and server Follows the group-selection change: honoring the client window is not enough if either peer will still accept a sub-floor group. Raise the default GEX minimum to 2048 bits and enforce WOLFSSH_DH_GEX_MIN_BITS on both roles, with the failure-path hardening and low-severity cleanups from the branch review folded in. - Raise WOLFSSH_DEFAULT_GEXDH_MIN 1024 -> 2048 and define WOLFSSH_DH_GEX_MIN_BITS in wolfssh/internal.h so the build and the unit tests share the configured floor. - Server: clamp the client's lower bound up to the floor before the candidate scan; keep group 1 in the set so a lowered floor stays usable; reject a window whose max is below the floor with WS_DH_SIZE_E. Cap the window to MAX_KEX_KEY_SZ so selection can never exceed the server's own key buffers. - Client: enforce WOLFSSH_DH_GEX_MIN_BITS in DoKexDhGexGroup and clamp the advertised min up to the floor in SendKexDhGexRequest, pulling preferred up with it and clamping into [min, max] in both directions (RFC 4419 sec. 3). Reject a configured max below the floor. - Cache the selected group on the handshake in SendKexDhGexGroup and reuse it from GetDHPrimeGroup so the wire group, the exchange hash, and the shared secret cannot diverge. Move KEX_F_SIZE to wolfssh/internal.h behind an #ifndef guard, with #error guards so a too-small override fails the build rather than overflowing f. - Guard KeyAgreeDh_server's wc_FreeDhKey with a keyInited flag so a GetDHPrimeGroup failure cannot free an uninitialized DhKey. - GetDHPrimeGroup's GEX branch pairs the cached prime with the cached generator (client, where a peer may pick g != 2) or dhGenerator on the server; SendKexGetSigningKey now propagates its return (e.g. WS_DH_SIZE_E) instead of flattening it to WS_BAD_ARGUMENT. - Cast sizeof to word32 in the SelectKexDhGexGroup candidate table. - Add test wrappers (wolfSSH_TestSendKexDhGexRequest, wolfSSH_TestGetDHPrimeGroup) and cover the request clamp, sub-floor rejection, an unsatisfiable window, send/hash consistency, a 4096-bit group-16 key agreement, and the cache-miss re-selection path. Issue: F-55 --- src/internal.c | 147 ++++++++--- tests/unit.c | 631 +++++++++++++++++++++++++++++++++++++++++---- wolfssh/internal.h | 52 +++- 3 files changed, 734 insertions(+), 96 deletions(-) diff --git a/src/internal.c b/src/internal.c index 80682b25a..5df210017 100644 --- a/src/internal.c +++ b/src/internal.c @@ -7437,9 +7437,16 @@ static int DoKexDhGexGroup(WOLFSSH* ssh, ret = GetMpint(&generatorSz, &generator, buf, len, &begin); if (ret == WS_SUCCESS) { + /* Enforce the floor on accept as well as on select, so a server can't + * hand us a weak group by ignoring the min we asked for (RFC 8270). */ + word32 minBits = ssh->handshake->dhGexMinSz; + + if (minBits < WOLFSSH_DH_GEX_MIN_BITS) + minBits = WOLFSSH_DH_GEX_MIN_BITS; + ret = ValidateKexDhGexGroup(primeGroup, primeGroupSz, generator, generatorSz, - ssh->handshake->dhGexMinSz, + minBits, ssh->handshake->dhGexMaxSz, ssh->rng); } @@ -12641,19 +12648,6 @@ struct wolfSSH_sigKeyBlockFull { } sk; }; -#ifndef WOLFSSH_NO_NISTP384_MLKEM1024_SHA384 - /* Size of ML-KEM-1024 ciphertext (1568) plus ECC P-384 component (97). */ - #define KEX_F_SIZE 1700 -#elif !defined(WOLFSSH_NO_NISTP256_MLKEM768_SHA256) || \ - !defined(WOLFSSH_NO_CURVE25519_MLKEM768_SHA256) - /* Size of ML-KEM-768 public key (1184) plus ECC/X25519 component. */ - #define KEX_F_SIZE 1300 -#elif !defined(WOLFSSH_NO_DH_GROUP16_SHA512) - #define KEX_F_SIZE (512 + 1) -#else - #define KEX_F_SIZE (256 + 1) -#endif - #ifdef WOLFSSH_NO_MLDSA #define KEX_SIG_SIZE (512) #else @@ -12779,58 +12773,75 @@ static int BuildRFC6187Info(WOLFSSH* ssh, int pubKeyID, #ifndef WOLFSSH_NO_DH_GEX_SHA256 -/* Pick the in-window group closest to preferredBits, ties favoring the - * smaller. WS_DH_SIZE_E if none fits. */ + +/* Pick the smallest in-window group >= preferredBits, else the largest + * in-window group (RFC 4419 sec. 3, as OpenSSH's choose_dh() reads it: the + * comparison is >=, not the RFC's literal >). WS_DH_SIZE_E if none fits. + * See WOLFSSH_DH_GEX_MIN_BITS. */ static int SelectKexDhGexGroup(word32 minBits, word32 preferredBits, word32 maxBits, const byte** primeGroup, word32* primeGroupSz) { + /* Ascending by bits; the scan below depends on that order. No entry may + * exceed (KEX_F_SIZE - 1) * 8 bits, the server's f buffer. */ static const struct { word32 bits; const byte* group; - const word32* groupSz; + word32 groupSz; } candidates[] = { + /* Cast sizeof to word32 to match the file's dhPrimeGroupNSz style; the + * named constants are not constant expressions usable in this static + * initializer. */ #ifndef WOLFSSH_NO_DH_GROUP1_SHA1 - { 1024, dhPrimeGroup1, &dhPrimeGroup1Sz }, + /* Only reachable with a lowered WOLFSSH_DH_GEX_MIN_BITS. */ + { 1024, dhPrimeGroup1, (word32)sizeof(dhPrimeGroup1) }, #endif - { 2048, dhPrimeGroup14, &dhPrimeGroup14Sz }, + { 2048, dhPrimeGroup14, (word32)sizeof(dhPrimeGroup14) }, #ifndef WOLFSSH_NO_DH_GROUP16_SHA512 - { 4096, dhPrimeGroup16, &dhPrimeGroup16Sz }, + { 4096, dhPrimeGroup16, (word32)sizeof(dhPrimeGroup16) }, #endif }; word32 i; word32 best = 0; - word32 bestDelta = 0; int haveBest = 0; int ret = WS_SUCCESS; if (primeGroup == NULL || primeGroupSz == NULL) return WS_BAD_ARGUMENT; + /* Clamp up to the floor; a max below it leaves no candidate, so we reject + * rather than downgrade. */ + if (minBits < WOLFSSH_DH_GEX_MIN_BITS) + minBits = WOLFSSH_DH_GEX_MIN_BITS; + + /* Cap to the smaller of the shared secret buffer in KeyAgreeDh_server and + * the f buffer in SendKexDhReply, less its mpint sign pad. */ + if (maxBits > (word32)(MAX_KEX_KEY_SZ * 8)) + maxBits = (word32)(MAX_KEX_KEY_SZ * 8); + if (maxBits > (word32)((KEX_F_SIZE - 1) * 8)) + maxBits = (word32)((KEX_F_SIZE - 1) * 8); + + /* Ascending scan: keep the last in-window candidate, and stop at the first + * one that reaches preferredBits. */ for (i = 0; i < (word32)(sizeof(candidates) / sizeof(candidates[0])); i++) { word32 bits = candidates[i].bits; - word32 delta; if (bits < minBits || bits > maxBits) continue; - delta = (bits > preferredBits) ? (bits - preferredBits) - : (preferredBits - bits); - /* Ascending scan with a strict '<' keeps the smaller group on a tie. */ - if (!haveBest || delta < bestDelta) { - best = i; - bestDelta = delta; - haveBest = 1; - } + best = i; + haveBest = 1; + if (bits >= preferredBits) + break; } if (!haveBest) { WLOG(WS_LOG_DEBUG, - "DH GEX: no built-in group within client window [%u, %u]", + "DH GEX: no built-in group within effective window [%u, %u]", minBits, maxBits); ret = WS_DH_SIZE_E; } else { *primeGroup = candidates[best].group; - *primeGroupSz = *candidates[best].groupSz; + *primeGroupSz = candidates[best].groupSz; } return ret; @@ -12887,14 +12898,28 @@ static int GetDHPrimeGroup(WOLFSSH* ssh, const byte** primeGroup, case ID_DH_GEX_SHA256: /* Reuse the group SendKexDhGexGroup cached on the handshake so the * exchange hash and the shared secret match the group that was put - * on the wire. Fall back to selecting from the client's window if - * the cache is somehow unset, so this path can never desynchronize - * from the wire group. */ + * on the wire. An unset cache means no GROUP was ever sent (the + * peer skipped GEX_REQUEST); there is no wire group to match, so + * re-select from whatever window the handshake holds. + * + * handshake->primeGroup has a role-dependent meaning: on the server + * SendKexDhGexGroup caches the group we chose (generator always + * dhGenerator, and handshake->generator stays NULL); on the client + * DoKexDhGexGroup caches the group the peer sent and its generator + * separately in handshake->generator. Pair the cached prime with + * the cached generator when present so a peer's g != 2 is honored, + * falling back to dhGenerator on the server. */ if (ssh->handshake->primeGroup != NULL) { *primeGroup = ssh->handshake->primeGroup; *primeGroupSz = ssh->handshake->primeGroupSz; - *generator = dhGenerator; - *generatorSz = dhGeneratorSz; + if (ssh->handshake->generator != NULL) { + *generator = ssh->handshake->generator; + *generatorSz = ssh->handshake->generatorSz; + } + else { + *generator = dhGenerator; + *generatorSz = dhGeneratorSz; + } } else { ret = SelectKexDhGexGroup(ssh->handshake->dhGexMinSz, @@ -13319,9 +13344,11 @@ static int SendKexGetSigningKey(WOLFSSH* ssh, if (ssh->handshake->kexId == ID_DH_GEX_SHA256) { byte primeGroupPad = 0, generatorPad = 0; - if (GetDHPrimeGroup(ssh, &primeGroup, - &primeGroupSz, &generator, &generatorSz) != WS_SUCCESS) { - ret = WS_BAD_ARGUMENT; + if (ret == WS_SUCCESS) { + /* Propagate the real cause (e.g. WS_DH_SIZE_E from the GEX + * group fallback) rather than flattening it. */ + ret = GetDHPrimeGroup(ssh, &primeGroup, + &primeGroupSz, &generator, &generatorSz); } /* Hash in the client's requested minimum key size. */ @@ -13567,12 +13594,16 @@ static int KeyAgreeDh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz) WOLFSSH_UNUSED(hashId); if (ret == WS_SUCCESS) { + int keyInited = 0; + ret = GetDHPrimeGroup(ssh, &primeGroup, &primeGroupSz, &generator, &generatorSz); if (ret == WS_SUCCESS) { ssh->primeGroupSz = primeGroupSz; ret = wc_InitDhKey(privKey); + if (ret == 0) + keyInited = 1; } if (ret == 0) ret = wc_DhSetKey(privKey, primeGroup, primeGroupSz, @@ -13592,7 +13623,8 @@ static int KeyAgreeDh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz) PRIVATE_KEY_LOCK(); } ForceZero(y_ptr, ySz); - wc_FreeDhKey(privKey); + if (keyInited) + wc_FreeDhKey(privKey); } #ifdef WOLFSSH_SMALL_STACK if (y_ptr) @@ -15028,6 +15060,29 @@ int SendKexDhGexRequest(WOLFSSH* ssh) if (ssh == NULL || ssh->handshake == NULL) ret = WS_BAD_ARGUMENT; + /* Advertise the floor DoKexDhGexGroup enforces on the reply, so a lowered + * WOLFSSH_DEFAULT_GEXDH_MIN can't ask for a group we would then reject. */ + if (ret == WS_SUCCESS) { + if (ssh->handshake->dhGexMinSz < WOLFSSH_DH_GEX_MIN_BITS) + ssh->handshake->dhGexMinSz = WOLFSSH_DH_GEX_MIN_BITS; + if (ssh->handshake->dhGexMaxSz < ssh->handshake->dhGexMinSz) { + WLOG(WS_LOG_DEBUG, "DH GEX: max %u below the %u-bit floor", + ssh->handshake->dhGexMaxSz, + (word32)WOLFSSH_DH_GEX_MIN_BITS); + ret = WS_DH_SIZE_E; + } + } + + /* RFC 4419 sec. 3 requires min <= preferred <= max on the wire. Clamp + * preferred into that range in both directions; riding it up with a raised + * min or down under a lowered max keeps the advertised triple ordered. */ + if (ret == WS_SUCCESS) { + if (ssh->handshake->dhGexPreferredSz < ssh->handshake->dhGexMinSz) + ssh->handshake->dhGexPreferredSz = ssh->handshake->dhGexMinSz; + if (ssh->handshake->dhGexPreferredSz > ssh->handshake->dhGexMaxSz) + ssh->handshake->dhGexPreferredSz = ssh->handshake->dhGexMaxSz; + } + if (ret == WS_SUCCESS) { payloadSz = MSG_ID_SZ + (UINT32_SZ * 3); ret = PreparePacket(ssh, payloadSz); @@ -15099,8 +15154,11 @@ int SendKexDhGexGroup(WOLFSSH* ssh) WFREE(ssh->handshake->primeGroup, ssh->ctx->heap, DYNTYPE_MPINT); ssh->handshake->primeGroup = (byte*)WMALLOC(primeGroupSz, ssh->ctx->heap, DYNTYPE_MPINT); - if (ssh->handshake->primeGroup == NULL) + if (ssh->handshake->primeGroup == NULL) { + /* Keep the pointer and size consistent. */ + ssh->handshake->primeGroupSz = 0; ret = WS_MEMORY_E; + } else { WMEMCPY(ssh->handshake->primeGroup, primeGroup, primeGroupSz); ssh->handshake->primeGroupSz = primeGroupSz; @@ -20216,6 +20274,11 @@ int wolfSSH_TestKeyAgreeEcdh_client(WOLFSSH* ssh, byte hashId, #ifndef WOLFSSH_NO_DH_GEX_SHA256 +int wolfSSH_TestSendKexDhGexRequest(WOLFSSH* ssh) +{ + return SendKexDhGexRequest(ssh); +} + int wolfSSH_TestDoKexDhGexRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { diff --git a/tests/unit.c b/tests/unit.c index f7d324a68..36775c157 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -1269,76 +1269,162 @@ static int test_DhGexGroupValidate(void) return result; } +#if WOLFSSH_DH_GEX_MIN_BITS == 2048 +/* Assert the selector returns a group of exactly expectSz bytes, and that the + * bytes really are a built-in DH prime: every one of RFC 2409/3526 group 1, + * 14, and 16 begins and ends with 0xFF. Checking the pointer and not just the + * size keeps a mispaired (group, sizeof) entry in candidates[] from passing. + * Returns 0, or code on mismatch. */ +static int DhGexExpectGroup(word32 minBits, word32 prefBits, word32 maxBits, + word32 expectSz, int code, const char* what) +{ + const byte* group = NULL; + word32 groupSz = 0; + int ret; + + ret = wolfSSH_TestSelectKexDhGexGroup(minBits, prefBits, maxBits, + &group, &groupSz); + if (ret != WS_SUCCESS || groupSz != expectSz || group == NULL) { + printf("DhGexGroupSelect: %s got ret %d sz %u want %u\n", + what, ret, groupSz, expectSz); + return code; + } + if (group[0] != 0xFF || group[groupSz - 1] != 0xFF) { + printf("DhGexGroupSelect: %s group is not a built-in prime\n", what); + return code; + } + return 0; +} +#endif /* WOLFSSH_DH_GEX_MIN_BITS == 2048 */ + +/* The selector must reject the window rather than serve a group outside it. */ +static int DhGexExpectReject(word32 minBits, word32 prefBits, word32 maxBits, + int code, const char* what) +{ + const byte* group = NULL; + word32 groupSz = 0; + int ret; + + ret = wolfSSH_TestSelectKexDhGexGroup(minBits, prefBits, maxBits, + &group, &groupSz); + if (ret != WS_DH_SIZE_E) { + printf("DhGexGroupSelect: %s got ret %d sz %u\n", what, ret, groupSz); + return code; + } + return 0; +} + /* Server-side group selection for DH GEX. Confirms the server honors the * client's requested min/preferred/max window instead of always handing back - * the 2048-bit group 14, and rejects windows no built-in group can satisfy. */ + * the 2048-bit group 14, and rejects windows no built-in group can satisfy. + * The concrete group sizes below are only meaningful at the default 2048-bit + * floor; a build that overrides WOLFSSH_DH_GEX_MIN_BITS gets the + * floor-relative checks instead. */ static int test_DhGexGroupSelect(void) { - const byte* group; - word32 groupSz; int ret; - /* Default-ish window: with group 16 enabled, preferred 3072 is equidistant - * from 2048 and 4096 and the tie favors the smaller group; without group 16, - * 2048 is simply the closest in-window candidate. Either way a stock client - * still receives the historical 2048-bit group (256 bytes). */ - group = NULL; groupSz = 0; - ret = wolfSSH_TestSelectKexDhGexGroup(1024, 3072, 8192, &group, &groupSz); - if (ret != WS_SUCCESS || groupSz != 256) { - printf("DhGexGroupSelect: default window got ret %d sz %u\n", - ret, groupSz); - return -200; - } +#if WOLFSSH_DH_GEX_MIN_BITS == 2048 + /* NULL out-params are rejected before anything else. */ + if (wolfSSH_TestSelectKexDhGexGroup(2048, 2048, 8192, NULL, NULL) + != WS_BAD_ARGUMENT) { + printf("DhGexGroupSelect: NULL out-params not rejected\n"); + return -199; + } + + /* Default-ish window. RFC 4419 sec. 3: return the smallest group at or + * above the client's preferred size. With group 16 that is 4096 (512 + * bytes) for a preferred of 3072; without it -- or when a reduced + * MAX_KEX_KEY_SZ / KEX_F_SIZE caps the selector's maxBits below 4096 -- + * nothing reaches 3072 and the largest in-window group, 2048 (256 bytes), + * is returned instead. Guard on the same buffer sizes SelectKexDhGexGroup + * caps against, matching test_DhGexGroup16KeyAgree. */ +#if !defined(WOLFSSH_NO_DH_GROUP16_SHA512) && \ + (MAX_KEX_KEY_SZ * 8) >= 4096 && ((KEX_F_SIZE - 1) * 8) >= 4096 + ret = DhGexExpectGroup(1024, 3072, 8192, 512, -200, "default window"); +#else + ret = DhGexExpectGroup(1024, 3072, 8192, 256, -200, "default window"); +#endif + if (ret != 0) + return ret; + + /* A preferred size that a group matches exactly takes that group: the + * comparison is >=, not the RFC's literal "larger than". */ + ret = DhGexExpectGroup(2048, 2048, 8192, 256, -201, "exact-preferred"); + if (ret != 0) + return ret; /* A max below 4096 must cap the choice even when preferred is huge. */ - group = NULL; groupSz = 0; - ret = wolfSSH_TestSelectKexDhGexGroup(1024, 8192, 2048, &group, &groupSz); - if (ret != WS_SUCCESS || groupSz != 256) { - printf("DhGexGroupSelect: max-cap window got ret %d sz %u\n", - ret, groupSz); - return -201; - } + ret = DhGexExpectGroup(1024, 8192, 2048, 256, -202, "max-cap window"); + if (ret != 0) + return ret; /* A window no built-in group falls inside must be rejected, not silently * served a group outside it. */ - group = NULL; groupSz = 0; - ret = wolfSSH_TestSelectKexDhGexGroup(3000, 3000, 3500, &group, &groupSz); - if (ret != WS_DH_SIZE_E) { - printf("DhGexGroupSelect: impossible window got ret %d\n", ret); - return -202; - } + ret = DhGexExpectReject(3000, 3000, 3500, -203, "impossible window"); + if (ret != 0) + return ret; -#ifndef WOLFSSH_NO_DH_GROUP16_SHA512 +#if !defined(WOLFSSH_NO_DH_GROUP16_SHA512) && \ + (MAX_KEX_KEY_SZ * 8) >= 4096 && ((KEX_F_SIZE - 1) * 8) >= 4096 /* A client demanding a 4096-bit minimum gets the 4096-bit group (512 * bytes), never a silent downgrade to 2048. */ - group = NULL; groupSz = 0; - ret = wolfSSH_TestSelectKexDhGexGroup(4096, 4096, 8192, &group, &groupSz); - if (ret != WS_SUCCESS || groupSz != 512) { - printf("DhGexGroupSelect: 4096 min got ret %d sz %u\n", ret, groupSz); - return -203; - } + ret = DhGexExpectGroup(4096, 4096, 8192, 512, -204, "4096 min"); #else - /* Without group 16 there is nothing >= 4096, so the request is rejected - * rather than downgraded. */ - group = NULL; groupSz = 0; - ret = wolfSSH_TestSelectKexDhGexGroup(4096, 4096, 8192, &group, &groupSz); - if (ret != WS_DH_SIZE_E) { - printf("DhGexGroupSelect: 4096 min (no group16) got ret %d\n", ret); - return -203; - } + /* Without group 16 -- or when the selector's buffers cap maxBits below + * 4096 -- there is nothing >= 4096, so the request is rejected rather than + * downgraded. */ + ret = DhGexExpectReject(4096, 4096, 8192, -204, "4096 min (no group16)"); #endif + if (ret != 0) + return ret; -#ifndef WOLFSSH_NO_DH_GROUP1_SHA1 - /* A window only the 1024-bit group fits returns group 1 (128 bytes). */ - group = NULL; groupSz = 0; - ret = wolfSSH_TestSelectKexDhGexGroup(1024, 1024, 1536, &group, &groupSz); - if (ret != WS_SUCCESS || groupSz != 128) { - printf("DhGexGroupSelect: 1024 window got ret %d sz %u\n", - ret, groupSz); - return -204; - } + /* A sub-2048 window must be rejected, not downgraded to the 1024-bit group + * 1, even when group 1 is compiled in for direct group1-sha1: the GEX path + * enforces the WOLFSSH_DH_GEX_MIN_BITS (2048) floor from RFC 8270. The + * client's max (1536) is below the floor, so no candidate fits. */ + ret = DhGexExpectReject(1024, 1024, 1536, -205, "sub-2048 window"); + if (ret != 0) + return ret; + + /* A window whose minimum is below the floor but whose max admits 2048 must + * clamp up to the 2048-bit group (256 bytes), never the 1024-bit group. + * The low preferred size keeps the choice at 2048 rather than 4096. */ + ret = DhGexExpectGroup(1024, 1024, 4096, 256, -206, "floor-clamp window"); + if (ret != 0) + return ret; +#else + /* No built-in group lies in [3000, 3500] at any floor: either the window + * is empty on its own, or the clamped min exceeds the max. */ + ret = DhGexExpectReject(3000, 3000, 3500, -207, "impossible window"); + if (ret != 0) + return ret; + + /* A window topping out below the floor is rejected, never downgraded. + * Guarded so a zero floor cannot underflow the max into 0xFFFFFFFF. */ +#if WOLFSSH_DH_GEX_MIN_BITS >= 2 + ret = DhGexExpectReject(1, 1, WOLFSSH_DH_GEX_MIN_BITS - 1, -209, + "sub-floor window"); + if (ret != 0) + return ret; #endif + /* A wide-open window never selects a group below the floor. */ + { + const byte* group = NULL; + word32 groupSz = 0; + + ret = wolfSSH_TestSelectKexDhGexGroup(1, 1, 8192, &group, &groupSz); + if (ret == WS_SUCCESS && + (groupSz * 8) < (word32)WOLFSSH_DH_GEX_MIN_BITS) { + printf("DhGexGroupSelect: floor %d violated, sz %u\n", + WOLFSSH_DH_GEX_MIN_BITS, groupSz); + return -208; + } + } +#endif /* WOLFSSH_DH_GEX_MIN_BITS == 2048 */ + + (void)ret; return 0; } @@ -1445,14 +1531,17 @@ static int test_DhGexGroupSendHashConsistency(void) int ret; /* Default client window. Covers the cache plumbing on every build, - * including group14-only ones. */ + * including group14-only ones. A floor raised above the largest built-in + * group leaves nothing to select, and this case reports that. */ ret = DhGexSendHashConsistencyCase(WOLFSSH_DEFAULT_GEXDH_MIN, WOLFSSH_DEFAULT_GEXDH_PREFERRED, WOLFSSH_DEFAULT_GEXDH_MAX, 0, -210); if (ret != 0) return ret; -#ifndef WOLFSSH_NO_DH_GROUP16_SHA512 +#if !defined(WOLFSSH_NO_DH_GROUP16_SHA512) && \ + WOLFSSH_DH_GEX_MIN_BITS == 2048 && \ + (MAX_KEX_KEY_SZ * 8) >= 4096 && ((KEX_F_SIZE - 1) * 8) >= 4096 /* Force the 4096-bit group 16 (512 bytes) so a send/hash divergence can't * hide behind both paths independently landing on the default group 14. */ ret = DhGexSendHashConsistencyCase(4096, 4096, 8192, 512, -230); @@ -1463,6 +1552,417 @@ static int test_DhGexGroupSendHashConsistency(void) return 0; } +/* Exercise the GetDHPrimeGroup cache-miss fallback: with the cached group + * cleared, it must re-select the same group from the client window (and set a + * generator), not desync from the wire. */ +static int test_DhGexGroupCacheMissFallback(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte request[UINT32_SZ * 3]; + word32 idx = 0; + const byte* group = NULL; + word32 groupSz = 0; + const byte* generator = NULL; + word32 generatorSz = 0; + word32 wireSz = 0; + int ret; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -240; + wolfSSH_SetIOSend(ctx, GexSinkSend); + ssh = wolfSSH_new(ctx); + if (ssh == NULL || ssh->handshake == NULL) { + result = -241; + goto out; + } + ssh->handshake->kexId = ID_DH_GEX_SHA256; + + PutU32BE(request, WOLFSSH_DEFAULT_GEXDH_MIN); + PutU32BE(request + UINT32_SZ, WOLFSSH_DEFAULT_GEXDH_PREFERRED); + PutU32BE(request + UINT32_SZ * 2, WOLFSSH_DEFAULT_GEXDH_MAX); + + ret = wolfSSH_TestDoKexDhGexRequest(ssh, request, sizeof(request), &idx); + if (ret != WS_SUCCESS || ssh->handshake->primeGroup == NULL) { + printf("DhGexGroupCacheMissFallback: request ret %d\n", ret); + result = -242; + goto out; + } + wireSz = ssh->handshake->primeGroupSz; + + /* Drop the cache to force the fallback branch. primeGroup is a WMALLOC'd + * copy; the fallback returns a pointer into static group data. */ + WFREE(ssh->handshake->primeGroup, ssh->ctx->heap, DYNTYPE_MPINT); + ssh->handshake->primeGroup = NULL; + + ret = wolfSSH_TestGetDHPrimeGroup(ssh, &group, &groupSz, + &generator, &generatorSz); + if (ret != WS_SUCCESS) { + printf("DhGexGroupCacheMissFallback: fallback ret %d\n", ret); + result = -243; + goto out; + } + /* Must re-select the group the wire used, with a non-NULL generator. */ + if (group == NULL || groupSz != wireSz || generator == NULL || + generatorSz == 0) { + printf("DhGexGroupCacheMissFallback: group sz %u wire %u gen %p\n", + groupSz, wireSz, (const void*)generator); + result = -244; + goto out; + } + +out: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + +/* Drive the real server request path with a client window no built-in group + * satisfies (3000..3500, and no group exists between 2048 and 4096). The server + * must return WS_DH_SIZE_E and leave no group cached, rather than emit a + * KEXDH_GEX_GROUP with a NULL prime. The window is impossible at any floor, so + * this is not gated on WOLFSSH_DH_GEX_MIN_BITS. */ +static int test_DhGexServerRejectsUnsatisfiableWindow(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte request[UINT32_SZ * 3]; + word32 idx = 0; + int ret; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -280; + wolfSSH_SetIOSend(ctx, GexSinkSend); + ssh = wolfSSH_new(ctx); + if (ssh == NULL || ssh->handshake == NULL) { + result = -281; + goto out; + } + ssh->handshake->kexId = ID_DH_GEX_SHA256; + + PutU32BE(request, 3000); + PutU32BE(request + UINT32_SZ, 3000); + PutU32BE(request + UINT32_SZ * 2, 3500); + + ret = wolfSSH_TestDoKexDhGexRequest(ssh, request, sizeof(request), &idx); + if (ret != WS_DH_SIZE_E) { + printf("DhGexServerRejectsUnsatisfiableWindow: ret %d want %d\n", + ret, WS_DH_SIZE_E); + result = -282; + goto out; + } + /* A rejected selection must not leave a half-cached group behind. */ + if (ssh->handshake->primeGroup != NULL || + ssh->handshake->primeGroupSz != 0) { + printf("DhGexServerRejectsUnsatisfiableWindow: stale cache %p sz %u\n", + (const void*)ssh->handshake->primeGroup, + ssh->handshake->primeGroupSz); + result = -283; + goto out; + } + +out: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + +/* Drive SendKexDhGexRequest with the window preset on the handshake. The send + * sink swallows the packet; the caller inspects the window it advertised. */ +static int DhGexRequestCase(word32 minBits, word32 prefBits, word32 maxBits, + int expectRet, WOLFSSH_CTX* ctx, WOLFSSH** sshOut) +{ + WOLFSSH* ssh = wolfSSH_new(ctx); + + *sshOut = ssh; + if (ssh == NULL || ssh->handshake == NULL) + return -1; + + ssh->handshake->kexId = ID_DH_GEX_SHA256; + ssh->handshake->dhGexMinSz = minBits; + ssh->handshake->dhGexPreferredSz = prefBits; + ssh->handshake->dhGexMaxSz = maxBits; + + return (wolfSSH_TestSendKexDhGexRequest(ssh) == expectRet) ? 0 : -1; +} + +/* The client must advertise the same minimum DoKexDhGexGroup enforces on the + * reply. Otherwise a lowered WOLFSSH_DEFAULT_GEXDH_MIN asks for a group that + * the accept path then rejects. */ +static int test_DhGexRequestFloorClamp(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + /* Window edges expressed relative to the floor so the clamp is exercised at + * any WOLFSSH_DH_GEX_MIN_BITS, not just the 2048-bit default. */ + word32 subFloor = WOLFSSH_DH_GEX_MIN_BITS / 2; + word32 prefAbove = WOLFSSH_DH_GEX_MIN_BITS + (WOLFSSH_DH_GEX_MIN_BITS / 2); + word32 maxHigh = WOLFSSH_DH_GEX_MIN_BITS * 4; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -290; + wolfSSH_SetIOSend(ctx, GexSinkSend); + + /* A sub-floor min is raised to the floor, and the preferred size rides up + * with it so the advertised triple stays min <= preferred. */ + if (DhGexRequestCase(subFloor, subFloor, maxHigh, WS_SUCCESS, ctx, &ssh) + != 0) { + result = -291; + goto out; + } + if (ssh->handshake->dhGexMinSz != WOLFSSH_DH_GEX_MIN_BITS || + ssh->handshake->dhGexPreferredSz != WOLFSSH_DH_GEX_MIN_BITS) { + printf("DhGexRequestFloorClamp: min %u pref %u want %u\n", + ssh->handshake->dhGexMinSz, ssh->handshake->dhGexPreferredSz, + (word32)WOLFSSH_DH_GEX_MIN_BITS); + result = -292; + goto out; + } + wolfSSH_free(ssh); + ssh = NULL; + + /* A window topping out below the floor has nothing to ask for. Reject it + * here rather than emit a request whose reply we would refuse. */ + if (DhGexRequestCase(subFloor, subFloor, WOLFSSH_DH_GEX_MIN_BITS - 1, + WS_DH_SIZE_E, ctx, &ssh) != 0) { + result = -293; + goto out; + } + wolfSSH_free(ssh); + ssh = NULL; + + /* A window already at or above the floor is advertised untouched. */ + if (DhGexRequestCase(WOLFSSH_DH_GEX_MIN_BITS, prefAbove, maxHigh, WS_SUCCESS, + ctx, &ssh) != 0) { + result = -294; + goto out; + } + if (ssh->handshake->dhGexMinSz != WOLFSSH_DH_GEX_MIN_BITS || + ssh->handshake->dhGexPreferredSz != prefAbove || + ssh->handshake->dhGexMaxSz != maxHigh) { + result = -295; + goto out; + } + wolfSSH_free(ssh); + ssh = NULL; + + /* A preferred size above the max is clamped down to the max, so the + * advertised triple keeps min <= preferred <= max (RFC 4419 sec. 3). */ + if (DhGexRequestCase(WOLFSSH_DH_GEX_MIN_BITS, prefAbove, + WOLFSSH_DH_GEX_MIN_BITS, WS_SUCCESS, ctx, &ssh) != 0) { + result = -296; + goto out; + } + if (ssh->handshake->dhGexPreferredSz != WOLFSSH_DH_GEX_MIN_BITS || + ssh->handshake->dhGexMaxSz != WOLFSSH_DH_GEX_MIN_BITS) { + printf("DhGexRequestFloorClamp: pref %u max %u want %u\n", + ssh->handshake->dhGexPreferredSz, ssh->handshake->dhGexMaxSz, + (word32)WOLFSSH_DH_GEX_MIN_BITS); + result = -297; + goto out; + } + +out: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + +/* Mirror of test_DhGexRequestFloorClamp for the accept side: even when the + * client advertised a sub-floor min, DoKexDhGexGroup must clamp the accept + * minimum up to WOLFSSH_DH_GEX_MIN_BITS and reject a below-floor group the + * server sent anyway (RFC 8270). Without the clamp, a 1024-bit prime paired + * with dhGexMinSz == 1024 would pass. The size check runs before primality, + * so an arbitrary 1024-bit value is a sufficient fixture. */ +static int test_DhGexGroupAcceptFloor(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + /* A prime half the floor's bit length: below the floor at any + * WOLFSSH_DH_GEX_MIN_BITS, so the accept-side clamp must reject it. */ + word32 magBytes = WOLFSSH_DH_GEX_MIN_BITS / 16; + byte group[UINT32_SZ + 1 + (WOLFSSH_DH_GEX_MIN_BITS / 16) + + UINT32_SZ + 1]; + word32 gIdx = 0; + word32 idx = 0; + int ret; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -270; + ssh = wolfSSH_new(ctx); + if (ssh == NULL || ssh->handshake == NULL) { + result = -271; + goto out; + } + ssh->handshake->kexId = ID_DH_GEX_SHA256; + /* Sub-floor min: the accept-side clamp, not this value, must govern. */ + ssh->handshake->dhGexMinSz = WOLFSSH_DH_GEX_MIN_BITS / 2; + ssh->handshake->dhGexMaxSz = WOLFSSH_DH_GEX_MIN_BITS * 4; + + /* mpint prime: a canonical (floor/2)-bit value (0x00 sign pad, then a 0x80 + * top byte so GetMpint accepts it), followed by mpint generator 2. The + * size check runs before primality, so an arbitrary value is sufficient. */ + PutU32BE(group + gIdx, 1 + magBytes); + gIdx += UINT32_SZ; + group[gIdx++] = 0x00; + group[gIdx++] = 0x80; + WMEMSET(group + gIdx, 0xFF, magBytes - 1); + gIdx += magBytes - 1; + PutU32BE(group + gIdx, 1); + gIdx += UINT32_SZ; + group[gIdx++] = 0x02; + + ret = wolfSSH_TestDoKexDhGexGroup(ssh, group, gIdx, &idx); + if (ret != WS_DH_SIZE_E) { + printf("DhGexGroupAcceptFloor: ret %d want %d\n", ret, WS_DH_SIZE_E); + result = -272; + goto out; + } + +out: + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +} + +/* Drive a real 4096-bit GEX key agreement on the server. Selecting group 16 is + * now the ordinary outcome for a stock client window (preferred 3072), so the + * 512-byte modulus has to survive KeyAgreeDh_server's f buffer (KEX_F_SIZE) + * and shared-secret buffer (ssh->k). Confirms the server's f and k are a real + * DH pair by recomputing the secret from the client side. */ +static int test_DhGexGroup16KeyAgree(void) +{ +#if !defined(WOLFSSH_NO_DH_GROUP16_SHA512) && \ + WOLFSSH_DH_GEX_MIN_BITS == 2048 && \ + (MAX_KEX_KEY_SZ * 8) >= 4096 && ((KEX_F_SIZE - 1) * 8) >= 4096 + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte request[UINT32_SZ * 3]; + word32 idx = 0; + const byte* group = NULL; + word32 groupSz = 0; + const byte* generator = NULL; + word32 generatorSz = 0; + DhKey clientKey; + int keyInited = 0; + byte f[KEX_F_SIZE]; + word32 fSz = (word32)sizeof(f); + byte cPriv[MAX_KEX_KEY_SZ]; + word32 cPrivSz = (word32)sizeof(cPriv); + byte cPub[KEX_F_SIZE]; + word32 cPubSz = (word32)sizeof(cPub); + byte cSecret[MAX_KEX_KEY_SZ]; + word32 cSecretSz = (word32)sizeof(cSecret); + int ret; + int result = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -250; + wolfSSH_SetIOSend(ctx, GexSinkSend); + ssh = wolfSSH_new(ctx); + if (ssh == NULL || ssh->handshake == NULL) { + result = -251; + goto out; + } + ssh->handshake->kexId = ID_DH_GEX_SHA256; + + /* Window that resolves to group 16 under the RFC 4419 rule. */ + PutU32BE(request, 2048); + PutU32BE(request + UINT32_SZ, 4096); + PutU32BE(request + UINT32_SZ * 2, 8192); + + ret = wolfSSH_TestDoKexDhGexRequest(ssh, request, sizeof(request), &idx); + if (ret != WS_SUCCESS || ssh->handshake->primeGroupSz != 512) { + printf("DhGexGroup16KeyAgree: request ret %d sz %u\n", + ret, ssh->handshake->primeGroupSz); + result = -252; + goto out; + } + + /* Build a client key pair over the very group the server put on the wire. */ + ret = wolfSSH_TestGetDHPrimeGroup(ssh, &group, &groupSz, + &generator, &generatorSz); + if (ret != WS_SUCCESS) { + result = -253; + goto out; + } + if (wc_InitDhKey(&clientKey) != 0) { + result = -254; + goto out; + } + keyInited = 1; + if (wc_DhSetKey(&clientKey, group, groupSz, generator, generatorSz) != 0) { + result = -255; + goto out; + } + if (wc_DhGenerateKeyPair(&clientKey, ssh->rng, cPriv, &cPrivSz, + cPub, &cPubSz) != 0) { + result = -256; + 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; + ssh->kSz = MAX_KEX_KEY_SZ; + + ret = wolfSSH_TestKeyAgreeDh_server(ssh, WC_HASH_TYPE_SHA256, f, &fSz); + if (ret != WS_SUCCESS || fSz == 0 || fSz > 512) { + printf("DhGexGroup16KeyAgree: agree ret %d fSz %u\n", ret, fSz); + result = -257; + goto out; + } + if (ssh->primeGroupSz != 512 || ssh->kSz == 0) { + printf("DhGexGroup16KeyAgree: primeGroupSz %u kSz %u\n", + ssh->primeGroupSz, ssh->kSz); + result = -258; + goto out; + } + + /* The server's k must be the shared secret the client derives from f. */ + if (wc_DhAgree(&clientKey, cSecret, &cSecretSz, cPriv, cPrivSz, + f, fSz) != 0) { + result = -259; + goto out; + } + if (cSecretSz != ssh->kSz || + WMEMCMP(cSecret, ssh->k, cSecretSz) != 0) { + printf("DhGexGroup16KeyAgree: secret mismatch, client %u server %u\n", + cSecretSz, ssh->kSz); + result = -260; + goto out; + } + +out: + if (keyInited) + wc_FreeDhKey(&clientKey); + if (ssh != NULL) + wolfSSH_free(ssh); + if (ctx != NULL) + wolfSSH_CTX_free(ctx); + return result; +#else + return 0; +#endif +} + /* Drive a successful client-side GEX GROUP accept and confirm the client honors * a peer's non-default generator (g != 2). DoKexDhGexGroup caches the wire prime * and generator on the handshake, then GetDHPrimeGroup must hand both back to @@ -8171,11 +8671,36 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_DhGexRequestFloorClamp(); + printf("DhGexRequestFloorClamp: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_DhGexGroupAcceptFloor(); + printf("DhGexGroupAcceptFloor: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_DhGexServerRejectsUnsatisfiableWindow(); + printf("DhGexServerRejectsUnsatisfiableWindow: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + unitResult = test_DhGexGroupSendHashConsistency(); printf("DhGexGroupSendHashConsistency: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_DhGexGroupCacheMissFallback(); + printf("DhGexGroupCacheMissFallback: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_DhGexGroup16KeyAgree(); + printf("DhGexGroup16KeyAgree: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + unitResult = test_DhGexGroupAcceptHonorsGenerator(); printf("DhGexGroupAcceptHonorsGenerator: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index d3f97ea1f..8c76b5f2f 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -511,7 +511,7 @@ enum NameIdType { #define WOLFSSH_MAX_NAMELIST_CNT 64 #endif #ifndef WOLFSSH_DEFAULT_GEXDH_MIN - #define WOLFSSH_DEFAULT_GEXDH_MIN 1024 + #define WOLFSSH_DEFAULT_GEXDH_MIN 2048 #endif #ifndef WOLFSSH_DEFAULT_GEXDH_PREFERRED #define WOLFSSH_DEFAULT_GEXDH_PREFERRED 3072 @@ -519,6 +519,55 @@ enum NameIdType { #ifndef WOLFSSH_DEFAULT_GEXDH_MAX #define WOLFSSH_DEFAULT_GEXDH_MAX 8192 #endif +/* Floor on the GEX modulus, enforced when the server selects a group and when + * the client accepts one (RFC 8270). Lowering it below 2048 re-enables group + * 1, when group 1 is compiled in. */ +#ifndef WOLFSSH_DH_GEX_MIN_BITS + #define WOLFSSH_DH_GEX_MIN_BITS 2048 +#endif +/* Size of the buffer holding the server's KEX public value f. For DH this + * bounds the modulus: a p of n bytes needs n+1 here for the mpint sign pad. */ +#ifndef KEX_F_SIZE + #ifndef WOLFSSH_NO_NISTP384_MLKEM1024_SHA384 + /* Size of ML-KEM-1024 ciphertext (1568) plus ECC P-384 component + * (97). */ + #define KEX_F_SIZE 1700 + #elif !defined(WOLFSSH_NO_NISTP256_MLKEM768_SHA256) || \ + !defined(WOLFSSH_NO_CURVE25519_MLKEM768_SHA256) + /* Size of ML-KEM-768 public key (1184) plus ECC/X25519 component. */ + #define KEX_F_SIZE 1300 + #elif !defined(WOLFSSH_NO_DH_GROUP16_SHA512) + #define KEX_F_SIZE (512 + 1) + #else + #define KEX_F_SIZE (256 + 1) + #endif +#endif +/* A user-supplied KEX_F_SIZE override must still hold the largest f/ciphertext + * the enabled algorithms write; the KeyAgree*_server paths do not bound-check + * it at runtime. A DH-enabled build writes at least a classical DH group 14 f + * (a 2048-bit p needs 257 bytes) and its ECDH points are smaller, so assert + * that floor whenever DH is compiled in -- it matches the (256 + 1) baseline + * the derivation above falls back to. DH GEX is self-limiting: SelectKexDhGexGroup + * caps its selection at (KEX_F_SIZE - 1) * 8 bits, so it needs no separate + * assert. Add the larger group 16 and ML-KEM sizes on top only when those are + * enabled. A DH-disabled (ECC/Curve25519/ML-KEM-only) build writes no DH f; its + * largest f fits well under the 257-byte default, and the ML-KEM asserts below + * cover the PQ builds, so this floor does not apply there. */ +#ifndef WOLFSSH_NO_DH + #if KEX_F_SIZE < (256 + 1) + #error "KEX_F_SIZE too small for DH group 14 (2048-bit p needs 257 bytes)" + #endif +#endif +#if !defined(WOLFSSH_NO_NISTP384_MLKEM1024_SHA384) && KEX_F_SIZE < 1700 + #error "KEX_F_SIZE too small for the ML-KEM-1024 ciphertext" +#endif +#if (!defined(WOLFSSH_NO_NISTP256_MLKEM768_SHA256) || \ + !defined(WOLFSSH_NO_CURVE25519_MLKEM768_SHA256)) && KEX_F_SIZE < 1300 + #error "KEX_F_SIZE too small for the ML-KEM-768 artifact" +#endif +#if !defined(WOLFSSH_NO_DH_GROUP16_SHA512) && KEX_F_SIZE < (512 + 1) + #error "KEX_F_SIZE too small for DH group 16 (4096-bit p needs 513 bytes)" +#endif #ifndef MAX_KEX_KEY_SZ #ifndef WOLFSSH_NO_NISTP384_MLKEM1024_SHA384 /* Private key size of ML-KEM 1024. Biggest artifact. */ @@ -1500,6 +1549,7 @@ enum WS_MessageIdLimits { const byte* f, word32 fSz); #endif /* !WOLFSSH_NO_ECDH */ #ifndef WOLFSSH_NO_DH_GEX_SHA256 + WOLFSSH_API int wolfSSH_TestSendKexDhGexRequest(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_TestDoKexDhGexRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestDoKexDhGexGroup(WOLFSSH* ssh, byte* buf, From d604c1d2dfcfaf76115693012e39f3af9727dc78 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 16 Jul 2026 16:48:48 -0700 Subject: [PATCH 4/4] Address DH GEX review comments - Shorten the KEX_F_SIZE and GetDHPrimeGroup comments to the facts the code doesn't already show. - Move the role-dependent meaning of handshake->primeGroup to its declaration, since SendKexDhGexGroup, DoKexDhGexGroup, and GetDHPrimeGroup all depend on it. - Brace the primeGroup free in SendKexDhGexGroup to match the if/else below it. --- src/internal.c | 21 ++++++--------------- wolfssh/internal.h | 20 +++++++++----------- 2 files changed, 15 insertions(+), 26 deletions(-) diff --git a/src/internal.c b/src/internal.c index 5df210017..e2e73a73e 100644 --- a/src/internal.c +++ b/src/internal.c @@ -5534,7 +5534,7 @@ static const word32 dhPrimeGroup16Sz = (word32)sizeof(dhPrimeGroup16); #endif /* RFC 4253 sec. 7.1: discard the KEX packet a peer sent after a wrong - * first_packet_follows guess. Leave expectMsgId at MSGID_NONE -- the real + * first_packet_follows guess. Leave expectMsgId at MSGID_NONE, the real * follow-up's ID comes from the negotiated KEX, not the guess, and may cross * the GEX boundary. Returns 1 if consumed, else 0. Caller validates handshake. */ static int SkipGuessedKexMsg(WOLFSSH* ssh, const char* what, @@ -12896,19 +12896,9 @@ static int GetDHPrimeGroup(WOLFSSH* ssh, const byte** primeGroup, #endif #ifndef WOLFSSH_NO_DH_GEX_SHA256 case ID_DH_GEX_SHA256: - /* Reuse the group SendKexDhGexGroup cached on the handshake so the - * exchange hash and the shared secret match the group that was put - * on the wire. An unset cache means no GROUP was ever sent (the - * peer skipped GEX_REQUEST); there is no wire group to match, so - * re-select from whatever window the handshake holds. - * - * handshake->primeGroup has a role-dependent meaning: on the server - * SendKexDhGexGroup caches the group we chose (generator always - * dhGenerator, and handshake->generator stays NULL); on the client - * DoKexDhGexGroup caches the group the peer sent and its generator - * separately in handshake->generator. Pair the cached prime with - * the cached generator when present so a peer's g != 2 is honored, - * falling back to dhGenerator on the server. */ + /* Reuse the cached wire group so the exchange hash and the shared + * secret match what was sent. An unset cache means the peer skipped + * GEX_REQUEST, so re-select from the handshake's window. */ if (ssh->handshake->primeGroup != NULL) { *primeGroup = ssh->handshake->primeGroup; *primeGroupSz = ssh->handshake->primeGroupSz; @@ -15150,8 +15140,9 @@ int SendKexDhGexGroup(WOLFSSH* ssh) /* Cache the selected group so the exchange hash and shared secret * (GetDHPrimeGroup) reuse exactly what goes on the wire here. */ if (ret == WS_SUCCESS) { - if (ssh->handshake->primeGroup != NULL) + if (ssh->handshake->primeGroup != NULL) { WFREE(ssh->handshake->primeGroup, ssh->ctx->heap, DYNTYPE_MPINT); + } ssh->handshake->primeGroup = (byte*)WMALLOC(primeGroupSz, ssh->ctx->heap, DYNTYPE_MPINT); if (ssh->handshake->primeGroup == NULL) { diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 8c76b5f2f..d209fe982 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -542,17 +542,11 @@ enum NameIdType { #define KEX_F_SIZE (256 + 1) #endif #endif -/* A user-supplied KEX_F_SIZE override must still hold the largest f/ciphertext - * the enabled algorithms write; the KeyAgree*_server paths do not bound-check - * it at runtime. A DH-enabled build writes at least a classical DH group 14 f - * (a 2048-bit p needs 257 bytes) and its ECDH points are smaller, so assert - * that floor whenever DH is compiled in -- it matches the (256 + 1) baseline - * the derivation above falls back to. DH GEX is self-limiting: SelectKexDhGexGroup - * caps its selection at (KEX_F_SIZE - 1) * 8 bits, so it needs no separate - * assert. Add the larger group 16 and ML-KEM sizes on top only when those are - * enabled. A DH-disabled (ECC/Curve25519/ML-KEM-only) build writes no DH f; its - * largest f fits well under the 257-byte default, and the ML-KEM asserts below - * cover the PQ builds, so this floor does not apply there. */ +/* The KeyAgree*_server paths do not bound-check KEX_F_SIZE at runtime, so + * assert here that an override still holds every enabled algorithm's f. DH GEX + * needs no assert: SelectKexDhGexGroup caps itself at (KEX_F_SIZE - 1) * 8 + * bits. A DH-disabled build writes no DH f and its ECDH points fit the 257-byte + * default, so the DH floor is guarded on WOLFSSH_NO_DH. */ #ifndef WOLFSSH_NO_DH #if KEX_F_SIZE < (256 + 1) #error "KEX_F_SIZE too small for DH group 14 (2048-bit p needs 257 bytes)" @@ -779,6 +773,10 @@ typedef struct HandshakeInfo { word32 dhGexMinSz; word32 dhGexPreferredSz; word32 dhGexMaxSz; + /* GEX group cache, role-dependent. Server: SendKexDhGexGroup stores the + * group it chose and leaves generator NULL, meaning dhGenerator. Client: + * DoKexDhGexGroup stores the group the peer sent and its generator. A NULL + * primeGroup means no GROUP crossed the wire. */ byte* primeGroup; word32 primeGroupSz; byte* generator;