Validate server's group by ejohnstown · Pull Request #922 · wolfSSL/wolfssh

ejohnstown · 2026-04-15T17:11:15Z

The client wasn't validating the DH group parameters in the KEX DH GEX Group message. This adds a function to perform the validation of the prime p to verify it is safe. (Prime and that ((p - 1) / 2) is prime.) Also adds a test to a known unsafe prime and known safe prime to verify the validate function.

Affected function: DoKexDhGexGroup.
Issue: F-1688

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds client-side validation of server-provided DH GEX group parameters to reject unsafe primes (non-safe prime p) and out-of-range generators, addressing missing validation in DoKexDhGexGroup (Issue: F-1688).

Changes:

Added ValidateKexDhGexGroup() and invoked it from DoKexDhGexGroup().
Exposed an internal test hook wolfSSH_TestValidateKexDhGexGroup() behind test/feature guards.
Added a unit test that generates a prime p that is prime but not a safe prime and asserts rejection.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
wolfssh/internal.h	Adds internal test API prototype for DH GEX group validation behind feature guards.
src/internal.c	Implements DH GEX group validation (safe-prime + generator bounds) and integrates it into KEX processing; adds test-only wrapper.
tests/unit.c	Adds unit test to generate a non-safe prime and verify the validator rejects it.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #922

Scan targets checked: wolfssh-bugs, wolfssh-compliance, wolfssh-consttime, wolfssh-defaults, wolfssh-mutation, wolfssh-proptest, wolfssh-src, wolfssh-zeroize

Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The client wasn't validating the DH group parameters in the KEX DH GEX Group message. This adds a function to perform the validation of the prime `p` to verify it is safe. (Prime and that ((p - 1) / 2) is prime.) Also adds a test to a known unsafe prime and known safe prime to verify the validate function. Affected function: DoKexDhGexGroup. Issue: F-1688

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #922

Scan targets checked: wolfssh-bugs, wolfssh-compliance, wolfssh-consttime, wolfssh-defaults, wolfssh-mutation, wolfssh-proptest, wolfssh-src, wolfssh-zeroize

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

wolfSSL-Fenrir-bot · 2026-04-16T00:52:42Z

+        }
+    }
+
+    /* Miller-Rabin: p must be prime. */
+    if (ret == WS_SUCCESS) {
+        isPrime = MP_NO;
+        ret = mp_prime_is_prime_ex(&p, WOLFSSH_MR_ROUNDS, &isPrime, rng);
+        if (ret != MP_OKAY || isPrime == MP_NO) {
+            WLOG(WS_LOG_DEBUG, "DH GEX: p is not prime");
+            ret = WS_CRYPTO_FAILED;
+        }
+        else {
+            ret = WS_SUCCESS;
+        }
+    }
+
+    /* Safe prime check: q = (p - 1) / 2 must also be prime. */
+    if (ret == WS_SUCCESS) {


🟠 [Medium] Generator bounds checks survive deletion mutation — no test exercises g < 2 or g >= p-1
Category: Surviving deletion mutations

Both test cases in test_DhGexGroupValidate use generator = 2, which trivially passes both generator bounds checks (mp_cmp_d(&g, 1) != MP_GT and mp_cmp(&g, &q) != MP_LT). A mutation that deletes the entire generator validation block (lines 6390-6407) would not be detected by any test. This is significant because g = 1 (trivial subgroup) and g = p-1 (order-2 subgroup) are cryptographically dangerous values that an attacker-controlled server could supply.

/* 2 <= g: reject g == 0 and g == 1. */ if (ret == WS_SUCCESS) { if (mp_cmp_d(&g, 1) != MP_GT) { WLOG(WS_LOG_DEBUG, "DH GEX: generator < 2"); ret = WS_CRYPTO_FAILED; } } /* g <= p - 2: reject g == p - 1 (order 2) and anything larger. */ if (ret == WS_SUCCESS) { if (mp_sub_d(&p, 1, &q) != MP_OKAY) ret = WS_CRYPTO_FAILED; else if (mp_cmp(&g, &q) != MP_LT) { WLOG(WS_LOG_DEBUG, "DH GEX: generator >= p - 1"); ret = WS_CRYPTO_FAILED; } }

Recommendation: Add test cases that call wolfSSH_TestValidateKexDhGexGroup with generator = 1 (expect WS_CRYPTO_FAILED) and with generator set to safePrime - 1 (expect WS_CRYPTO_FAILED) to cover both bounds.

wolfSSL-Fenrir-bot · 2026-04-16T00:52:43Z

+                ret, WS_CRYPTO_FAILED);
+        result = -121;
+    }
+
+    ret = wolfSSH_TestValidateKexDhGexGroup(safe, safeSz, &generator, 1,
+            1024, 8192, &rng);
+    if (ret != WS_SUCCESS) {


🔵 [Low] Test checks only return code, not which validation stage rejected the prime
Category: Weak test assertions

The unsafe-prime test asserts ret != WS_CRYPTO_FAILED but cannot distinguish whether the rejection came from the Miller-Rabin check on p, the safe-prime check on (p-1)/2, or a generator check. Since the GOST prime is actually prime (passes Miller-Rabin) but not a safe prime, the test relies on the rejection coming from the (p-1)/2 check. However, if the Miller-Rabin primality check on p were deleted (a mutation), the function would still reach and fail on the safe-prime check, meaning the deletion would go undetected.

ret = wolfSSH_TestValidateKexDhGexGroup(unsafe, unsafeSz, &generator, 1, 512, 8192, &rng); if (ret != WS_CRYPTO_FAILED) { printf("DhGexGroupValidate: validator returned %d, expected %d\n", ret, WS_CRYPTO_FAILED); result = -121; }

Recommendation: Add a test with a prime that is not prime at all (a known composite) to specifically exercise the Miller-Rabin check on p independently of the safe-prime check. This way, deleting the p primality check would cause the composite to reach later checks and produce a different outcome.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T00:53:58Z

+ * Validate a DH GEX group received from the server per RFC 4419 sec. 3:
+ *   - p's bit length falls within the client's requested [minBits, maxBits]
+ *   - p is odd and probably prime
+ *   - (p-1)/2 is also probably prime, i.e. p is a safe prime, which bounds
+ *     the order of g to q or 2q and closes the small-subgroup attack


The comment claims the validation is “per RFC 4419 sec. 3”, but the newly added safe-prime requirement ((p-1)/2 is prime) is stricter than RFC 4419’s requirements and can change interoperability. Update the comment to explicitly state this is an additional hardening requirement (or note it is policy-driven), so behavior differences are clear to maintainers and downstream consumers.

Suggested change

* Validate a DH GEX group received from the server per RFC 4419 sec. 3:

* - p's bit length falls within the client's requested [minBits, maxBits]

* - p is odd and probably prime

* - (p-1)/2 is also probably prime, i.e. p is a safe prime, which bounds

* the order of g to q or 2q and closes the small-subgroup attack

* Validate a DH GEX group received from the server against RFC 4419 sec. 3

* requirements, with an additional hardening/policy check:

* - p's bit length falls within the client's requested [minBits, maxBits]

* - p is odd and probably prime

* - (additional hardening requirement) (p-1)/2 is also probably prime,

* i.e. p is a safe prime, which bounds the order of g to q or 2q and

* closes the small-subgroup attack

Copilot · 2026-04-16T00:53:59Z

+    /* Miller-Rabin: p must be prime. */
+    if (ret == WS_SUCCESS) {
+        isPrime = MP_NO;
+        ret = mp_prime_is_prime_ex(&p, WOLFSSH_MR_ROUNDS, &isPrime, rng);


Two probabilistic primality tests at WOLFSSH_MR_ROUNDS=64 during handshake can be expensive for large GEX groups (especially near the upper bound) and may introduce noticeable latency on constrained systems. Consider (mandatory if this impacts supported targets): (1) selecting rounds based on bit length (security-equivalent but faster for smaller sizes), and/or (2) making rounds a tunable runtime/compile-time policy with project-documented defaults and guidance for embedded builds.

Copilot · 2026-04-16T00:53:59Z

+    }
+    if (ret == WS_SUCCESS) {
+        isPrime = MP_NO;
+        ret = mp_prime_is_prime_ex(&q, WOLFSSH_MR_ROUNDS, &isPrime, rng);


Two probabilistic primality tests at WOLFSSH_MR_ROUNDS=64 during handshake can be expensive for large GEX groups (especially near the upper bound) and may introduce noticeable latency on constrained systems. Consider (mandatory if this impacts supported targets): (1) selecting rounds based on bit length (security-equivalent but faster for smaller sizes), and/or (2) making rounds a tunable runtime/compile-time policy with project-documented defaults and guidance for embedded builds.

Copilot · 2026-04-16T00:53:59Z

+    mp_int p;
+    mp_int g;
+    mp_int q;
+    int pInit = 0, gInit = 0, qInit = 0;


The manual *_Init flags and repeated mp_init() blocks make the function harder to extend safely. Prefer a consolidated initialization/cleanup pattern (e.g., a single init sequence with one cleanup: label, or mp_init_multi/mp_clear_multi if available in the project’s wolfCrypt version) to reduce branching and the chance of future leaks or double-clears.

Copilot · 2026-04-16T00:53:59Z

+        if (mp_init(&p) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+        else
+            pInit = 1;
+    }
+    if (ret == WS_SUCCESS) {
+        if (mp_init(&g) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+        else
+            gInit = 1;
+    }
+    if (ret == WS_SUCCESS) {
+        if (mp_init(&q) != MP_OKAY)
+            ret = WS_CRYPTO_FAILED;
+        else
+            qInit = 1;


The manual *_Init flags and repeated mp_init() blocks make the function harder to extend safely. Prefer a consolidated initialization/cleanup pattern (e.g., a single init sequence with one cleanup: label, or mp_init_multi/mp_clear_multi if available in the project’s wolfCrypt version) to reduce branching and the chance of future leaks or double-clears.

Suggested change

if (mp_init(&p) != MP_OKAY)

ret = WS_CRYPTO_FAILED;

else

pInit = 1;

}

if (ret == WS_SUCCESS) {

if (mp_init(&g) != MP_OKAY)

ret = WS_CRYPTO_FAILED;

else

gInit = 1;

}

if (ret == WS_SUCCESS) {

if (mp_init(&q) != MP_OKAY)

ret = WS_CRYPTO_FAILED;

else

qInit = 1;

if (mp_init(&p) != MP_OKAY) {

ret = WS_CRYPTO_FAILED;

}

else if (mp_init(&g) != MP_OKAY) {

mp_clear(&p);

ret = WS_CRYPTO_FAILED;

}

else if (mp_init(&q) != MP_OKAY) {

mp_clear(&g);

mp_clear(&p);

ret = WS_CRYPTO_FAILED;

}

else {

pInit = 1;

gInit = 1;

qInit = 1;

}

Copilot · 2026-04-16T00:53:59Z

+    if (qInit) mp_clear(&q);
+    if (gInit) mp_clear(&g);
+    if (pInit) mp_clear(&p);


The manual *_Init flags and repeated mp_init() blocks make the function harder to extend safely. Prefer a consolidated initialization/cleanup pattern (e.g., a single init sequence with one cleanup: label, or mp_init_multi/mp_clear_multi if available in the project’s wolfCrypt version) to reduce branching and the chance of future leaks or double-clears.

Copilot · 2026-04-16T00:54:00Z

+    ret = wolfSSH_TestValidateKexDhGexGroup(unsafe, unsafeSz, &generator, 1,
+            512, 8192, &rng);
+    if (ret != WS_CRYPTO_FAILED) {
+        printf("DhGexGroupValidate: validator returned %d, expected %d\n",
+                ret, WS_CRYPTO_FAILED);
+        result = -121;
+    }
+
+    ret = wolfSSH_TestValidateKexDhGexGroup(safe, safeSz, &generator, 1,
+            1024, 8192, &rng);
+    if (ret != WS_SUCCESS) {
+        printf("DhGexGroupValidate: validator returned %d, expected %d\n",
+                ret, WS_SUCCESS);
+        result = -122;
+    }


The test only covers “safe vs unsafe prime” but doesn’t exercise other newly added acceptance criteria (bit-length bounds, p oddness, 2 <= g <= p-2, and the (p-1)/2 check separately from p primality). Add targeted cases to ensure each validation branch returns the expected error (e.g., g=1, g=p-1, minBits/maxBits mismatch, and a composite p) so regressions are detected.

Copilot AI review requested due to automatic review settings April 15, 2026 17:11

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Comment thread wolfssh/internal.h Outdated

Comment thread src/internal.c

Comment thread src/internal.c

Comment thread src/internal.c

Comment thread tests/unit.c Outdated

Comment thread tests/unit.c Outdated

Comment thread tests/unit.c Outdated

ejohnstown force-pushed the check-dhgroup branch from 0de8587 to 2e92637 Compare April 15, 2026 18:49

ejohnstown requested a review from Copilot April 15, 2026 18:51

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Comment thread src/internal.c

Comment thread tests/unit.c Outdated

Comment thread wolfssh/internal.h Outdated

Copilot started reviewing on behalf of ejohnstown April 15, 2026 19:17 View session

Copilot started reviewing on behalf of ejohnstown April 15, 2026 20:40 View session

JacobBarthelmeh requested a review from wolfSSL-Fenrir-bot April 15, 2026 22:59

wolfSSL-Fenrir-bot reviewed Apr 15, 2026

View reviewed changes

Comment thread wolfssh/internal.h Outdated

Comment thread src/internal.c

Comment thread tests/unit.c

ejohnstown force-pushed the check-dhgroup branch from 2e92637 to 4613bca Compare April 15, 2026 23:58

Copilot AI review requested due to automatic review settings April 16, 2026 00:04

ejohnstown force-pushed the check-dhgroup branch from 4613bca to 218952b Compare April 16, 2026 00:04

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Comment thread tests/unit.c Outdated

Comment thread tests/unit.c

Comment thread tests/unit.c Outdated

Comment thread tests/unit.c Outdated

Comment thread src/internal.c

Comment thread src/internal.c

Comment thread tests/unit.c Outdated

Comment thread wolfssh/internal.h Outdated

Copilot started reviewing on behalf of ejohnstown April 16, 2026 00:12 View session

ejohnstown force-pushed the check-dhgroup branch from 218952b to 253f05b Compare April 16, 2026 00:22

Copilot AI review requested due to automatic review settings April 16, 2026 00:31

ejohnstown force-pushed the check-dhgroup branch from 253f05b to 49dbbba Compare April 16, 2026 00:31

ejohnstown requested a review from wolfSSL-Fenrir-bot April 16, 2026 00:32

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Comment thread tests/unit.c Outdated

ejohnstown force-pushed the check-dhgroup branch from 49dbbba to 20f3be1 Compare April 16, 2026 00:40

ejohnstown requested a review from Copilot April 16, 2026 00:44

Copilot started reviewing on behalf of ejohnstown April 16, 2026 00:51 View session

wolfSSL-Fenrir-bot reviewed Apr 16, 2026

View reviewed changes

Copilot AI reviewed Apr 16, 2026

View reviewed changes

Conversation

ejohnstown commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #922

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #922

Uh oh!

wolfSSL-Fenrir-bot Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Fenrir-bot Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

ejohnstown commented Apr 15, 2026 •

edited

Loading