Skip to content

tests: exercise DoKexDhReply host-key signature verify with corrupted signatures#1051

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

tests: exercise DoKexDhReply host-key signature verify with corrupted signatures#1051
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_6272

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

tests: exercise DoKexDhReply host-key signature verification with corrupted signatures

Summary

DoKexDhReply (client side) authenticates the server by verifying its
signature over the exchange hash ssh->h with the negotiated host key — RSA
(wc_SignatureVerify), ECC (wc_SignatureVerify), and Ed25519
(wc_ed25519_verify_msg). None of these verify calls was reached by any
negative test:

  • RewriteKexDhReplySignatureName rewrote only the signature name string,
    which is rejected earlier at the name-match check (WS_PARSE_E).
  • TestKexDhReplyRejectsNoPublicKeyCheck / …WhenCallbackRejects fail at the
    public-key-check callback, also before the crypto verify.

As a result, mutation testing showed the verify could be deleted without any
test failing — i.e. an attacker who knows only the server's public key could
impersonate the server / MITM the connection, and CI would stay green.

This PR adds negative regression tests that corrupt the signature data
(not the name) so the client reaches the actual cryptographic verify and must
reject the handshake. Test-only change — no production code is modified;
the verify logic in src/internal.c was already correct.

Addressed by f_6272.

What changed (tests/regress.c)

  • Added a second mode to the existing duplex KEXDH_REPLY mutator
    (REGRESS_MUTATE_SIG_DATA) that keeps the signature name and flips one byte
    of the signature data. The name-rewrite and data-corrupt paths now share a
    single parametrized helper pair (RewriteSingleKexDhReplyPacket /
    RewriteKexDhReplyPacket) instead of duplicating the packet parse/locate
    logic.
  • Parametrized the test harness with a host-key path so ECC and Ed25519 keys
    can be loaded (it previously hardcoded the RSA key).
  • New tests, each paired with a positive control and guarded by the relevant
    feature macros:
    • TestKexDhReplyRejectsRsaSha2_256CorruptSig / …512WS_RSA_E
    • TestKexDhReplyRejectsEccCorruptSig (nistp256) → WS_ECC_E
    • TestKexDhReplyRejectsEd25519CorruptSigWS_ED25519_E

Testing

  • tests/regress.test passes.
  • Confirmed each new test fails when its verify is temporarily neutered
    (deleted the RSA/ECC wc_SignatureVerify assignment; bypassed the Ed25519
    branch) — the negative controls prove the tests have teeth.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 23, 2026
Copilot AI review requested due to automatic review settings June 23, 2026 04:36

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.

Pull request overview

This PR strengthens wolfSSH’s regression test suite by ensuring the client-side DoKexDhReply path actually reaches and exercises the cryptographic host-key signature verification (RSA/ECC/Ed25519) under tampering, preventing a class of “verify removed but tests still pass” regressions.

Changes:

  • Extended the existing duplex KEXDH_REPLY mutator to support a new mode that corrupts signature data (while keeping the signature name valid) so the handshake reaches the real verify call.
  • Parameterized the KEXDH_REPLY harness to load different server host keys (RSA/ECC/Ed25519) via a passed-in key path.
  • Added new negative tests (with positive controls) validating rejection/error codes for corrupted signatures across RSA-SHA2-256/512, ECDSA nistp256, and Ed25519.

💡 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 #1051

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