Fix readVersion() crash on bare LF as first byte (#237) - #238
Merged
Conversation
SSHPacketParser.readVersion() in the server branch accessed
slice[lfIndex.advanced(by: -1)] without checking that
lfIndex > slice.startIndex. A client sending a bare LF as its
first byte after the TCP handshake (e.g., a misbehaving probe
or scanner) caused the subscript to index one byte before the
buffer start, trapping the NIO event-loop thread with
EXC_BREAKPOINT and taking down the whole server process.
Guard the `-1` access. A leading LF now yields an empty
version string, matching the existing server-branch convention
that everything before the first LF is the version line. The
client branch is unaffected because `slice.starts(with: "SSH-".utf8)`
already guarantees at least 4 bytes precede lfIndex.
Adds a regression test that traps without the fix (reproducing
the production crash signature `index -1 out of range`) and
returns `.version("")` with it.
Contributor
|
LGTM. Thank you for the fix! |
josephnoir
approved these changes
Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #237.
SSHPacketParser.readVersion()in the server branch accessedslice[lfIndex.advanced(by: -1)]without checking thatlfIndex > slice.startIndex. A client whose first byte after the TCPhandshake is a bare
\nmade the subscript index before the bufferstart, trapping the NIO event-loop thread with
EXC_BREAKPOINTandtaking down the whole server process. Reproducible with
printf '\n' | nc <host> <port>.The fix
One-line guard on the
-1access. A leading LF now yields an emptyversion string, which is consistent with the existing server-branch
convention that everything before the first LF is the version line.
Client branch
Unaffected —
slice.starts(with: \"SSH-\".utf8)is checked first,which guarantees
lfIndex >= 4 > slice.startIndex.Test
Added
testReadVersionLineFeedFirstByteOnServer. Verified that thetest reproduces the production crash signature
(
NIOCore/ByteBuffer-views.swift:80: Fatal error: index -1 out of range)without the fix, and passes with it. All 12 existing
SSHPacketParserTestsstill pass.
Background
Reported in #237 after we hit this in production after ~4 days of
uptime on a public SSH port.