Skip to content

Enforce minimum DH group size on validate and derive - #462

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_4695
Open

Enforce minimum DH group size on validate and derive#462
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_4695

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

DH domain parameters imported through EVP_PKEY_fromdata or the DH/DHX parameter decoders were stored with no validation of the prime. WP_DH_MIN_BITS was 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's DH_check() flags DH_MODULUS_TOO_SMALL.
  • wp_dh_derive_secret() passed the group straight to wc_DhAgree. No released wolfSSL through v5.9.2 has a minimum-prime guard there, so a 256-bit group completes a full key agreement.
  • q was never checked at all: not primality, not that it divides p-1, not that g has order q.

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(), and wp_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:

Path Check
wp_dh_validate() prime size, plus q prime / q | p-1 / g^q ≡ 1 mod p
wp_dh_derive_secret() prime size, before wc_DhAgree

Both raise PROV_R_KEY_SIZE_TOO_SMALL, matching wp_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_rejected imports a 256-bit safe prime with g = 2 — well formed, so only its size can cause rejection — and asserts param_check and derive both fail while the 2048-bit group still works. test_dh_param_check_q drives explicit p/q/g through fromdata with a matching q plus three rejection cases.

The q | p-1 check is retained for consistency with SP 800-56A and OpenSSL, but cannot be isolated by a test: g^q ≡ 1 with prime q and g > 1 already implies it, so the order check rejects those inputs first. Noted in the test.

Verification

  • Build clean under -Werror. Full suite 200 passed, 0 failed. ASan + UBSan clean.
  • Both tests fail before the fix; a standalone probe confirmed a 256-bit derive produced the expected shared secret pre-fix and is rejected post-fix.
  • FIPS note: WP_DH_MIN_BITS is 2048 under HAVE_FIPS, so the derive gate rejects 1024-bit groups there. Matches wolfSSL's own FIPS DH_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 in wp_dh_validate and wp_dh_params_validate into a shared helper; they differ in p primality, the q guard, and g-range semantics.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_BITS for explicit-parameter DH validation (wp_dh_validate) and for DH derive (wp_dh_derive_secret).
  • Add explicit q consistency validation when q is present (primality, q | (p-1), and g^q ≡ 1 (mod p)).
  • Add unit tests to ensure weak groups are rejected and q validation 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 wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread test/test_dh.c Outdated
Comment thread test/test_dh.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants