DH GEX group selection hardening#1056
Conversation
- 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). - Add the guard already used by DoKexDhReply / DoKexDhInit: consume the packet, clear the flag, return WS_SUCCESS. - Extend TestFirstPacketFollowsSkipped via a new wolfSSH_TestDoKexDhGexGroup wrapper. Issue: F-2866
- 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
There was a problem hiding this comment.
Pull request overview
Hardens server-side Diffie-Hellman Group Exchange (DH-GEX, RFC 4419/8270) by selecting a built-in DH group that actually matches the client’s requested size window (with a 2048-bit minimum floor), and ensures consistency between the group sent on the wire and the group used for exchange-hash / shared-secret computation. It also aligns DH-GEX message handlers with existing first_packet_follows skip behavior.
Changes:
- Add server-side DH-GEX group selection (
SelectKexDhGexGroup) honoring the client’s[min, preferred, max]window, with a 2048-bit floor and rejection when no built-in group fits. - Cache and reuse the exact selected DH group via the handshake to keep wire-group, exchange hash, and key agreement consistent (updates
GetDHPrimeGroupto takeWOLFSSH*). - Extend internal/regression/unit tests to cover selection, skip-on-
first_packet_follows, send-vs-hash consistency, and cache-miss fallback.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
wolfssh/internal.h |
Exposes new internal-test wrappers for DH-GEX group selection, group handler, and DH prime-group retrieval. |
src/internal.c |
Implements DH-GEX group selection + 2048-bit floor, caches selected group for consistency, updates GetDHPrimeGroup signature, and adds ignoreNextKexMsg handling for DoKexDhGexGroup. |
tests/unit.c |
Adds unit tests for selection behavior, wire-vs-hash consistency, and cache-miss fallback. |
tests/regress.c |
Extends first_packet_follows skip regression coverage to include DoKexDhGexGroup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Clamp client's lower bound up to WOLFSSH_DH_GEX_MIN_BITS (default 2048) before the candidate scan; prevents downgrade to group 1. - Reject a window whose max is below the floor with WS_DH_SIZE_E. - Keep group 1 in the candidate set so a lowered floor stays usable. - Cap the client window to MAX_KEX_KEY_SZ so selection can never pick a group larger than the server's own key buffers (defense-in-depth for builds enabling group 16 with a lowered WOLFSSH_DEFAULT_GEXDH_MAX). - Relabel the no-candidate log "effective window" so the clamped min is not mistaken for the raw client request. - Note the client-side DoKexDhGexGroup ignoreNextKexMsg skip as defensive symmetry not expected to fire. - Update test_DhGexGroupSelect for rejection and clamp-up cases; add test_DhGexGroupCacheMissFallback covering the GetDHPrimeGroup cache-miss re-selection path. Issue: F-55
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 6 total — 5 posted, 1 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review] Server now rejects GEX windows it can't satisfy (behavior change vs. always-group14) —
src/internal.c:11847-11851 - [Low] [review] Send/hash consistency and cache-miss tests are compiled out on group14-only builds —
tests/unit.c:1246-1330 - [Low] [bugs] DH GEX selection tests ignore configurable floor —
tests/unit.c:1148-1209 - [Low] [review+review-security] New null checks use implicit-pointer form instead of != NULL —
src/internal.c:13941 - [Low] [review] 1024-bit group 1 candidate is unreachable (dead entry) —
src/internal.c:11802-11804
Skipped findings
- [Info]
Client can now steer server to a 4096-bit GEX group (modest CPU amplification)
Review generated by Skoll
| } | ||
| } | ||
|
|
||
| if (!haveBest) { |
There was a problem hiding this comment.
🟠 [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.
| * the 4096-bit group 16 and confirms the cached wire group and the group that | ||
| * GetDHPrimeGroup returns are one and the same -- the property the send/hash | ||
| * cache exists to guarantee. */ | ||
| static int test_DhGexGroupSendHashConsistency(void) |
There was a problem hiding this comment.
🔵 [Low] Send/hash consistency and cache-miss tests are compiled out on group14-only builds · Test coverage
Both test_DhGexGroupSendHashConsistency and test_DhGexGroupCacheMissFallback are entirely wrapped in #ifndef WOLFSSH_NO_DH_GROUP16_SHA512 and return 0 otherwise (verified: unit.c:1248/1318-1322 and 1330). Since group 16 (4096-bit) is disabled in many default configurations, the send-vs-hash cache consistency guarantee — the core property this PR adds — goes completely unexercised on those builds. The consistency is trivially true when only group 14 exists, so this is a reduced-coverage note rather than a correctness gap, but the wire-vs-hash divergence the cache exists to prevent is only tested when group 16 is present.
Fix: Consider a group14-only variant that still asserts GetDHPrimeGroup returns the exact cached handshake->primeGroup pointer/size after SendKexDhGexGroup, so the caching path is covered on default builds too.
| * 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); |
There was a problem hiding this comment.
🔵 [Low] DH GEX selection tests ignore configurable floor · Logic Errors
The PR adds WOLFSSH_DH_GEX_MIN_BITS as an overridable compile-time floor in src/internal.c, but the new unit tests assume that floor is always exactly 2048 bits and hard-code 256-byte group14 outcomes. With a valid non-default build such as -DWOLFSSH_DH_GEX_MIN_BITS=4096 and group16 enabled, wolfSSH_TestSelectKexDhGexGroup(1024, 3072, 8192, ...) correctly selects the 4096-bit group, but this test fails because it still expects groupSz == 256. With a lower override, the sub-2048 rejection/floor-clamp checks can similarly fail by allowing group1. This is introduced by the PR because it adds both the configurable floor and tests that do not derive expectations from it.
Fix: Make the floor visible to the tests and condition expected results on it, or only run the 2048-specific assertions when the floor is 2048. For example, move the default WOLFSSH_DH_GEX_MIN_BITS definition to a shared internal header, then guard these checks with #if WOLFSSH_DH_GEX_MIN_BITS == 2048 or compute the expected group size from the configured floor.
| * making the wire group the single source of truth instead of re-running | ||
| * the selection independently. */ | ||
| if (ret == WS_SUCCESS) { | ||
| if (ssh->handshake->primeGroup) |
There was a problem hiding this comment.
🔵 [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.
| const byte* group; | ||
| const word32* groupSz; | ||
| } candidates[] = { | ||
| #ifndef WOLFSSH_NO_DH_GROUP1_SHA1 |
There was a problem hiding this comment.
🔵 [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.
This fixes the server-side Diffie-Hellman Group Exchange (RFC 4419) group selection, which previously ignored the client's requested size window and always sent group 14, and adds a 2048-bit floor to prevent downgrade.