Skip to content

fix: send SSH_MSG_DISCONNECT on KEX failure #1091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MarkAtwood wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: send SSH_MSG_DISCONNECT on KEX failure #1091

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5332,6 +5332,11 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
ret = ssh->error;
}
}
/* RFC 4253 11.1: best-effort SSH_MSG_DISCONNECT on negotiation failure. */

@aidangarske aidangarske Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟠 [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 aidangarske Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 [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.

if (ret == WS_MATCH_KEX_ALGO_E || ret == WS_MATCH_KEY_ALGO_E ||
ret == WS_MATCH_ENC_ALGO_E || ret == WS_MATCH_MAC_ALGO_E) {
(void)SendDisconnect(ssh, WOLFSSH_DISCONNECT_KEY_EXCHANGE_FAILED);
Comment on lines +5335 to +5338
}
WLOG(WS_LOG_DEBUG, "Leaving DoKexInit(), ret = %d", ret);
return ret;
}
Expand Down
Loading