Uh oh!
There was an error while loading. Please reload this page.
Enforce minimum DH group size on validate and derive - #462
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens DH handling in the provider by enforcing a minimum DH modulus size during validation and key derivation, closing the gap where undersized imported groups could still pass param_check and successfully derive a shared secret.
Changes:
- Enforce
WP_DH_MIN_BITSfor explicit-parameter DH validation (wp_dh_validate) and for DH derive (wp_dh_derive_secret). - Add explicit
qconsistency validation whenqis present (primality,q | (p-1), andg^q ≡ 1 (mod p)). - Add unit tests to ensure weak groups are rejected and
qvalidation behaves as expected.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit.h | Declares the two new DH unit tests. |
| test/unit.c | Registers the new DH unit tests in the test case table. |
| test/test_dh.c | Adds coverage for rejecting undersized imported groups and validating explicit p/q/g via EVP_PKEY_fromdata. |
| src/wp_dh_kmgmt.c | Adds minimum-modulus enforcement and q consistency checks during explicit DH parameter validation. |
| src/wp_dh_exch.c | Adds a minimum-modulus guard before calling wc_DhAgree to prevent deriving in undersized groups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #462
Scan targets checked:wolfprovider-bugs, wolfprovider-src
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Problem
DH domain parameters imported through
EVP_PKEY_fromdataor the DH/DHX parameter decoders were stored with no validation of the prime.WP_DH_MIN_BITSwas enforced only on the generation side, so the provider refused to generate a group below the minimum but would still validate and derive on one:EVP_PKEY_param_check()reported an undersized group as valid, where OpenSSL'sDH_check()flagsDH_MODULUS_TOO_SMALL.wp_dh_derive_secret()passed the group straight towc_DhAgree. No released wolfSSL through v5.9.2 has a minimum-prime guard there, so a 256-bit group completes a full key agreement.qwas never checked at all: not primality, not that it dividesp-1, not thatghas orderq.An application that imports DH parameters from an untrusted source and derives without checking them agrees a secret in a group small enough to solve by discrete log. Closes f-4695.
Not affected: TLS DHE at default settings. libssl gates on
EVP_PKEY_get_security_bits(), andwp_dh_get_security_bits()returns 0 below 1024 bits, so security level 1 rejects the group before any derive.Fix (
src/wp_dh_kmgmt.c)Enforce the floor where the crypto decision is made, not at import:
wp_dh_validate()qprime /q | p-1/g^q ≡ 1 mod pwp_dh_derive_secret()wc_DhAgreeBoth raise
PROV_R_KEY_SIZE_TOO_SMALL, matchingwp_dh_gen_set_params(). Import stays permissive so a weak parameter file still parses and prints, as it does with OpenSSL.Tests
test_dh_weak_group_rejectedimports a 256-bit safe prime withg = 2— well formed, so only its size can cause rejection — and assertsparam_checkandderiveboth fail while the 2048-bit group still works.test_dh_param_check_qdrives explicitp/q/gthroughfromdatawith a matchingqplus three rejection cases.The
q | p-1check is retained for consistency with SP 800-56A and OpenSSL, but cannot be isolated by a test:g^q ≡ 1with primeqandg > 1already implies it, so the order check rejects those inputs first. Noted in the test.Verification
-Werror. Full suite 200 passed, 0 failed. ASan + UBSan clean.WP_DH_MIN_BITSis 2048 underHAVE_FIPS, so the derive gate rejects 1024-bit groups there. Matches wolfSSL's own FIPSDH_MIN_SIZE.Not in this PR
A positive small-
q(FIPS 186-4 subgroup) accept vector — it needs ~68 lines of constants for one case. Factoring the overlapping checks inwp_dh_validateandwp_dh_params_validateinto a shared helper; they differ inpprimality, theqguard, and g-range semantics.