Enforce DH minimum prime size on the parameters-only generation path#427
Enforce DH minimum prime size on the parameters-only generation path#427yosuke-wolfssl wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR closes a security gap in the DH keymgmt generation path where parameters-only generation could bypass the provider’s minimum prime-size enforcement, particularly impacting FIPS builds.
Changes:
- Enforce
WP_DH_MIN_BITSfor DH parameter generation across all selection paths by validating generated/copied parameters before any keypair generation. - Fail fast in
wp_dh_gen_set_paramswhen a caller requestsOSSL_PKEY_PARAM_FFC_PBITSbelowWP_DH_MIN_BITS. - Add a new unit test to assert below-minimum DH prime sizes are rejected while the minimum size still succeeds (with FIPS/non-FIPS thresholds).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/unit.h | Registers the new DH minimum-prime-size test prototype. |
| test/unit.c | Adds the new DH minimum-prime-size test to the unit test table. |
| test/test_dh.c | Implements test_dh_pgen_min_bits to exercise set-params rejection and minimum-size success. |
| src/wp_dh_kmgmt.c | Enforces minimum DH prime size in set-params and ensures parameter validation runs for parameters-only generation as well. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5d1f266 to
06af890
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #427
Scan targets checked: wolfprovider-bugs, wolfprovider-src
No new issues found in the changed files. ✅
06af890 to
ffb3fe2
Compare
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 1 total — 1 posted, 0 skipped
Posted findings
- [Medium] Regression test does not exercise relocated generation-time validation —
test/test_dh.c:1355-1368
Review generated by Skoll.
|
|
||
| PRINT_MSG("Testing DH parameter generation minimum prime size enforcement"); | ||
|
|
||
| /* Below-minimum size must be rejected at set-params time. */ |
There was a problem hiding this comment.
🟡 [Medium] Regression test does not exercise relocated generation-time validation
💡 SUGGEST test
The PR moves wp_dh_params_validate(dh) so generated or copied parameters are validated in every selection path, but the new weak-size test stops at EVP_PKEY_CTX_set_dh_paramgen_prime_len(). With the fail-fast guard in wp_dh_gen_set_params() present, this test would still pass if the relocated validation in wp_dh_gen() were accidentally reverted. That leaves the main behavioral change under-tested, especially for the caller pattern described in the PR where a caller ignores the set-params return and continues to parameter generation.
Recommendation: Keep the existing fail-fast assertion, but add a second assertion covering generation-time rejection so the test fails if the wp_dh_gen() validation move is removed. Add a FIPS-specific check that deliberately continues after the below-minimum set_dh_paramgen_prime_len() failure and asserts EVP_PKEY_paramgen() still fails, exercising the relocated wp_dh_params_validate() path for 1024-bit DH in FIPS.
Summary
DH parameter generation could produce keys weaker than the provider's own declared minimum on the parameters-only path.
wp_dh_params_validate— which enforcesWP_DH_MIN_BITS— was only invoked whenOSSL_KEYMGMT_SELECT_KEYPAIRwas set. A caller requesting onlyOSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS(parameter-only generation) could therefore have the provider generate DH parameters below the minimum with no check, andwp_dh_gen_set_paramsreadOSSL_PKEY_PARAM_FFC_PBITSintoctx->bitswith no range check.This is most impactful in FIPS builds:
WP_DH_MIN_BITSis 2048, but wolfCrypt's ownwc_DhGenerateParamsfloor is 1024, so sub-2048 parameters could be generated unchecked. In non-FIPS builds wolfCrypt's floor (1024) already equalsWP_DH_MIN_BITS, so there the change is defense-in-depth.Changes
wp_dh_gen: runwp_dh_params_validateafter parameters are generated or copied, in every selection path, before key pair generation (previously only under the keypair branch).wp_dh_gen_set_params: reject anFFC_PBITSrequest belowWP_DH_MIN_BITSat set-params time (fail-fast for callers that check the return).test_dh_pgen_min_bits(new): asserts a below-minimum prime length is rejected and a request at exactly the minimum still succeeds. Gated onHAVE_FIPSto mirrorWP_DH_MIN_BITS(1024 non-FIPS / 2048 FIPS).Testing
test_dh_pgen_min_bits.Notes
WP_DH_MIN_BITSis left at 1024 for non-FIPS to avoid breaking interop with legitimate 1024-bit DH consumers. Raising it to 2048 (per NIST SP 800-131A / Logjam) is a separate policy decision and not included here.wp_dh_params_validateis the actual enforcer; OpenSSL buffersprime_lenand the set-params guard is a secondary fail-fast.