Skip to content

fix: validate DH GEX request bounds #1089

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
MarkAtwood wants to merge 1 commit into
base: master
Choose a base branch
from
+10 −0
Open

fix: validate DH GEX request bounds #1089

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7253,6 +7253,16 @@ static int DoKexDhGexRequest(WOLFSSH* ssh,
ret = GetUint32(&ssh->handshake->dhGexMaxSz, buf, len, &begin);
}

if (ret == WS_SUCCESS) {

@aidangarske aidangarske Jul 10, 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.

🟠 [High] DH GEX validation accepts ranges the server cannot satisfy
🚫 BLOCK bug

The new validation accepts any ordered request inside WOLFSSH_DEFAULT_GEXDH_MIN..WOLFSSH_DEFAULT_GEXDH_MAX, then immediately calls SendKexDhGexGroup(). That send path always emits dhPrimeGroup14, a 2048-bit group. A request such as min=3072, preferred=3072, max=4096 passes the new check but cannot be satisfied by the current server implementation, so the server still sends a group outside the client's requested range. The PR-added validation gate should match what this path can actually produce, or select a supported group within the requested range.

Recommendation: Reject unsatisfiable ranges such as dhGexMinSz > 2048 or dhGexMaxSz < 2048 while the GEX server path is hard-coded to group14, or implement group selection before sending.

@aidangarske aidangarske Jul 10, 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] New DH GEX request validation has no regression coverage
💡 SUGGEST test

This PR changes externally visible server behavior by rejecting malformed DH GEX request sizes, but no tests were added. The only existing wolfSSH_TestDoKexDhGexRequest coverage found is the first-packet-follows skip case, which intentionally avoids parsing the payload and does not exercise the new validation block.

Recommendation: Cover the new rejection paths through wolfSSH_TestDoKexDhGexRequest() or an integration-style server handshake test for min < 1024, max > 8192, min > preferred, preferred > max, and at least one accepted boundary case.

if (ssh->handshake->dhGexMinSz < WOLFSSH_DEFAULT_GEXDH_MIN ||
ssh->handshake->dhGexMaxSz > WOLFSSH_DEFAULT_GEXDH_MAX ||
ssh->handshake->dhGexMinSz > ssh->handshake->dhGexPreferredSz ||
ssh->handshake->dhGexPreferredSz > ssh->handshake->dhGexMaxSz) {
WLOG(WS_LOG_DEBUG, "Invalid DH GEX request parameters");
ret = WS_BAD_ARGUMENT;
}
Comment on lines +7256 to +7263
}
Comment on lines +7256 to +7264

if (ret == WS_SUCCESS) {
WLOG(WS_LOG_INFO, " min = %u, preferred = %u, max = %u",
ssh->handshake->dhGexMinSz,
Expand Down
Loading