Validate server's group

[ejohnstown]

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.

[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]

🟠 [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]

🔵 [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]

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]

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]

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]

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]

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]

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]

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.