Validate server's group#922
Conversation
There was a problem hiding this comment.
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 fromDoKexDhGexGroup(). - Exposed an internal test hook
wolfSSH_TestValidateKexDhGexGroup()behind test/feature guards. - Added a unit test that generates a prime
pthat 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.
ejohnstown
force-pushed
the
check-dhgroup
branch
from
0de8587 to
2e92637
Compare
There was a problem hiding this comment.
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
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
ejohnstown
force-pushed
the
check-dhgroup
branch
from
2e92637 to
4613bca
Compare
ejohnstown
force-pushed
the
check-dhgroup
branch
from
4613bca to
218952b
Compare
There was a problem hiding this comment.
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.
ejohnstown
force-pushed
the
check-dhgroup
branch
from
218952b to
253f05b
Compare
ejohnstown
force-pushed
the
check-dhgroup
branch
from
253f05b to
49dbbba
Compare
There was a problem hiding this comment.
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.
ejohnstown
force-pushed
the
check-dhgroup
branch
from
49dbbba to
20f3be1
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ejohnstown
force-pushed
the
check-dhgroup
branch
from
20f3be1 to
bc06dce
Compare
ejohnstown
force-pushed
the
check-dhgroup
branch
from
bc06dce to
cd1b667
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ejohnstown
force-pushed
the
check-dhgroup
branch
from
cd1b667 to
841aed7
Compare
There was a problem hiding this comment.
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
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #922
Scan targets checked: wolfssh-bugs, wolfssh-compliance, wolfssh-consttime, wolfssh-defaults, wolfssh-mutation, wolfssh-proptest, wolfssh-src, wolfssh-zeroize
No new issues found in the changed files. ✅
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
ejohnstown
force-pushed
the
check-dhgroup
branch
from
841aed7 to
16ddcd9
Compare
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
pto 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