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

[yosuke-wolfssl]

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 / …512 → WS_RSA_E
  • TestKexDhReplyRejectsEccCorruptSig (nistp256) → WS_ECC_E
  • TestKexDhReplyRejectsEd25519CorruptSig → WS_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.

[Copilot]

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]

Fenrir Automated Review — PR #1051

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped

Posted findings

• [High] Ed25519 key fixture is not linked for out-of-tree test runs — tests/regress.c:388,1101-1106
• [Medium] Non-RSA host-key tests remain hidden behind the RSA build gate — tests/regress.c:332-333,1090-1106

Review generated by Skoll.

Superseded by corrected review (findings now attached as inline comments).

[yosuke-wolfssl]

Hello @aidangarske , @ejohnstown ,
I fixed the issues you mentioned. Could you please review this again ?

[aidangarske]

over to @ejohnstown

[aidangarske]

Skoll Code Review

Scan type: review-securityOverall recommendation: APPROVE
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Low] Signature-name WMEMCMP branch still unexercised: replacement name "ssh-rsa" only ever trips the length check — tests/regress.c:792-793
• [Low] New ECC/Ed25519 name-downgrade tests do not pin the rejection error code — tests/regress.c:1012-1028
• [Info] Multi-line block comments violate repository comment policy — tests/regress.c:575-577,666-670,1088-1090,1133-1134
• [Info] Positive-control handshake runs twice per algorithm, doubling test runtime — tests/regress.c:1117,1126,1137,1146

Review generated by Skoll

[aidangarske]

🔵 [Low] Signature-name WMEMCMP branch still unexercised: replacement name "ssh-rsa" only ever trips the length check · Missing Tests

Verified independently. The PR's stated purpose is closing mutation-testing gaps in DoKexDhReply. It closes the crypto-verify gap but leaves an adjacent one open, and the two new name-downgrade tests do not close it. DuplexSend hardcodes "ssh-rsa" (7 bytes) as the replacement signature name for every algorithm (regress.c:792-795; confirmed via git diff that this literal is introduced by this PR). DoKexDhReply checks the name in two steps at src/internal.c:7031-7040: first scratch != expectedSigNameSz (length), then WMEMCMP(sig + begin, expectedSigName, scratch) != 0 (content), joined by ||. Confirmed by reading src/internal.c:7032-7033 that the length comparison short-circuits. Because "ssh-rsa" is 7 bytes and every expected name is longer -- rsa-sha2-256/rsa-sha2-512 = 12, ecdsa-sha2-nistp256 = 19, ssh-ed25519 = 11 -- the length check returns WS_PARSE_E on every name test and the WMEMCMP is never reached. The pre-existing RSA tests had this gap; the PR adds TestKexDhReplyRejectsEccSigNameDowngrade and TestKexDhReplyRejectsEd25519SigNameDowngrade (regress.c:1012-1028), which reuse the same hardcoded replacement and therefore extend the gap to two more algorithms rather than closing it. The WMEMCMP could be deleted and all four tests would stay green. Confirmed that the only other callers of the SIG_NAME mutation path (AssertHandshakeRejectsWithNoPublicKeyCheck, regress.c:1036) pass mutation-enabled = 0, so no test anywhere reaches the WMEMCMP. A same-length replacement would reach it -- e.g. rsa-sha2-256 -> rsa-sha2-512 (12 vs 12)…

Fix: Plumb the replacement name through the harness the same way keyPath and mutateMode were plumbed, and add at least one same-length substitution (e.g. rsa-sha2-256 -> rsa-sha2-512) so the WMEMCMP at src/internal.c:7033 is actually reached and mutation-covered.

[yosuke-wolfssl]

Fixed. The replacement signature name is now plumbed through the harness (new replaceName field on KexReplyMutator, passed via InitKexReplyHarnessEx / AssertHandshakeRejectsMutatedReply) instead of the hardcoded "ssh-rsa". The name-downgrade tests now use same-length wrong names so the WMEMCMP at internal.c:7033 is actually reached: rsa-sha2-256 -> rsa-sha2-512, rsa-sha2-512 -> rsa-sha2-256, and ecdsa-sha2-nistp256 -> ecdsa-sha2-nistp384. I verified with a negative control: deleting the WMEMCMP (leaving only the length check) now fails the RSA-256 downgrade test, which it did not before this change.

[aidangarske]

🔵 [Low] New ECC/Ed25519 name-downgrade tests do not pin the rejection error code · Missing Tests

Verified independently. The PR adds AssertHandshakeRejectsCorruptedSig (regress.c:1091-1112) which pins the exact error with AssertIntEQ(result.clientErr, expectedErr) at line 1109 -- that is what gives the new corrupt-sig tests their teeth. The two new name-downgrade tests added in the same PR route through AssertHandshakeRejectsMutatedReply (regress.c:976-994), which only asserts clientRet == WS_FATAL_ERROR (line 990) and that the error is not WS_WANT_READ/WS_WANT_WRITE (line 991). Any rejection reason passes. Confirmed via git diff that this PR modifies that helper's signature (adding the keyPath parameter) without taking the opportunity to add an expectedErr parameter, while two sibling helpers in the same file (AssertHandshakeRejectsWithNoPublicKeyCheck at 1043 and AssertHandshakeRejectsWhenCallbackRejects at 1072) already do pin their error with AssertIntEQ(result.clientErr, WS_PUBKEY_REJECTED_E). If a future refactor made the client fail earlier for an unrelated reason (a parse bug returning WS_BUFFER_E, a key-load failure, an algo-negotiation change), these tests would still report green while no longer testing the name check at all. The paired AssertHandshakeSucceeds positive control does rule out the gross negotiation/key-load failures, which is why this is Low rather than Medium -- but the assertion is strictly weaker than the standard the PR itself establishes three functions below it, and weaker than two pre-existing helpers in the same file.

Fix: Add an expectedErr parameter to AssertHandshakeRejectsMutatedReply mirroring AssertHandshakeRejectsCorruptedSig, and assert AssertIntEQ(result.clientErr, WS_PARSE_E) for all four name-downgrade tests.

[yosuke-wolfssl]

Fixed. AssertHandshakeRejectsMutatedReply now takes an expectedErr parameter and asserts AssertIntEQ(result.clientErr, expectedErr), mirroring AssertHandshakeRejectsCorruptedSig. All four name-downgrade tests now pin WS_PARSE_E.

[aidangarske]

can you confirm at least one CI job builds wolfSSL with WOLFSSL_ED25519_STREAMING_VERIFY (and ECC P-256) so the new Ed25519 and ECC negative tests actually execute

[yosuke-wolfssl]

Confirmed. wolfSSL's --enable-all turns these on: in configure.ac, enable_ed25519_stream=yes is defaulted inside the ENABLED_ALL_CRYPT block (alongside enable_ed25519, ECC, etc.), so --enable-all defines HAVE_ED25519, WOLFSSL_ED25519_STREAMING_VERIFY, the ed25519 key import/export, and ECC P-256. That means WOLFSSH_NO_ED25519 is not defined and both the Ed25519 and ECC negative tests compile and run. The CI jobs that build wolfSSL with --enable-all and run the wolfSSH test suite (os-check.yml, sanitizer.yml, plus the sshd/scp/sftp test workflows) exercise these paths, so the Ed25519 and ECC mutation coverage is active in CI, not just the RSA paths.

[aidangarske]

Drop the duplicate AssertHandshakeSucceeds from the ...CorruptSig tests, or fold the name-downgrade and corrupt-sig assertions into a single per-algorithm test that runs the positive control once?

[yosuke-wolfssl]

Done. Dropped the duplicate AssertHandshakeSucceeds from the four *CorruptSig tests.

[yosuke-wolfssl]

Hello @aidangarske ,
Could you please review this again ?