Skip to content

Validate peer ECDH public key before shared-secret in key agreement#1061

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

Validate peer ECDH public key before shared-secret in key agreement#1061
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_5691

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Summary

Adds an explicit wc_ecc_check_key() validation of the peer-supplied ECDH public value in the server-side key-agreement paths, mirroring the public-value validation already performed for Curve25519.

  • KeyAgreeEcdh_server (pure ECDH: ecdh-sha2-nistp256/384/521)
  • KeyAgreeEcdhMlKem_server (ECC branch of the hybrid ML-KEM kex)

In both, the client's e value is imported with wc_ecc_import_x963_ex() and then fed to wc_ecc_shared_secret(). The new check sits between them and returns the wolfCrypt error via the existing ret chain on failure.

Addressed by f_5691.

Background / scope

This was raised as a missing-public-value-validation issue (potential pre-auth degenerate/off-curve point injection). On investigation, the reported severity is overstated for current wolfSSL:

  • wc_ecc_import_x963_ex() imports as untrusted (wc_ecc_import_x963_ex2(..., untrusted=1)) and already rejects the point at infinity and off-curve points via sp_ecc_is_point_* / wc_ecc_point_is_on_curve, independent of WOLFSSL_VALIDATE_ECC_IMPORT.
  • Verified empirically: an off-curve P-256 point (generator with the last Y byte flipped) is rejected at import with -98 (MP_VAL), before wc_ecc_shared_secret() is reached.
  • The NIST curves used here are prime-order (cofactor 1), so on-curve + not-infinity already implies the correct subgroup — wc_ecc_check_key() adds no exploitable protection over the existing import checks.

This change is therefore defense-in-depth / hardening, not a behavior-changing vulnerability fix. It pins the explicit-validation contract at the call site regardless of the underlying wolfCrypt import path, and matches the style of the sibling KeyAgreeCurve25519_server (which validates with wc_curve25519_check_public() — a check that genuinely matters there since Curve25519 has cofactor 8).

No regression test is included: for these prime-order curves any off-curve input is already rejected by the import, so such a test would pass with or without this change (equivalent mutant) and give false confidence.

Changes

/* KeyAgreeEcdh_server */
    if (ret == 0)
        ret = wc_ecc_import_x963_ex(ssh->handshake->e,
                                    ssh->handshake->eSz,
                                    pubKey, primeId);

    if (ret == 0)
        ret = wc_ecc_check_key(pubKey);

    if (ret == 0)
        ret = wc_ecc_make_key_ex(ssh->rng, ...);

The same if (ret == 0) ret = wc_ecc_check_key(pubKey); is added after the import in the ECC branch of KeyAgreeEcdhMlKem_server.

Testing

  • ./configure --enable-all build: clean.
  • unit.test and kex.test: pass — real ECDH and hybrid ML-KEM handshakes still succeed, confirming legitimate peers are not rejected.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 26, 2026
Copilot AI review requested due to automatic review settings June 26, 2026 02:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

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