DH GEX group selection hardening#1056
Open
ejohnstown wants to merge 4 commits into
Open
Conversation
Contributor
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.
aidangarske
reviewed
Jul 7, 2026
aidangarske
left a comment
aidangarske
left a comment
Member
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
- 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
- Clamp client's lower bound up to WOLFSSH_DH_GEX_MIN_BITS (default 2048) before the candidate scan; prevents downgrade to group 1. - Define WOLFSSH_DH_GEX_MIN_BITS in wolfssh/internal.h so both the build and the unit tests can see the configured floor. - 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, gating the 2048-specific expectations on the configured floor; add test_DhGexGroupCacheMissFallback covering the GetDHPrimeGroup cache-miss re-selection path. Issue: F-55
The floor added in 1d2d2a0 only ran on the server's group selection. A wolfSSH client would still accept whatever group a server handed back, and would advertise a minimum it did not itself enforce. - Raise WOLFSSH_DEFAULT_GEXDH_MIN 1024 -> 2048 and enforce WOLFSSH_DH_GEX_MIN_BITS in DoKexDhGexGroup, so a server cannot hand the client a weak group by ignoring the requested min. - Clamp the advertised min up to the floor in SendKexDhGexRequest, and pull preferred up with it so the advertised triple stays min <= preferred. Reject with WS_DH_SIZE_E when the configured max is below the floor, rather than emitting a request whose only valid replies the accept path would refuse. Previously a lowered WOLFSSH_DEFAULT_GEXDH_MIN advertised a min DoKexDhGexGroup rejected. - Cache the selected group on the handshake in SendKexDhGexGroup and reuse it from GetDHPrimeGroup, so the group on the wire, the exchange hash, and the shared secret cannot diverge. - Move KEX_F_SIZE to wolfssh/internal.h behind an #ifndef guard, so SelectKexDhGexGroup and the unit tests can bound the window by the server's f buffer. - Add wolfSSH_TestSendKexDhGexRequest and wolfSSH_TestGetDHPrimeGroup wrappers; cover the request clamp, the sub-floor rejection, the send/hash consistency of the selected group, and a real 4096-bit group 16 key agreement. Issue: F-55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.