Harden KEXINIT parsing#1055
Conversation
ejohnstown
commented
Jun 24, 2026
ejohnstown
commented
- Strip trailing comma from peer name lists (F-2478): GetNameListRaw folded a legal trailing comma (e.g. aes128-cbc,) into the final name, decoding it as ID_UNKNOWN and failing negotiation. Now trims it, mirroring AlgoListSz.
- Bound KEXINIT language name-list skips (F-5576): the two language lists were skipped via an unchecked GetUint32 + begin += skipSz, letting a forged length wrap the parse offset back into earlier payload bytes and still return success. Both now use GetSkip, which validates the length against the remaining payload.
There was a problem hiding this comment.
Pull request overview
Hardens SSH_MSG_KEXINIT parsing to be more robust against edge-case (but wire-valid) name-lists and against malformed packets that attempt to manipulate parser offsets via oversized language list lengths.
Changes:
- Trim a trailing comma from incoming (peer) name-lists before decoding IDs to avoid mis-parsing the final algorithm name.
- Replace unchecked
GetUint32+begin += skipSzskipping of KEXINIT language name-lists withGetSkip()to enforce bounds and prevent offset wrap. - Add regression tests covering trailing-comma negotiation and overlong language length rejection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/regress.c |
Adds targeted regression tests to lock in the KEXINIT parsing hardening behaviors. |
src/internal.c |
Adjusts name-list decoding to ignore a trailing comma and uses bounded skipping for KEXINIT language fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ejohnstown
force-pushed
the
kexinit-parse
branch
from
048dc7c to
e49154d
Compare
- 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
- 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
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 2 total — 1 posted, 1 skipped
1 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Low] [review] Second KEXINIT language skip (server-to-client) has no independent regression coverage —
tests/regress.c:3042-3059
Skipped findings
- [Info]
Single trailing-comma trim changes outcome for doubly-trailing-comma name-lists
Review generated by Skoll
| 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 |
There was a problem hiding this comment.
🔵 [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.
|
|
||
| /* 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 |
There was a problem hiding this comment.
remove emdash? comment seems a little too long maybe reduce the size
