fix: send SSH_MSG_DISCONNECT on KEX failure

[MarkAtwood]

Bug

DoKexInit() in src/internal.c sets ret to one of the negotiation-failure codes WS_MATCH_KEX_ALGO_E / WS_MATCH_KEY_ALGO_E / WS_MATCH_ENC_ALGO_E / WS_MATCH_MAC_ALGO_E when no common KEX / host-key / cipher / MAC algorithm can be negotiated, then returns. That code propagates unchanged through DoPacket() and DoReceive(), which set ssh->error and return WS_FATAL_ERROR. No SSH_MSG_DISCONNECT is ever sent on these paths, so the peer is dropped without notification.

RFC 4253 s11.1 says an implementation SHOULD send SSH_MSG_DISCONNECT on a protocol error. The library already follows this best-effort pattern with (void)SendDisconnect(...) for service-request/accept errors (internal.c:7691, 7723); the KEX negotiation paths were inconsistent with that pattern.

Fix

At the single DoKexInit() exit point, send a best-effort SSH_MSG_DISCONNECT with reason WOLFSSH_DISCONNECT_KEY_EXCHANGE_FAILED (3) when ret is any of the four match-failure codes. Uses the same (void)SendDisconnect best-effort form as existing call sites. Safe: ret is not overwritten for match errors (the SendKexInit/ssh->error propagation block is gated on ret == WS_SUCCESS).

Build verification

Compiled the flagged translation unit against a full wolfSSL build (make src/libwolfssh_la-internal.lo, exit 0) on wolfssh master 974f4fd.

Reported by static analysis (Fenrir finding 608). Severity Low (RFC uses SHOULD, not MUST).

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.

Ensures the library sends a best-effort SSH_MSG_DISCONNECT when KEX negotiation fails due to no mutually supported algorithms, aligning behavior with RFC 4253 guidance and existing disconnect-on-error patterns in the codebase.

Changes:

• Adds a best-effort (void)SendDisconnect(...) call in DoKexInit() when KEX/hostkey/cipher/MAC matching fails.
• Uses WOLFSSH_DISCONNECT_KEY_EXCHANGE_FAILED as the disconnect reason for these negotiation failures.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[aidangarske]

🐺 Skoll Code Review

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

Posted findings

• [High] Disconnect can be followed by another KEX packet during rekey failure — src/internal.c:5335-5338
• [Medium] Missing regression coverage for the new disconnect side effect — src/internal.c:5335-5338

Review generated by Skoll.

[aidangarske]

🟠 [High] Disconnect can be followed by another KEX packet during rekey failure
🚫 BLOCK bug

The new SendDisconnect() call is placed inside DoKexInit(), but the production caller still continues its post-DoKexInit() rekey send path based only on ssh->isKeying. DoKexInit() sets WOLFSSH_PEER_IS_KEYING before negotiation completes, so on a post-auth rekey (connectState == CONNECT_SERVER_CHANNEL_REQUEST_DONE) a host-key, cipher, or MAC mismatch can send SSH_MSG_DISCONNECT here and then return to DoPacket(), which can immediately call SendKexDhInit() or SendKexDhGexRequest() because ret is not checked. That means the PR can emit a key-exchange packet after the disconnect it just added, which breaks the intended terminate-on-negotiation-failure behavior.

Recommendation: Gate the rekey response in DoPacket() on successful DoKexInit() completion (e.g. if (ret == WS_SUCCESS && ssh->isKeying && ssh->connectState == CONNECT_SERVER_CHANNEL_REQUEST_DONE)) so no follow-on KEX response is emitted after DoKexInit() returns a negotiation-failure error.

[aidangarske]

🟡 [Medium] Missing regression coverage for the new disconnect side effect
💡 SUGGEST test

The PR adds observable behavior: negotiation match failures should now send SSH_MSG_DISCONNECT with reason WOLFSSH_DISCONNECT_KEY_EXCHANGE_FAILED. Existing regression tests assert the returned match errors, such as WS_MATCH_KEX_ALGO_E, WS_MATCH_ENC_ALGO_E, and WS_MATCH_MAC_ALGO_E, but they do not capture the outbound packet or reason code. A future change could preserve the return code while dropping the new disconnect behavior.

Recommendation: Add a regression test using a send-capture IO callback for at least one negotiation mismatch path, and assert that the emitted packet is MSGID_DISCONNECT with reason WOLFSSH_DISCONNECT_KEY_EXCHANGE_FAILED.