Skip to content

Harden KEXINIT parsing #1055

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
ejohnstown wants to merge 2 commits into
base: master
Choose a base branch
from
+97 −12
Open

Harden KEXINIT parsing #1055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ae1d97
Strip trailing comma from peer name lists
ejohnstown Jun 23, 2026
0357bd0
Bound KEXINIT language name-list skips
ejohnstown Jun 23, 2026
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
13 changes: 7 additions & 6 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3915,6 +3915,11 @@ static int GetNameListRaw(byte* idList, word32* idListSz,
return WS_BAD_ARGUMENT;
}

/* Adjust nameListSz for a trailing comma. */
if (nameListSz > 0 && nameList[nameListSz - 1] == ',') {
nameListSz--;
}

/* Reject oversized name-lists to bound the per-token NameToId scan cost.
* Applies to every list parsed here; the built-in canned lists are far
* under these caps. */
Expand Down Expand Up @@ -4722,17 +4727,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 */
Expand Down
96 changes: 90 additions & 6 deletions tests/regress.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -431,12 +437,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)
{
Expand Down Expand Up @@ -2980,6 +2980,88 @@ static void TestKexInitNameListCaps(void)
WFREE(list, NULL, DYNTYPE_STRING);
}

/* 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);
}

/* 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

@aidangarske aidangarske Jul 7, 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.

🔵 [Low] Second KEXINIT language skip (server-to-client) has no independent regression coverage · test

The PR hardens both language name-list skips in DoKexInit: the client-to-server list at src/internal.c:4730 and the server-to-client list at src/internal.c:4736, both now bounded via GetSkip. The new regression test only corrupts the first (client-to-server) language length: it walks past the cookie and the eight algorithm name-lists to reach 'off', overwrites that C2S length with 0xFFFFFFFC, and asserts WS_BUFFER_E. Because parsing fails at the C2S list, execution never reaches the S2C GetSkip at internal.c:4736, so that changed branch is never exercised. If a future refactor reverted only the S2C skip back to the unchecked begin += skipSz, this test would still pass. The two call sites are identical, so this is a coverage gap rather than a current correctness bug.

Fix: Add a mirrored case (or parameterize the test over the two language fields) that advances past the first language name-list and corrupts the second (server-to-client) language length, confirming it is also rejected with WS_BUFFER_E so both changed branches at internal.c:4730 and internal.c:4736 are independently locked.

* 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

@aidangarske aidangarske Jul 7, 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.

remove emdash? comment seems a little too long maybe reduce the size

* 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)
Expand Down Expand Up @@ -4391,6 +4473,8 @@ int main(int argc, char** argv)
TestFirstPacketFollows();
TestKexInitReservedNonZeroRejected();
TestKexInitNameListCaps();
TestKexInitTrailingComma();
TestKexInitLanguageLengthOverflow();
TestDoKexInitRejectsWhenPeerIsKeying();
#endif
#if !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256) && !defined(WOLFSSH_NO_RSA) \
Expand Down
Loading