Skip to content

DH GEX group selection hardening #1056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ejohnstown wants to merge 3 commits into
base: master
Choose a base branch
from
+471 −10
Open

DH GEX group selection hardening #1056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6db3b7f
Discard guessed KEX packet in DoKexDhGexGroup
ejohnstown Jun 23, 2026
20a6463
Select DH GEX group from client size window
ejohnstown Jun 23, 2026
167c037
Enforce 2048-bit floor on DH GEX group select
ejohnstown Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
192 changes: 182 additions & 10 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6765,10 +6765,23 @@ 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) {
if (ssh->handshake->ignoreNextKexMsg) {
/* 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 (RFC
* 4253 7.1), mirroring the other Do* handlers. */
WLOG(WS_LOG_DEBUG, "Skipping server's KEXDH_GEX_GROUP message due "
"to first_packet_follows guess mismatch.");
ssh->handshake->ignoreNextKexMsg = 0;
*idx += len;
return WS_SUCCESS;
}

begin = *idx;
ret = GetMpint(&primeGroupSz, &primeGroup, buf, len, &begin);
if (ret == WS_SUCCESS && primeGroupSz > (MAX_KEX_KEY_SZ + 1)) {
Expand Down Expand Up @@ -11757,11 +11770,107 @@ static int BuildRFC6187Info(WOLFSSH* ssh, int pubKeyID,
#endif /* WOLFSSH_CERTS */


#ifndef WOLFSSH_NO_DH_GEX_SHA256

/* Lower bound on the GEX modulus the server will hand out, regardless of how
* small a window the client requests. RFC 8270 (updating RFC 4419) says a
* server SHOULD NOT select a group smaller than 2048 bits; clamping here keeps
* the 1024-bit group 1 from being reachable via GEX even when it is otherwise
* enabled for direct group1-sha1 negotiation. */
#ifndef WOLFSSH_DH_GEX_MIN_BITS
#define WOLFSSH_DH_GEX_MIN_BITS 2048
#endif

/* Select a built-in DH group for a GEX exchange that fits the client's
* requested window. Picks the group whose modulus size in bits lies within
* [minBits, maxBits] and is closest to preferredBits; ties favor the smaller
* group, which preserves the historical 2048-bit choice for a default client.
* The client's lower bound is first raised to WOLFSSH_DH_GEX_MIN_BITS so the
* server never downgrades below the 2048-bit floor (RFC 8270). Returns
* WS_SUCCESS with primeGroup/primeGroupSz set, or WS_DH_SIZE_E if no built-in
* group falls inside the (clamped) client window -- the server rejects the
* exchange rather than silently handing back a group the client did not ask
* for (RFC 4419 sec. 3). */
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

@aidangarske aidangarske Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] 1024-bit group 1 candidate is unreachable (dead entry) · convention

The candidates[] table includes the 1024-bit group 1 (guarded by WOLFSSH_NO_DH_GROUP1_SHA1), but minBits is unconditionally raised to WOLFSSH_DH_GEX_MIN_BITS (2048) before the scan (verified at internal.c:11822-11823), so the bits < minBits check at 11835 always skips the 1024 entry. It can never be selected. This is intentional/defensive per the header comment, and harmless, but the entry is effectively dead code that could mislead a future reader into thinking 1024 is selectable via GEX.

Fix: Either drop the group 1 entry (the floor already guarantees it is never chosen) or keep a one-line note that it is present only for documentary parity; no functional change needed.

{ 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;

/* Never select below the 2048-bit floor even if the client asks for less.
* If the client's max is also below the floor, no candidate matches and we
* reject (WS_DH_SIZE_E) rather than downgrade. */
if (minBits < WOLFSSH_DH_GEX_MIN_BITS)
minBits = WOLFSSH_DH_GEX_MIN_BITS;

/* Cap to the server's own key buffer (MAX_KEX_KEY_SZ sizes the private
* exponent in KeyAgreeDh_server) so the client window can't select a group
* larger than it holds. */
if (maxBits > (word32)(MAX_KEX_KEY_SZ * 8))
maxBits = (word32)(MAX_KEX_KEY_SZ * 8);

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) {

@aidangarske aidangarske Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [Medium] Server now rejects GEX windows it can't satisfy (behavior change vs. always-group14) · Behavior change / interop

Previously the server unconditionally sent group 14 (2048-bit) for every GEX request. Now SelectKexDhGexGroup clamps the client's minBits up to WOLFSSH_DH_GEX_MIN_BITS (2048) and returns WS_DH_SIZE_E when no built-in group falls in the effective window. This is the intended hardening, but it is an externally observable behavior change: a client that requests a window not covered by the compiled-in groups now gets a hard handshake failure instead of a working exchange. Two interop cases worth confirming: (a) a client requesting min > 2048 when WOLFSSH_NO_DH_GROUP16_SHA512 is defined (only 2048 available) — the server now fails the handshake; (b) a build where WOLFSSH_DEFAULT_GEXDH_MAX/MAX_KEX_KEY_SZ caps maxBits below a client's minimum. The stock wolfSSH client (min=1024, pref=3072, max=8192) still resolves to 2048, so wolfSSH↔wolfSSH is unaffected. Verified: minBits is unconditionally raised to the 2048 floor (11822-11823), maxBits is capped to MAX_KEX_KEY_SZ*8 (11828-11829), and the empty-window path returns WS_DH_SIZE_E (11847-11851).

Fix: Confirm this rejection path is acceptable for the interop matrix (especially clients configured with min>2048 on builds without group 16). Optionally log at WS_LOG_INFO/WARN rather than DEBUG so the rejection is diagnosable in the field.

WLOG(WS_LOG_DEBUG,
"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;
}

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
Expand Down Expand Up @@ -11798,10 +11907,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:
Expand Down Expand Up @@ -12132,7 +12258,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;
}
Expand Down Expand Up @@ -12365,7 +12491,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) {
Expand Down Expand Up @@ -13786,8 +13912,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;
Expand All @@ -13798,6 +13924,32 @@ 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 on the handshake so the exchange hash and the
* shared secret (GetDHPrimeGroup) reuse exactly what goes on the wire here,
* making the wire group the single source of truth instead of re-running
* the selection independently. */
if (ret == WS_SUCCESS) {
if (ssh->handshake->primeGroup)

@aidangarske aidangarske Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] New null checks use implicit-pointer form instead of != NULL · style

The new caching block null-checks with if (ssh->handshake->primeGroup) (verified at internal.c:13941) rather than the wolfSSL-preferred if (ssh->handshake->primeGroup != NULL). GetDHPrimeGroup (line 11915) correctly uses != NULL. Behavior is identical and it mirrors the adjacent pre-existing style in DoKexDhGexGroup, so it is locally consistent and purely a style-consistency note, not a defect. Both reviewers flagged the same line: the review mode rated it Low/NIT and the review-security mode rated it Info; the stricter Low is retained here.

Fix: Use if (ssh->handshake->primeGroup != NULL) for the new pointer check to match wolfSSL C style; leave the pre-existing block unchanged and do not reformat surrounding lines.

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;
Expand Down Expand Up @@ -18532,6 +18684,13 @@ int wolfSSH_TestKeyAgreeDh_server(WOLFSSH* ssh, byte hashId,
return KeyAgreeDh_server(ssh, hashId, f, fSz);
}

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_DH_GEX_SHA256
Expand All @@ -18542,6 +18701,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)
Expand All @@ -18550,6 +18715,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
Expand Down
2 changes: 2 additions & 0 deletions tests/regress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2634,6 +2634,8 @@ 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);
#endif
RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhReply,
"DoKexDhReply", WOLFSSH_ENDPOINT_CLIENT, SERVER_KEXINIT_DONE);
Expand Down
Loading
Loading