From dba1e36e766fea7aa593462c546d8d05c947fd04 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 23 Jun 2026 14:52:48 -0700 Subject: [PATCH 1/2] Strip trailing comma from peer name lists - GetNameListRaw folded a trailing comma into the last name, so NameToId returned ID_UNKNOWN and negotiation failed. - Trim one trailing comma up front, matching AlgoListSz. - Add regression test for a KEX list with a trailing comma. Issue: F-2478 --- src/internal.c | 8 ++++++++ tests/regress.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/src/internal.c b/src/internal.c index dfb92d728..64eb48bd6 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3915,6 +3915,14 @@ static int GetNameListRaw(byte* idList, word32* idListSz, return WS_BAD_ARGUMENT; } + /* A peer name list arrives via GetStringRef with its exact wire size, + * which may include a trailing comma (e.g. "aes128-cbc,"). Trim it so the + * final name is not folded together with the comma and mis-parsed as + * ID_UNKNOWN. Mirrors AlgoListSz, which does this for local lists. */ + if (nameListSz > 0 && nameList[nameListSz - 1] == ',') { + nameListSz--; + } + /* * The strings we want are now in the bounds of the message, and the * length of the list. Find the commas, or end of list, and then decode diff --git a/tests/regress.c b/tests/regress.c index de22281bd..d86e136e5 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2688,6 +2688,40 @@ static void TestKexInitReservedNonZeroRejected(void) wolfSSH_CTX_free(ctx); } +/* A peer name list may legally end with a trailing comma. GetNameListRaw + * used to fold that comma into the final name, yielding ID_UNKNOWN and a + * failed negotiation. Build a KEXINIT whose KEX list ends in a comma and + * assert the algorithm still negotiates. */ +static void TestKexInitTrailingComma(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + byte payload[512]; + word32 payloadSz; + word32 idx = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertIntEQ(wolfSSH_SetAlgoListKex(ssh, FPF_KEX_GOOD), WS_SUCCESS); + AssertIntEQ(wolfSSH_SetAlgoListKey(ssh, FPF_KEY_GOOD), WS_SUCCESS); + + /* KEX list carries a trailing comma. */ + payloadSz = BuildKexInitPayload(ssh, FPF_KEX_GOOD ",", FPF_KEY_GOOD, + 0, payload, (word32)sizeof(payload)); + + /* The tail (host key/send) errors on this bare ssh, but negotiation + * runs first and records the result in handshake->kexId. */ + (void)wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx); + + AssertNotNull(ssh->handshake); + AssertIntEQ(ssh->handshake->kexId, ID_ECDH_SHA2_NISTP256); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + #if !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \ && !defined(WOLFSSH_NO_HMAC_SHA1) && !defined(WOLFSSH_NO_HMAC_SHA2_256) static void TestIndependentAlgoNegotiation(void) @@ -3987,6 +4021,7 @@ int main(int argc, char** argv) && !defined(WOLFSSH_NO_RSA_SHA2_256) TestFirstPacketFollows(); TestKexInitReservedNonZeroRejected(); + TestKexInitTrailingComma(); TestDoKexInitRejectsWhenPeerIsKeying(); #endif #if !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256) && !defined(WOLFSSH_NO_RSA) \ From e49154d94f17199fca0c5540774589f2faf5f157 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 23 Jun 2026 14:54:14 -0700 Subject: [PATCH 2/2] Bound KEXINIT language name-list skips - DoKexInit skipped both language name-lists with an unchecked begin += skipSz; a forged length could wrap begin into earlier payload bytes and still return success. - Use bounds-checked GetSkip for both language fields. - Add regression test; checked skip now rejects with WS_BUFFER_E. Issue: F-5576 --- src/internal.c | 8 ++----- tests/regress.c | 61 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/src/internal.c b/src/internal.c index 64eb48bd6..be7c4c9b9 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4717,17 +4717,13 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) /* Languages - Client to Server, skip */ if (ret == WS_SUCCESS) { WLOG(WS_LOG_DEBUG, "DKI: Languages - Client to Server"); - ret = GetUint32(&skipSz, buf, len, &begin); - if (ret == WS_SUCCESS) - begin += skipSz; + ret = GetSkip(buf, len, &begin); } /* Languages - Server to Client, skip */ if (ret == WS_SUCCESS) { WLOG(WS_LOG_DEBUG, "DKI: Languages - Server to Client"); - ret = GetUint32(&skipSz, buf, len, &begin); - if (ret == WS_SUCCESS) - begin += skipSz; + ret = GetSkip(buf, len, &begin); } /* First KEX Packet Follows */ diff --git a/tests/regress.c b/tests/regress.c index d86e136e5..29102cf6a 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -141,6 +141,12 @@ static word32 AppendUint32(byte* buf, word32 bufSz, word32 idx, word32 value) return idx; } +static word32 ReadUint32(const byte* buf) +{ + return ((word32)buf[0] << 24) | ((word32)buf[1] << 16) | + ((word32)buf[2] << 8) | (word32)buf[3]; +} + static word32 AppendData(byte* buf, word32 bufSz, word32 idx, const byte* data, word32 dataSz) { @@ -394,12 +400,6 @@ typedef struct { word32 steps; } KexReplyRunResult; -static word32 ReadUint32(const byte* buf) -{ - return ((word32)buf[0] << 24) | ((word32)buf[1] << 16) | - ((word32)buf[2] << 8) | (word32)buf[3]; -} - static int ReadStringRef(word32* strSz, const byte** str, const byte* buf, word32 len, word32* idx) { @@ -2722,6 +2722,54 @@ static void TestKexInitTrailingComma(void) wolfSSH_CTX_free(ctx); } +/* The two KEXINIT language name-lists were skipped with an unchecked + * begin += skipSz, so a bogus length could wrap begin back into the payload + * and let a malformed packet parse as valid. GetSkip now bounds the declared + * length against the remaining payload; an overlong language length must be + * rejected. */ +static void TestKexInitLanguageLengthOverflow(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + byte payload[512]; + word32 payloadSz; + word32 idx = 0; + word32 off; + int i; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertIntEQ(wolfSSH_SetAlgoListKex(ssh, FPF_KEX_GOOD), WS_SUCCESS); + AssertIntEQ(wolfSSH_SetAlgoListKey(ssh, FPF_KEY_GOOD), WS_SUCCESS); + + payloadSz = BuildKexInitPayload(ssh, FPF_KEX_GOOD, FPF_KEY_GOOD, + 0, payload, (word32)sizeof(payload)); + + /* Walk past the cookie and the eight algorithm name-lists to reach the + * first (client-to-server) language length field. */ + off = COOKIE_SZ; + for (i = 0; i < 8; i++) + off += UINT32_SZ + ReadUint32(payload + off); + + /* 0xFFFFFFFC = 2^32 - 4. After reading this 4-byte length the parser is + * 4 bytes past 'off', so the old unchecked begin += skipSz wraps begin + * back to 'off' itself -- in bounds, not past the buffer. The buggy + * parser then keeps going from the wrong offset and reads a non-zero + * reserved field, returning WS_PARSE_E. The checked GetSkip instead + * rejects the overlong length outright with WS_BUFFER_E, which is what + * locks the fix: the pre-fix code returns a different (non-buffer) + * error here. */ + (void)AppendUint32(payload, (word32)sizeof(payload), off, 0xFFFFFFFCu); + + AssertIntEQ(wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx), + WS_BUFFER_E); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + #if !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \ && !defined(WOLFSSH_NO_HMAC_SHA1) && !defined(WOLFSSH_NO_HMAC_SHA2_256) static void TestIndependentAlgoNegotiation(void) @@ -4022,6 +4070,7 @@ int main(int argc, char** argv) TestFirstPacketFollows(); TestKexInitReservedNonZeroRejected(); TestKexInitTrailingComma(); + TestKexInitLanguageLengthOverflow(); TestDoKexInitRejectsWhenPeerIsKeying(); #endif #if !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256) && !defined(WOLFSSH_NO_RSA) \