Fix parser corruption on unknown channel request types#222
Open
jeffellin wants to merge 1 commit intoapple:mainfrom
Open
Fix parser corruption on unknown channel request types#222jeffellin wants to merge 1 commit intoapple:mainfrom
jeffellin wants to merge 1 commit intoapple:mainfrom
Conversation
Motivation: NIOSSH suffers from critical parser state corruption when receiving SSH channel requests with unknown request type strings. The parser fails to consume payload bytes, leaving unconsumed data that corrupts all subsequent packet reads, causing permanent connection failure. This blocks production usage with enterprise SSH servers, cloud providers, and security appliances that send vendor-specific channel request extensions using the RFC 4250 name@domain format. Modifications: - readChannelRequestMessage(): Consume remaining bytes with moveReaderIndex(to: writerIndex) before setting type = .unknown - Add testUnknownChannelRequestWithPayload to verify bytes consumed - Add testParserContinuesAfterUnknownChannelRequest to verify parser state remains valid This matches the existing pattern in readGlobalRequestMessage() which correctly consumes payload bytes for unknown global requests. Result: NIOSSH can now handle SSH channel requests with unknown request types without connection corruption. Connections continue normally when encountering vendor-specific extensions, enabling interoperability with enterprise SSH servers, cloud providers, and security appliances.
5 tasks
| } | ||
| XCTAssertEqual(request.type, .unknown) | ||
|
|
||
| // Write a known message after |
There was a problem hiding this comment.
Suggested change
| // Write a known message after | |
| // Write a known message afterward |
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
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.
Fixes #221
Summary
This PR fixes the critical parser state corruption bug when NIOSSH receives SSH channel requests with unknown request types. See #221 for detailed problem analysis and reproduction steps.
Changes
readChannelRequestMessage(): Consume remaining bytes withmoveReaderIndex(to: writerIndex)before settingtype = .unknowntestUnknownChannelRequestWithPayloadto verify bytes are consumedtestParserContinuesAfterUnknownChannelRequestto verify parser state remains validThis matches the existing pattern in
readGlobalRequestMessage()(lines 814-823) which correctly consumes payload bytes for unknown global requests.Root Cause
When the parser encountered an unknown channel request type (e.g., vendor-specific extensions like
session-id@example.com), it would settype = .unknownbut fail to consume the payload bytes from the buffer. The unconsumed bytes would corrupt the next packet read, causingCryptoKitError.authenticationFailureon all subsequent SSH messages and making the connection permanently unusable.Solution
Apply the same buffer-consumption pattern used for unknown global requests:
Test Results
Before fix:
testUnknownChannelRequestWithPayload: ❌ FAILS - 24 unconsumed bytes in buffertestParserContinuesAfterUnknownChannelRequest: ❌ FAILS - throwsunknownType(0)errorAfter fix:
testUnknownChannelRequestWithPayload: ✅ PASSES - all bytes consumedtestParserContinuesAfterUnknownChannelRequest: ✅ PASSES - parser continues normallyFull test suite:
Breaking Changes
None. This is a pure bug fix:
SSHMessage.channelRequestwithtype = .unknownImpact
This fix enables NIOSSH to interoperate with:
name@domainextension formatChecklist