dh: reject generation controls wolfSSL cannot apply - #486
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new DH control test can produce false-negatives by not asserting that setting a valid generator succeeds before expecting paramgen to fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the DH generation API contract by rejecting DH generation controls (DH_GENERATOR, DH_PRIV_LEN) that wolfSSL cannot actually honor, and adds unit coverage to ensure callers see deterministic failures instead of silent backend defaults.
Changes:
- Reject non-zero
DH_PRIV_LENat set-params time withPROV_R_NOT_SUPPORTED. - Validate and enforce
DH_GENERATORbehavior: reject invalid (<2), reject during parameter generation, and only accept during keygen when it matches the group/template generator. - Add unit tests covering generator/priv_len behavior across paramgen and keygen flows.
File summaries
| File | Description |
|---|---|
| test/unit.h | Registers the new DH control test declaration. |
| test/unit.c | Adds the new test case to the unit test table. |
| test/test_dh.c | Adds coverage for DH generator and private-length controls across paramgen/keygen. |
| src/wp_dh_kmgmt.c | Enforces rejection/acceptance rules for unsupported DH generation controls. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5d3bc7f to
467e17f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #486
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
467e17f to
0a6576d
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #486
Scan targets checked: wolfprovider-bugs, wolfprovider-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
- wp_dh_gen_set_params() reads DH_PRIV_LEN and DH_GENERATOR into locals: a non-zero length fails with PROV_R_NOT_SUPPORTED, a generator below 2 with PROV_R_INVALID_DATA, and ctx->generator is assigned only after that check. - wp_dh_gen_parameters() fails before wc_DhGenerateParams() when a generator other than 2 was requested; wp_dh_gen_copy_parameters() fails when mp_cmp_d() against the group's g is not equal. - wp_DhGenCtx drops privLen, and generator holds the caller's request with 0 meaning none, so wp_dh_gen_init() no longer sets it to 2. - README.md describes the DH generator and private key length requests that are refused. - test_dh_pgen_controls covers both controls over parameter and key generation, running the generator cases through the group-name and the parameters-key path. Issue: F-12485
0a6576d to
736272f
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #486
Scan targets checked: wolfprovider-src, wolfprovider-bugs
Coverage: 1 of 3 in-scope changed file(s) opened by the reviewer; not opened: test/test_dh.c, test/unit.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
Problem
The DH generation context accepted
DH_GENERATORandDH_PRIV_LENbut neverread them, so a caller asking for a generator or a private-key length got
success and backend-default output. wolfCrypt cannot apply either:
wc_DhGenerateParams()derivesgitself, andwc_DhGenerateKeyPair()takesthe private length from the group. Reachable from
openssl dhparam -5,genpkey -pkeyopt priv_len:NandEVP_PKEY_CTX_set_params(). An API contractviolation, not a crypto weakness. Closes 12485.
Fix (
src/wp_dh_kmgmt.c)priv_lennon-zeroPROV_R_NOT_SUPPORTEDat set-params (0 accepted)PROV_R_INVALID_DATAat set-paramsPROV_R_NOT_SUPPORTEDgmatchesBoth parameters stay in the settable list, so
-pkeyoptand theEVP_PKEY_CTX_set_dh_*helpers reach the provider and report its error.README.mddocuments the refused requests under DH.Limitation:
openssl dhparamsends generator 2 whenever the user namesnone, so 2 is accepted to keep
openssl dhparam 2048working, but wolfSSLstill derives its own
g.Tests
test_dh_pgen_controlscovers both controls over parameter and keygeneration, including the group-name and parameters-key paths.
Verification
test_dh_pgen_controlsfails on master, where generator 5 andpriv_len:256are accepted and ignored.openssl dhparam 2048works;dhparam -5fails withnot supported.