Add strict key exchange, the Terrapin mitigation - #1271
ejohnstown wants to merge 8 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.
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 both the -v00@openssh.com name deployed OpenSSH uses and the draft's unprefixed one - DoKexInit() matches either spelling and enables the mitigation when both sides asked for it; a marker in a rekey KEXINIT is ignored - 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() and wolfSSH_SetStrictKex() opt out per context or per session, the context seeding each session it makes - the handshake freezes the setting when it starts, so advertising the marker and enforcing it cannot read different values - 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-outs 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 an Ed25519 host key for the same case - 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
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.
The strict KEX allow list is armed only once the peer's KEXINIT has negotiated it, so a packet spliced in ahead of that KEXINIT got through. DoKexInit() now ends the connection with KEY_EXCHANGE_FAILED when strict KEX is on and the KEXINIT is not the peer's first packet. - regress: a harness mode that splices a packet in ahead of the server's KEXINIT - regress: IGNORE, UNIMPLEMENTED, EXT_INFO and an unassigned id there are refused with strict KEX; an IGNORE is accepted without it
SendKexInit() now lists kex-strict-*-v00@openssh.com after the plain kex-strict-* name. Paramiko lets the last kex-strict-* name it sees decide strict KEX and knows only the v00 form, so with it first Paramiko turned strict KEX off.
A build with neither ECDSA nor Ed25519 can verify only an RSA host key, so openssh-interop.test now starts sshd with an RSA host key beside the ECDSA and Ed25519 ones.
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)
| } | ||
|
|
||
| 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.
| 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.
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.