Skip to content

wp_dh_kmgmt: pass the full generator length to wc_DhAgree - #484

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

wp_dh_kmgmt: pass the full generator length to wc_DhAgree#484
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_12484

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

wp_dh_decode_pki() rebuilds a DH public key from the decoded private value by
calling wc_DhAgree() with the generator as the "peer public key". It
serialized the whole generator into base but passed a hardcoded length of 1:

rc = wc_DhAgree(&dh->key, dh->pub, &idx, dh->priv, (word32)dh->privSz,
    base, 1);

For any generator wider than one byte, only its first byte was used, so the
decoder produced base[0] ^ priv mod p instead of g ^ priv mod p. It failed
silentlywc_DhAgree()'s only check on that argument is 2 <= y <= p-2,
which a truncated first byte normally satisfies — and dh->pub is what the
keymgmt exports for OSSL_PKEY_PARAM_PUB_KEY. f-12484.

Reachable with stock OpenSSL, no exotic input:

openssl genpkey -genparam -algorithm DH -pkeyopt group:dh_2048_224 -out p.pem
openssl genpkey -paramfile p.pem -out k.pem

Fix (src/wp_dh_kmgmt.c)

  • baseSz holds the generator's encoded size and is passed to
    wc_DhAgree() as the peer public key length; a non-positive size fails the
    decode.
  • pubLen is a dedicated output-capacity variable for dh->pub, replacing
    the reuse of idx, which is the wc_DhKeyDecode() output index. dh->pubSz
    now takes pubLen.

The wc_DhAgree() call is retained so the FIPS DH primitive-Z CAST still fires
from this path. wp_dh_decode_spki() is unaffected — it copies dh->key.pub
directly, and this is the only site in the tree deriving pub from priv.

Tests (test/test_dh.c)

test_dh_decode_big_g decodes a PKCS#8 key on the RFC 5114 dh_2048_224 group
through both OpenSSL and wolfProvider and compares p, g, priv, pub. The
body of test_dh_decode() moved into a shared test_dh_decode_der() helper.

Verification

  • Negative control: the new test fails on the public-key compare before the fix
    and passes after; the existing g = 0x02 vector passes throughout.
  • Full suite: 225/225 passed. Build clean, no warnings in the touched files.
  • ASan + UBSan: all 15 DH tests clean.

- wp_dh_decode_pki() stores the generator's encoded size in baseSz,
  fails when it is not positive, allocates base with it and passes
  it as the peer public key length.
- A pubLen variable holds the dh->pub allocation size and carries
  the agreed length out of wc_DhAgree(); dh->pubSz takes its value.
  idx stays the wc_DhKeyDecode() output index.
- test_dh.c adds dh_big_g_der, a PKCS#8 key on the RFC 5114
  dh_2048_224 group, and test_dh_decode_big_g. The body of
  test_dh_decode() moves into test_dh_decode_der(), which both
  tests call, and reports a public key mismatch on stderr.

Issue: F-12484
@yosuke-wolfssl yosuke-wolfssl self-assigned this Sep 3, 2026
Copilot AI lite review requested due to automatic review settings September 3, 2026 02:34

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.

🟡 Changes recommended

wp_dh_decode_pki() casts mp_unsigned_bin_size(&dh->key.p) to word32 without validating the size, which can underflow and trigger an unexpectedly large allocation attempt.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes DH PKCS#8 decode behavior where the public key is re-derived from the private key using only the first byte of a multi-byte generator, causing silently incorrect exported public keys for named groups like RFC 5114 dh_2048_224.

Changes:

  • Pass the full encoded generator length to wc_DhAgree() when rebuilding the public key in wp_dh_decode_pki().
  • Separate DH decode index tracking from the wc_DhAgree() output-length parameter via a dedicated pubLen.
  • Add a regression test vector using a DH group with a multi-byte generator and refactor decode testing into a shared helper.
File summaries
File Description
test/unit.h Declares the new test_dh_decode_big_g unit test.
test/unit.c Registers test_dh_decode_big_g in the test suite.
test/test_dh.c Adds a multi-byte-generator PKCS#8 DER vector and a shared decode helper used by both decode tests.
src/wp_dh_kmgmt.c Fixes DH public-key re-derivation to use the full generator and a proper output length variable.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/wp_dh_kmgmt.c

@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 #484

Scan targets checked: wolfprovider-bugs, wolfprovider-src

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

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.

4 participants