Enforce minimum DH group size on validate and derive - #462
Open
yosuke-wolfssl wants to merge 1 commit into
Open
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.
yosuke-wolfssl
force-pushed
the
fix/f_4695
branch
from
August 7, 2026 07:43
3c0eb1c to
1329595
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.