Unknown message types should return .ignore() per RFC 4253
- Dominant language
- Swift
- Stars
- 515
- Forks
- 85
- PR merge metrics
- No merged PRs in 30d
Description
## Summary
NIOSSH violates RFC 4253 by throwing errors for unknown SSH message types instead of gracefully ignoring them. This causes parser state corruption through buffer rewind over decrypted data, and prevents interoperability with SSH implementations that use protocol extensions.
## Environment
- **SwiftNIO SSH version:** main branch (commit 8f33cac)
- **Swift version:** 6.0+
- **Platform:** macOS 15.0, iOS 18.0
## Problem Description
When NIOSSH receives an SSH message with an unknown message type ID (e.g., message IDs in the reserved range 192-255 for private use, or vendor-specific extensions), the parser throws `ParsingError.unknownType` instead of ignoring the message as required by RFC 4253.
### Code Location
`Sources/NIOSSH/SSHMessages.swift:readSSHMessage()` line 512
```swift
default:
throw SSHMessage.ParsingError.unknownType(type) // ❌ Violates RFC 4253
}
```
## RFC 4253 Violation
RFC 4253 Section 11.1 explicitly requires:
> **"An implementation MUST be able to process all message types, though it need not ACT on them."**
And regarding unrecognized messages:
> **"An implementation MAY ignore an unrecognized message."**
**NIOSSH currently violates this requirement** by treating unknown message types as fatal errors.
RFC 4253 Section 12 reserves message ID ranges for extensions:
- **192-255:** Reserved for local/private use
- Implementations must tolerate receiving these without breaking the connection
## Root Cause: Parser Corruption
The error handling causes buffer corruption:
```
1. Encrypted packet arrives and gets decrypted
2. Buffer now contains decrypted message
3. Parser reads unknown message type byte (e.g., 127)
4. Throws ParsingError.unknownType(127)
5. rewindOnNilOrError catches the error
6. Rewinds buffer reader index back before message type byte
7. Re-throws the error (propagates to errorCaught)
8. Buffer reader is now positioned over ALREADY-DECRYPTED data ❌
9. Next encrypted packet arrives
10. Gets decrypted and the decrypted data appended to buffer
11. Parser reads from rewound position
12. Reads corrupted data (partial old message + new message)
13. Sequence number tracking corrupted
14. All subsequent packets fail MAC verification
15. Connection permanently broken
```
## Impact
### Breaks RFC Compliance
The current behavior violates the SSH protocol specification, making NIOSSH non-compliant.
### Prevents Extension Interoperability
This bug blocks NIOSSH from working with:
- **OpenSSH extensions** (various `@openssh.com` message types)
- **Vendor-specific extensions** using reserved message IDs
- **Future SSH protocol extensions**
- **Enterprise SSH with custom features**
- **Research/experimental SSH implementations**
### Observable Symptoms
```
✅ SSH connection established
✅ Messages processed normally
❌ Server sends unknown message type (e.g., extension)
❌ NIOSSH throws ParsingError.unknownType
❌ Buffer rewind corrupts parser state
❌ All subsequent packets fail MAC verification
❌ Connection becomes permanently unusable
```
## Reproduction
### Test Case: Unknown Message Type
```swift
func testUnknownMessageTypeReturnsIgnore() throws {
var buffer = ByteBufferAllocator().buffer(capacity: 100)
// Use message ID 127 (reserved for local/private use per RFC 4253)
buffer.writeInteger(UInt8(127))
buffer.writeSSHString("extension-data".utf8)
// ❌ Current: Throws ParsingError.unknownType(127)
// ✅ Expected: Returns .ignore() per RFC 4253
let message = try buffer.readSSHMessage()
guard case .some(.ignore) = message else {
XCTFail("Expected .ignore for unknown message type per RFC 4253")
return
}
}
```
**Current behavior:** Test **FAILS** - throws `unknownType(127)`
**Expected behavior:** Test **PASSES** - returns `.ignore()`
### Test Case: Parser State Continuity
```swift
func testParserStateAfterUnknownMessageType() throws {
var buffer = ByteBufferAllocator().buffer(capacity: 200)
// Write unknown message type
buffer.writeInteger(UInt8(125))
buffer.writeBytes([1, 2, 3, 4]) // Some payload
// ❌ Current: Throws error
// ✅ Expected: Returns .ignore()
let message1 = try buffer.readSSHMessage()
guard case .some(.ignore) = message1 else {
XCTFail("Expected .ignore for unknown message type")
return
}
// Write known message
buffer.writeInteger(SSHMessage.ChannelSuccessMessage.id)
buffer.writeInteger(UInt32(0))
// ❌ Current: Parser corrupted, fails to read
// ✅ Expected: Parser continues normally
let message2 = try buffer.readSSHMessage()
guard case .some(.channelSuccess) = message2 else {
XCTFail("Parser state corrupted after unknown message type")
return
}
}
```
**Current behavior:** Test **FAILS** - throws `unknownType(125)` on first read
**Expected behavior:** Test **PASSES** - parser continues normally
### Existing Test Expects Wrong Behavior
The test `testTypeError` currently expects unknown message types to throw:
```swift
func testTypeError() throws {
var buffer = ByteBufferAllocator().buffer(capacity: 100)
XCTAssertNil(try buffer.readSSHMessage())
buffer.writeBytes([127])
XCTAssertThrowsError(try buffer.readSSHMessage()) // ❌ Wrong expectation
}
```
This test enforces RFC-violating behavior and needs to be updated.
## How Other Implementations Handle This
All major SSH implementations follow RFC 4253 by ignoring unknown messages:
- **OpenSSH:** Silently ignores, optionally logs for debugging
- **libssh2:** Ignores unknown message types
- **Paramiko (Python):** Ignores with optional logging
- **libssh (C):** Ignores per RFC requirements
NIOSSH is the outlier by making unknown messages fatal.
## Proposed Solution
Follow RFC 4253 by returning `.ignore()` for unknown message types:
```swift
default:
// Unknown SSH message type - consume remaining bytes and return as ignore.
// This prevents parser state corruption when encountering extension messages
// that NIOSSH doesn't support. Per RFC 4253 Section 11.1: "An implementation
// MAY ignore an unrecognized message."
self.moveReaderIndex(to: self.writerIndex)
return .ignore(.init(data: ByteBuffer()))
```
### Why This Works
1. **RFC Compliant:** Follows RFC 4253 Section 11.1
2. **Prevents Corruption:** Consumes all bytes, no buffer rewind
3. **Connection Continues:** Parser state remains valid
4. **Interoperable:** Works with extensions from any vendor
5. **Consistent:** Matches how all other SSH implementations work
## Breaking Changes
⚠️ **Yes, this is a breaking change:**
**Before:**
- Unknown message types throw `ParsingError.unknownType`
- Error propagates to `NIOSSHHandler.channelRead`
- Fires `context.fireErrorCaught(error)`
- Applications could catch this error
**After:**
- Unknown message types return `.ignore()`
- No error thrown
- Connection continues normally
- Applications won't receive error notification
### Rationale for Breaking Change
While this changes observable behavior, it's necessary because:
1. **Old behavior violates RFC 4253** - required by protocol spec
2. **Old behavior corrupts connections** - no recovery possible anyway
3. **Old behavior breaks interoperability** - can't work with extensions
4. **New behavior matches all other implementations**
5. **Connections that failed will now succeed** - net improvement
### Migration Guide
**If you were catching `ParsingError.unknownType`:**
```swift
// OLD CODE - will no longer receive this error
do {
try processSSH()
} catch SSHMessage.ParsingError.unknownType(let type) {
logger.warning("Unknown message type: \(type)")
closeConnection() // Connection was dying anyway
}
```
**After this fix:**
- The error is no longer thrown
- Connections continue normally per RFC 4253
- If you need observability, monitor at a higher level
**Impact:** Connections that previously failed will now succeed. This is the desired RFC-compliant behavior.
## Test Coverage
Add two new test cases and update one existing test:
1. **`testUnknownMessageTypeReturnsIgnore`** (NEW)
Verify unknown message types return `.ignore()` per RFC 4253
2. **`testParserStateAfterUnknownMessageType`** (NEW)
Verify parser continues normally after unknown messages
3. **`testTypeError`** (UPDATE)
Change from expecting throw to expecting `.ignore()`
## Files Changed
1. `Sources/NIOSSH/SSHMessages.swift`
Line ~512: Replace `throw` with `.ignore()` return + buffer consumption
2. `Tests/NIOSSHTests/SSHMessagesTests.swift`
- Add 2 new test cases (~60 lines)
- Update `testTypeError` to expect `.ignore()` instead of throw (~10 lines)
**Total changes:** ~5 lines fix + ~70 lines tests
## Priority
**IMPORTANT** - RFC compliance and extension interoperability
Not as urgent as Issue #1 (unknown channel requests) since this hasn't been observed in production yet, but important for:
- RFC 4253 compliance
- Future-proofing against OpenSSH extensions
- Consistency with other SSH implementations
## Relationship to Issue #1
This issue shares the same root cause pattern as Issue #1 (unknown channel requests):
- Both fail to consume buffer bytes
- Both cause parser corruption through `rewindOnNilOrError`
- Both fixed with `moveReaderIndex(to: writerIndex)`
However, they can be **fixed independently**:
- Issue #1: Critical production blocker, no breaking changes
- Issue #2: RFC compliance improvement, has breaking changes
## References
- RFC 4253 Section 11.1: Message processing requirements
- RFC 4253 Section 12: Message type number assignments (reserved ranges)
- RFC 4250 Section 4.6: SSH protocol assigned numbers
- OpenSSH implementation: Ignores unknown message types
- libssh2 implementation: Ignores unknown message types
Contributor guide
Research direction
Start in Sources/NIOSSH/SSHMessages.swift at readSSHMessage(), around line 512, and review the existing testTypeError in Tests/NIOSSHTests/SSHMessagesTests.swift. Run the SSH message tests first, then add coverage for unknown message handling and parser continuity. Done means unknown types are ignored without corrupting subsequent parsing and the updated tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- swift
- Domain
- networking, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100