Unknown message types should return .ignore() per RFC 4253

[jeffellin]

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

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

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

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:

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:

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:

// 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 Support more channel requests. #1: Critical production blocker, no breaking changes
• Issue Define new configuration logic and global request delegate #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