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
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() against the group's g is not equal, and its doxygen block names that failure. - 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. - 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
467e17f to
0a6576d
Compare
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.
Problem
The DH generation interface advertises and accepts
DH_GENERATORandDH_PRIV_LEN, stores them inwp_DhGenCtx, and never reads them — the structcomments said "value ignored" outright. wolfCrypt has no landing spot for
either:
wc_DhGenerateParams()takes only a modulus size and derivesgitself, and
wc_DhGenerateKeyPair()derives the private length fromq(orfrom the size of
p). A caller asking for a generator or a private-key lengththerefore got success and backend-default output. Reachable from
openssl genpkey -pkeyopt dh_paramgen_generator:N/-pkeyopt priv_len:Nandfrom
EVP_PKEY_CTX_set_params(). Closes 12485.This is an API contract violation, not a crypto weakness: the generated
gis avalid generator of the order-
qsubgroup, and an ignoredpriv_lenyields alonger private key, never a shorter one.
Fix (
src/wp_dh_kmgmt.c)Requests that cannot be applied now fail; requests that are already satisfied
still succeed.
priv_lennon-zeroPROV_R_NOT_SUPPORTEDat set-paramspriv_len0PROV_R_INVALID_DATAat set-paramsPROV_R_NOT_SUPPORTEDbefore the prime searchg, elsePROV_R_NOT_SUPPORTEDBoth parameters stay in
gen_settable_params()so the provider's own errorreaches the caller: delisting would make OpenSSL return
-2before calling us,with nothing on the error queue, while a direct
EVP_PKEY_CTX_set_params()bypasses the settable list entirely and would be silently ignored again.
Known limitation: generator 2 is tolerated, not applied
apps/dhparam.csetsg = 2whenever the caller passes none of-2/-3/-5, andalways forwards it, so
openssl dhparam 2048andopenssl dhparam -2 2048areindistinguishable to a provider. Refusing 2 broke every
dhparamgenerationrun, so it is accepted — but wolfSSL still derives its own generator, and the
parameters come back with a full-size
g, not 2. Values that can only have comefrom the user (3, 5) are refused rather than silently replaced.
Tests
test_dh_pgen_controlscovers both controls over parameter and key generation:generator 5 refused by paramgen and 2 accepted, generator 1 refused when set,
generator 2 and 5 accepted/refused against both a named group and a parameters
key, and
priv_len256 refused / 0 accepted.Verification
openssl dhparam 2048and-2 2048generate parameters;-3and-5fail with
wp_dh_gen_parameters:not supported;genpkey -genparamunchanged.