Skip to content

Return .ignore() for unknown message types per RFC 4253#224

Open
jeffellin wants to merge 1 commit intoapple:mainfrom
jeffellin:fix/unknown-message-rfc-compliance
Open

Return .ignore() for unknown message types per RFC 4253#224
jeffellin wants to merge 1 commit intoapple:mainfrom
jeffellin:fix/unknown-message-rfc-compliance

Conversation

@jeffellin
Copy link
Copy Markdown

Fixes #223

Summary

This PR fixes the RFC 4253 violation where NIOSSH throws errors for unknown SSH message types instead of gracefully ignoring them. See #223 for detailed problem analysis and reproduction steps.

Changes

  • readSSHMessage(): Return .ignore() for unknown message types instead of throwing, consuming remaining bytes to prevent parser corruption
  • Add testUnknownMessageTypeReturnsIgnore to verify RFC 4253 compliance
  • Add testParserStateAfterUnknownMessageType to verify parser state continuity
  • Update testTypeError to expect .ignore() instead of throw (reflects RFC-compliant behavior)

Motivation

RFC 4253 Section 11.1 explicitly requires:

"An implementation MUST be able to process all message types, though it need not ACT on them."

"An implementation MAY ignore an unrecognized message."

NIOSSH currently violates this by throwing ParsingError.unknownType, which:

  1. Violates the RFC requirement
  2. Causes parser state corruption through buffer rewind over decrypted data
  3. Prevents interoperability with OpenSSH extensions and vendor-specific message types

Root Cause

When the parser encountered an unknown message type (e.g., reserved IDs 192-255 for local use), it would:

  1. Throw ParsingError.unknownType
  2. Trigger rewindOnNilOrError to rewind the buffer reader
  3. Rewind over already-decrypted data, corrupting parser state
  4. Skip incrementing sequence numbers, causing cascading MAC failures
  5. Make the connection permanently unusable

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
    // (e.g. hostkeys-00@openssh.com) 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()))

This matches how all other major SSH implementations handle unknown messages:

  • OpenSSH: Silently ignores
  • libssh2: Ignores unknown message types
  • Paramiko: Ignores with optional logging

Test Results

Before fix:

  • testTypeError: ✅ Passes (but expects wrong behavior - throw instead of ignore)
  • testUnknownMessageTypeReturnsIgnore: ❌ FAILS - throws unknownType(127)
  • testParserStateAfterUnknownMessageType: ❌ FAILS - throws unknownType(125)

After fix:

  • testTypeError: ✅ Passes (now expects correct RFC behavior - ignore)
  • testUnknownMessageTypeReturnsIgnore: ✅ Passes - returns .ignore()
  • testParserStateAfterUnknownMessageType: ✅ Passes - parser continues normally

Full test suite:

$ swift test
Executed 322 tests, with 0 failures

$ swift test \
  --explicit-target-dependency-import-check error \
  -Xswiftc -require-explicit-sendable \
  -Xswiftc -warnings-as-errors
Executed 322 tests, with 0 failures

Breaking Changes

⚠️ Yes, this is a breaking change:

Before:

  • Unknown message types throw ParsingError.unknownType
  • Error propagates to errorCaught handlers
  • Applications could catch this error

After:

  • Unknown message types return .ignore()
  • No error thrown
  • Connections continue normally per RFC 4253

Rationale for Breaking Change

While this changes observable behavior, it's necessary because:

  1. RFC Compliance: RFC 4253 requires implementations to tolerate unknown messages
  2. Old behavior was broken: Connection corruption meant no recovery was possible anyway
  3. Interoperability: Enables working with OpenSSH extensions and vendor-specific message types
  4. Industry standard: All other SSH implementations ignore unknown messages
  5. Net improvement: Connections that previously failed will now succeed

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
  • Unknown messages are silently ignored as .ignore() messages

Impact: Connections that previously failed will now succeed. This is the desired RFC-compliant behavior.

Impact

This fix enables NIOSSH to interoperate with:

  • OpenSSH servers with extension messages (e.g., hostkeys-00@openssh.com)
  • Vendor-specific SSH implementations using reserved message IDs (192-255)
  • Future SSH protocol extensions
  • Experimental/research SSH implementations

Relationship to PR #222 (Issue #221)

This PR shares the same root cause pattern as PR #222:

  • Both bugs fail to consume buffer bytes
  • Both cause parser corruption through rewindOnNilOrError
  • Both fixed with moveReaderIndex(to: writerIndex)

However, they can be merged independently:

Checklist

  • Tests added and all passing
  • Commit message follows template
  • Breaking change documented
  • RFC 4253 compliant
  • Matches industry standard behavior

Motivation:

RFC 4253 Section 11.1 requires: "An implementation MUST be able to
process all message types, though it need not ACT on them" and "An
implementation MAY ignore an unrecognized message."

NIOSSH currently violates this requirement by throwing
ParsingError.unknownType for unknown message types. This causes parser
state corruption through buffer rewind over already-decrypted data, and
prevents interoperability with SSH implementations using protocol
extensions (OpenSSH extensions, vendor-specific message types).

Modifications:

- readSSHMessage(): Return .ignore() for unknown message types instead
  of throwing, consuming remaining bytes to prevent corruption
- Add testUnknownMessageTypeReturnsIgnore to verify RFC compliance
- Add testParserStateAfterUnknownMessageType to verify parser continuity
- Update testTypeError to expect .ignore() instead of throw

Result:

NIOSSH now follows RFC 4253 by gracefully ignoring unknown message
types. Connections continue normally when encountering extensions,
enabling interoperability with OpenSSH extensions and vendor-specific
message types.

Breaking Change: ParsingError.unknownType is no longer thrown for
unknown message types. Connections that previously failed will now
continue normally per RFC requirements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant