Add strict key exchange, the Terrapin mitigation - #1271
ejohnstown wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 5
Open (6)
sleep 0.1(fractional sleeps), which is not required by POSIX and can fail… · NewHandshakeStrictKex()is intended (per its comment) to “freeze” the strict-KEX setting the first… · Newssh.h/README state the session Strict KEX setting is “fixed” once… · NewID_EXT_PRE_STRICT_KEX_{S,C}don’t distinguish these as the OpenSSH… · New
What changed in this PR
Adds support for strict key exchange (draft-miller-sshm-strict-kex) to mitigate the Terrapin attack (CVE-2023-48795), including negotiation markers, initial-KEX message gating, and sequence-number resets at NEWKEYS, plus test coverage and OpenSSH interop checks.
Changes:
- Introduces strict-KEX negotiation (both marker spellings), initial-KEX allowlisting, and seqnr reset on every NEWKEYS.
- Adds public APIs to opt out per-CTX and per-session, and documents behavior in README.
- Expands regression/API tests and adds an OpenSSH 9.6+ interoperability script used by
make check.
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents and exposes new Strict KEX public APIs. |
| wolfssh/internal.h | Adds internal flags/IDs for strict-KEX negotiation and session state. |
| src/ssh.c | Implements new Strict KEX public API setters/getters; updates text IDs. |
| src/internal.c | Implements strict-KEX handshake freezing, initial-KEX allowlist, negotiation parsing, and seqnr resets at NEWKEYS. |
| tests/regress.c | Adds extensive strict-KEX/Terrapin regression harness and assertions. |
| tests/api.c | Adds API-level tests for new Strict KEX controls and defaults. |
| scripts/openssh-interop.test | Adds OpenSSH client/sshd interop verification for strict-KEX negotiation and continuity. |
| scripts/include.am | Adds the new interop script to distributed test scripts. |
| README.md | Documents strict key exchange behavior, defaults, and opt-out APIs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
154efec to
9670a8c
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1271
Scan targets checked: wolfssh-src, wolfssh-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
4869a3c to
6400e37
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1271
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
dbcc484 to
48fa84e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1271
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
philljj
left a comment
philljj
left a comment
There was a problem hiding this comment.
Looks good so far, just a few nits and a question.
| static byte HandshakeStrictKex(WOLFSSH* ssh) | ||
| { | ||
| if (ssh->handshake == NULL) { | ||
| return ssh->sendStrictKex; |
There was a problem hiding this comment.
Is the idea that if a handshake hasn't been allocated, then use what's set on the ssh? (which would be inherited from the ctx by default)
There was a problem hiding this comment.
Yes, that was the idea, but HandshakeStrictKex() is gone now. I dropped the per-session setter, so the setting lives only on the CTX. wolfSSH_new() copies it into the session, and nothing writes it after that. SendKexInit() and DoKexInit() both read that one copy, so they can't disagree, and the handshake doesn't need its own copy.
| } | ||
|
|
||
| if (!ssh->handshake->strictKexSet) { | ||
| ssh->handshake->strictKex = ssh->sendStrictKex ? 1 : 0; |
There was a problem hiding this comment.
nit: a short comment at this if would be nice.
The handshake is inheriting the strictKex setting from ssh here I guess.
This looks to be the only place handshake->strictKex member is assigned.
There was a problem hiding this comment.
This one's moot now. HandshakeInfo no longer has strictKex or strictKexSet. See the reply above: the session's copy made at wolfSSH_new() is the only one.
| if (state == WS_MSG_RECV && ssh->strictKexEnabled && | ||
| !ssh->initialKexDone) { | ||
| if (msg != MSGID_DISCONNECT && msg != MSGID_KEXINIT && | ||
| msg != MSGID_NEWKEYS && !MSGIDLIMIT_TRANS_KEX(msg)) { |
There was a problem hiding this comment.
I think MSGIDLIMIT_TRANS_KEX(msg) allows everything from MSGIDLIMIT_TRANS_KEX_MIN = 30 to MSGIDLIMIT_TRANS_KEX_MAX = 49.
Is this too broad?
Skoll complained about it:
HIGH-1: Strict KEX admits messages from the wrong key-exchange method
- File:
src/internal.c:1053-1056 - Function:
IsMessageAllowed
Description: The new strict-KEX gate exempts every message ID in the 30–49 KEX range, plus SSH_MSG_NEWKEYS, rather than only the messages expected for the negotiated method. The existing server policy does not close this gap: immediately after receiving KEXINIT, handshake->expectMsgId remains MSGID_NONE, and IsMessageAllowedServer() accepts client-side IDs through 34. For example, with ECDH negotiated, GEX_INIT (32) or GEX_REQUEST (34) can pass this strict gate and reach the wrong handler; an early NEWKEYS also passes and is later treated by DoPacket() as completing the initial KEX even when DoNewKeys() fails. Strict KEX requires messages to be specific to the negotiated method and accepted only the expected number of times, as specified in section 3.2 of the strict-KEX draft. This defect is introduced by the PR's blanket KEX-range allowlist.
Code:
if (msg != MSGID_DISCONNECT && msg != MSGID_KEXINIT &&
msg != MSGID_NEWKEYS && !MSGIDLIMIT_TRANS_KEX(msg)) {
Recommendation: Fix the server-side strict-KEX sequencing and add regression cases for wrong-method KEX messages and premature NEWKEYS before merging.
There was a problem hiding this comment.
Yes, there was a gap. The strict KEX allow list takes the whole 30-49 range, and in most places the existing expectMsgId check narrows it to the exact message due next. The server was the exception. After the client's KEXINIT it left expectMsgId unset until the client's first KEX message arrived. In that window, a message from another method (GEX_INIT or GEX_REQUEST under ECDH, say) or an early NEWKEYS got through.
Fixed:
- After the client's KEXINIT, the server expects KEXDH_INIT, or KEXDH_GEX_REQUEST for DH-GEX. Anything else is refused, and under strict KEX that ends the connection with KEY_EXCHANGE_FAILED. After a wrong
first_kex_packet_follows guess, the server discards the guessed packet first and only then sets the expectation, because the guessed packet's ID can be from another method. DoPacket()resets the sequence number and ends the initial KEX only on a NEWKEYS that actually installed the new keys. A NEWKEYS that fails is counted like any other packet.- New regress tests: NEWKEYS, KEXDH_REPLY, GEX_INIT and GEX_REQUEST sent ahead of the client's KEXDH_INIT each end the connection under strict KEX, and a NEWKEYS that fails doesn't end the initial KEX.
The expectation is set whether or not strict KEX is on, so this also tightens the non-strict server path
The "Unable to copy" message now carries the return code and the session error with its name. The return alone is usually the generic WS_FATAL_ERROR, which says nothing about the cause.
When select() reports the socket readable but only part of a packet has arrived, wolfSSH_worker() returns WS_WANT_READ. sftp_worker() now goes back to waiting on the socket instead of ending the session, which cut off SFTP transfers whose packets arrived split across reads.
GetInputLine() reads up to 255 bytes for the version line, so a peer that sends its first packets in the same segment leaves them buffered behind it. DoReceive() force-freed the input buffer after each packet, dropping any of those bytes past the first packet. It now keeps them and frees the buffer only once it is empty.
48fa84e to
2e2a361
Compare
wolfSSH negotiates strict key exchange, the mitigation from draft-miller-sshm-strict-kex for CVE-2023-48795, and turns it on when the peer asks for it too. Nothing in the initial KEX is authenticated, so a packet spliced into it shifts the receiver's sequence number, and that shift is the attack. - SendKexInit() carries the marker on the initial KEXINIT only, sending the draft's unprefixed name and then the -v00@openssh.com name deployed OpenSSH uses; Paramiko lets the last kex-strict-* name decide and knows only the v00 one, so it goes last - DoKexInit() matches either spelling and enables the mitigation when both sides asked for it; a marker in a rekey KEXINIT is ignored - DoKexInit() ends the connection when strict KEX is on and the KEXINIT was not the peer's first packet - DoPacket() and SendNewKeys() zero peerSeq and seq at every NEWKEYS, the outbound reset landing before SendPendingChannelWindowAdjust() can bundle a packet at the old number - IsMessageAllowed() takes an allow list until the peer's NEWKEYS arrives: DISCONNECT, KEXINIT, NEWKEYS and the key exchange range - a refused message ends the connection with SSH_MSG_DISCONNECT and KEY_EXCHANGE_FAILED rather than being dropped - wolfSSH_CTX_SetStrictKex() opts out per context; wolfSSH_new() copies the setting and nothing changes it after, so advertising the marker and enforcing it read the same value - wolfSSH_GetStrictKexNegotiated() reports whether a session has it on - wolfSSH_SendIgnore() refuses to send while strict KEX is offered and the initial KEX is still running, since a strict peer would disconnect - ID_EXTINFO_{S,C} become ID_EXT_INFO_{S,C}, beside four new pseudo-KEX ids for the strict KEX names - reword WS_MSGID_NOT_ALLOWED_E, which no longer only covers userauth - cover the negotiation, the gate, the injections and the resets in regress.c, and the new calls in api.c - describe strict KEX and its opt-out in README.md
Runs the OpenSSH client against the echoserver and the wolfSSH example client against sshd, checking that each pairing negotiates the strict KEX marker and that a session then runs over the connection. - skip when ssh is missing or predates OpenSSH 9.6 - fall back from the ECC keys to RSA, then Ed25519, on a build without them, and give sshd RSA and Ed25519 host keys beside the ECDSA one - skip the sshd half alone when sshd or ssh-keygen is missing, or sshd will not start unprivileged - report the sequence number resets OpenSSH logs rather than test them, the wording being no contract - run from make check through dist_noinst_SCRIPTS
After the client's KEXINIT the server left expectMsgId unset until the client's first KEX message arrived, so any transport ID got through in that window: a message from another KEX method, or an early NEWKEYS. Strict KEX allows only the messages the negotiated method expects (draft-ietf-sshm-strict-kex section 3.2). - DoKexInit() sets expectMsgId to KEXDH_INIT, or KEXDH_GEX_REQUEST for DH-GEX; after a wrong first_kex_packet_follows guess it is set once the guessed packet is skipped - DoPacket() resets peerSeq and ends the initial KEX only on a NEWKEYS that installed keys; one that fails is counted like any other packet - regress: out-of-turn KEX messages ahead of the client's KEXDH_INIT end the connection under strict KEX, and a failed NEWKEYS is counted - regress: the first_kex_packet_follows cross-boundary case expects the negotiated KEX's message after the skip
2e2a361 to
7b65663
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1271
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 5 of 7 in-scope changed file(s) opened by the reviewer; not opened: wolfssh/error.h, wolfssh/ssh.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
wolfSSH negotiates strict key exchange (draft-miller-sshm-strict-kex), the mitigation for the Terrapin attack, CVE-2023-48795, and enables it when the peer asks for it too.