Set a maximum packet size of 32768 bytes for version and cleartext packets

Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift

@@ -33,8 +33,10 @@ final class SSHChannelMultiplexer {
 
     /// Whether new channels are allowed. Set to `false` once the parent channel is shut down at the TCP level.
     private var canCreateNewChannels: Bool
+
+    private let maximumPacketSize: Int
 
-    init(delegate: SSHMultiplexerDelegate, allocator: ByteBufferAllocator, childChannelInitializer: SSHChildChannel.Initializer?) {
+    init(delegate: SSHMultiplexerDelegate, allocator: ByteBufferAllocator, childChannelInitializer: SSHChildChannel.Initializer?, maximumPacketSize: Int = 1 << 17) {
         self.channels = [:]
         self.channels.reserveCapacity(8)
         self.erroredChannels = []
@@ -43,6 +45,7 @@ final class SSHChannelMultiplexer {
         self.allocator = allocator
         self.childChannelInitializer = childChannelInitializer
         self.canCreateNewChannels = true
+        self.maximumPacketSize = maximumPacketSize
     }
 
     // Time to clean up. We drop references to things that may be keeping us alive.
@@ -190,14 +193,16 @@ extension SSHChannelMultiplexer {
             self.nextChannelID = 0
         }
 
-        // TODO: Make the window management parameters configurable
+        // `maximumPacketSize` can be safely cast, because overrides of the default by implementations are expected to have sensible values
+        // These are also asserted in SSHPackageParser.init
         let channel = SSHChildChannel(allocator: self.allocator,
                                       parent: parentChannel,
                                       multiplexer: self,
                                       initializer: initializer,
                                       localChannelID: channelID,
-                                      targetWindowSize: 1 << 24,
-                                      initialOutboundWindowSize: 0) // The initial outbound window size is presumed to be 0 until we're told otherwise.
+                                      targetWindowSize: Int32(maximumPacketSize),
+                                      initialOutboundWindowSize: 0,
+                                      maximumPacketSize: maximumPacketSize) // The initial outbound window size is presumed to be 0 until we're told otherwise.
 
         self.channels[channelID] = channel
         return channel

Sources/NIOSSH/Child Channels/SSHChildChannel.swift

@@ -97,6 +97,8 @@ final class SSHChildChannel {
     private var didClose = false
 
     private var type: SSHChannelType?
+
+    private let maximumPacketSize: Int
 
     /// A promise from the user that will be fired when the channel goes active.
     private var userActivatePromise: EventLoopPromise<Channel>?
@@ -107,14 +109,16 @@ final class SSHChildChannel {
                               initializer: Initializer?,
                               localChannelID: UInt32,
                               targetWindowSize: Int32,
-                              initialOutboundWindowSize: UInt32) {
+                              initialOutboundWindowSize: UInt32,
+                              maximumPacketSize: Int) {
         self.init(allocator: allocator,
                   parent: parent,
                   multiplexer: multiplexer,
                   initializer: initializer,
                   initialState: .init(localChannelID: localChannelID),
                   targetWindowSize: targetWindowSize,
-                  initialOutboundWindowSize: initialOutboundWindowSize)
+                  initialOutboundWindowSize: initialOutboundWindowSize,
+                  maximumPacketSize: maximumPacketSize)
     }
 
     private init(allocator: ByteBufferAllocator,
@@ -123,7 +127,8 @@ final class SSHChildChannel {
                  initializer: Initializer?,
                  initialState: ChildChannelStateMachine,
                  targetWindowSize: Int32,
-                 initialOutboundWindowSize: UInt32) {
+                 initialOutboundWindowSize: UInt32,
+                 maximumPacketSize: Int) {
         self.allocator = allocator
         self.closePromise = parent.eventLoop.makePromise()
         self.parent = parent
@@ -136,6 +141,7 @@ final class SSHChildChannel {
         self.state = initialState
         self.writabilityManager = ChildChannelWritabilityManager(initialWindowSize: initialOutboundWindowSize,
                                                                  parentIsWritable: parent.isWritable)
+        self.maximumPacketSize = maximumPacketSize
         self.peerMaxMessageSize = 0
 
         // To begin with we initialize autoRead and halfClosure to false, but we are going to fetch it from our parent before we
@@ -468,15 +474,15 @@ extension SSHChildChannel: Channel, ChannelCore {
             let message = SSHMessage.ChannelOpenConfirmationMessage(recipientChannel: self.state.remoteChannelIdentifier!,
                                                                     senderChannel: self.state.localChannelIdentifier,
                                                                     initialWindowSize: self.windowManager.targetWindowSize,
-                                                                    maximumPacketSize: 1 << 24) // This is a weirdly hard-coded choice.
+                                                                    maximumPacketSize: UInt32(maximumPacketSize))
             self.processOutboundMessage(.channelOpenConfirmation(message), promise: nil)
             self.writePendingToMultiplexer()
         } else if !self.state.isClosed {
             // We need to request the channel. We must have the channel by now.
             let message = SSHMessage.ChannelOpenMessage(type: .init(self.type!),
                                                         senderChannel: self.state.localChannelIdentifier,
                                                         initialWindowSize: self.windowManager.targetWindowSize,
-                                                        maximumPacketSize: 1 << 24)
+                                                        maximumPacketSize: UInt32(maximumPacketSize))
             self.processOutboundMessage(.channelOpen(message), promise: nil)
             self.writePendingToMultiplexer()
         } else {

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

@@ -686,7 +686,7 @@ struct SSHConnectionStateMachine {
             switch message {
             case .version:
                 try state.serializer.serialize(message: message, to: &buffer)
-                self.state = .sentVersion(.init(idleState: state, allocator: allocator))
+                self.state = .sentVersion(.init(idleState: state, allocator: allocator, maximumPacketSize: state.role.maximumPacketSize))
             case .disconnect:
                 try state.serializer.serialize(message: message, to: &buffer)
                 self.state = .sentDisconnect(state.role)

Sources/NIOSSH/Connection State Machine/States/SentVersionState.swift

@@ -30,12 +30,12 @@ extension SSHConnectionStateMachine {
 
         private let allocator: ByteBufferAllocator
 
-        init(idleState state: IdleState, allocator: ByteBufferAllocator) {
+        init(idleState state: IdleState, allocator: ByteBufferAllocator, maximumPacketSize: Int) {
             self.role = state.role
             self.serializer = state.serializer
             self.protectionSchemes = state.protectionSchemes
 
-            self.parser = SSHPacketParser(allocator: allocator)
+            self.parser = SSHPacketParser(allocator: allocator, maximumPacketSize: maximumPacketSize)
             self.allocator = allocator
         }
     }

Sources/NIOSSH/NIOSSHError.swift

@@ -43,6 +43,8 @@ extension NIOSSHError {
     }
 
     internal static let invalidNonceLength = NIOSSHError(type: .invalidNonceLength, diagnostics: nil)
+
+    internal static let excessiveVersionLength = NIOSSHError(type: .excessiveVersionLength, diagnostics: nil)
 
     internal static let invalidEncryptedPacketLength = NIOSSHError(type: .invalidEncryptedPacketLength, diagnostics: nil)
 
@@ -179,6 +181,7 @@ extension NIOSSHError {
             case invalidHostKeyForKeyExchange
             case invalidOpenSSHPublicKey
             case invalidCertificate
+            case excessiveVersionLength
         }
 
         private var base: Base
@@ -195,6 +198,9 @@ extension NIOSSHError {
 
         /// The length of the nonce provided to a cipher is invalid for that cipher.
         public static let invalidNonceLength: ErrorType = .init(.invalidNonceLength)
+
+        /// The length of the nonce provided to a cipher is invalid for that cipher.
+        public static let excessiveVersionLength: ErrorType = .init(.excessiveVersionLength)
 
         /// The encrypted packet received has an invalid length for the negotiated encyption scheme
         public static let invalidEncryptedPacketLength: ErrorType = .init(.invalidEncryptedPacketLength)

Sources/NIOSSH/NIOSSHHandler.swift

@@ -64,7 +64,7 @@ public final class NIOSSHHandler {
         self.pendingChannelInitializations = CircularBuffer(initialCapacity: 4)
         self.pendingGlobalRequests = CircularBuffer(initialCapacity: 4)
         self.pendingGlobalRequestResponses = CircularBuffer(initialCapacity: 4)
-        self.multiplexer = SSHChannelMultiplexer(delegate: self, allocator: allocator, childChannelInitializer: inboundChildChannelInitializer)
+        self.multiplexer = SSHChannelMultiplexer(delegate: self, allocator: allocator, childChannelInitializer: inboundChildChannelInitializer, maximumPacketSize: role.maximumPacketSize)
     }
 }
 

Sources/NIOSSH/Role.swift

@@ -34,4 +34,13 @@ public enum SSHConnectionRole {
             return true
         }
     }
+
+    internal var maximumPacketSize: Int {
+        switch self {
+        case .client(let client):
+            return client.maximumPacketSize
+        case .server(let server):
+            return server.maximumPacketSize
+        }
+    }
 }

Sources/NIOSSH/SSHClientConfiguration.swift

@@ -22,6 +22,9 @@ public struct SSHClientConfiguration {
 
     /// The global request delegate to be used with this client.
     public var globalRequestDelegate: GlobalRequestDelegate
+
+    /// The maximum packet size that this NIOSSH client will accept
+    public var maximumPacketSize = 1 << 17
 
     public init(userAuthDelegate: NIOSSHClientUserAuthenticationDelegate,
                 serverAuthDelegate: NIOSSHClientServerAuthenticationDelegate,

Sources/NIOSSH/SSHPacketParser.swift

@@ -25,15 +25,21 @@ struct SSHPacketParser {
 
     private var buffer: ByteBuffer
     private var state: State
+    private let maximumPacketSize: Int
 
     /// Testing only: the number of bytes we can discard from this buffer.
     internal var _discardableBytes: Int {
         self.buffer.readerIndex
     }
 
-    init(allocator: ByteBufferAllocator) {
+    init(allocator: ByteBufferAllocator, maximumPacketSize: Int = 1 << 17) {
+        // Assert that users don't provide a packet size lower than allowed by spec
+        assert(maximumPacketSize >= 32_768, "Maximum Packet Size is below minimum requirement as specified by RFC 4254")
+        assert(maximumPacketSize <= (1 << 24), "Maximum Packet Size is set abnormally high")
+
         self.buffer = allocator.buffer(capacity: 0)
         self.state = .initialized
+        self.maximumPacketSize = maximumPacketSize
     }
 
     mutating func append(bytes: inout ByteBuffer) {
@@ -71,6 +77,10 @@ struct SSHPacketParser {
             return nil
         case .cleartextWaitingForLength:
             if let length = self.buffer.getInteger(at: self.buffer.readerIndex, as: UInt32.self) {
+                if length >= self.maximumPacketSize {
+                    throw NIOSSHError.invalidEncryptedPacketLength
+                }
+
                 if let message = try self.parsePlaintext(length: length) {
                     self.state = .cleartextWaitingForLength
                     return message
@@ -114,12 +124,26 @@ struct SSHPacketParser {
     private mutating func readVersion() throws -> String? {
         // Looking for a string ending with \r\n
         let slice = self.buffer.readableBytesView
-        if let cr = slice.firstIndex(of: 13), cr.advanced(by: 1) < slice.endIndex, slice[cr.advanced(by: 1)] == 10 {
-            let version = String(decoding: slice[slice.startIndex ..< cr], as: UTF8.self)
-            // read \r\n
-            self.buffer.moveReaderIndex(forwardBy: slice.startIndex.distance(to: cr).advanced(by: 2))
-            return version
+
+        // Prevent the consumed bytes for a version from exceeding 4_096
+        // In practice, if SSH version packets come anywhere near this it's already likely an attack
+        // More data cannot be blindly regarded as malicious though, since this might contain multiple packets
+        let maxIndex = slice.index(slice.startIndex, offsetBy: min(slice.count, 4_096))
+
+        for index in slice.startIndex ..< slice.endIndex {
+            if index > maxIndex {
+                // Does not account for `CRLF`
+                throw NIOSSHError.excessiveVersionLength
+            }
+
+            if slice[index] == 13, index.advanced(by: 1) < slice.endIndex, slice[index.advanced(by: 1)] == 10 {
+                let version = String(decoding: slice[slice.startIndex ..< index], as: UTF8.self)
+                // read \r\n
+                self.buffer.moveReaderIndex(forwardBy: slice.startIndex.distance(to: index).advanced(by: 2))
+                return version
+            }
         }
+
         return nil
     }
 
@@ -132,7 +156,13 @@ struct SSHPacketParser {
         try protection.decryptFirstBlock(&self.buffer)
 
         // This force unwrap is safe because we must have a block size, and a block size is always going to be more than 4 bytes.
-        return self.buffer.getInteger(at: self.buffer.readerIndex)! + UInt32(protection.macBytes)
+        let packetLength = self.buffer.getInteger(at: self.buffer.readerIndex, as: UInt32.self)!
+
+        if packetLength >= self.maximumPacketSize {
+            throw NIOSSHError.invalidEncryptedPacketLength
+        }
+
+        return packetLength + UInt32(protection.macBytes)
     }
 
     private mutating func parsePlaintext(length: UInt32) throws -> SSHMessage? {

Sources/NIOSSH/SSHServerConfiguration.swift

@@ -22,6 +22,9 @@ public struct SSHServerConfiguration {
 
     /// The host keys for this server.
     public var hostKeys: [NIOSSHPrivateKey]
+
+    /// The maximum packet size that this NIOSSH server will accept
+    public var maximumPacketSize = 1 << 17
 
     public init(hostKeys: [NIOSSHPrivateKey], userAuthDelegate: NIOSSHServerUserAuthenticationDelegate, globalRequestDelegate: GlobalRequestDelegate? = nil) {
         self.hostKeys = hostKeys

Tests/NIOSSHTests/EndToEndTests.swift

@@ -79,11 +79,20 @@ class BackToBackEmbeddedChannel {
         try self.server.connect(to: .init(unixDomainSocketPath: "/fake")).wait()
     }
 
-    func configureWithHarness(_ harness: TestHarness) throws {
-        let clientHandler = NIOSSHHandler(role: .client(.init(userAuthDelegate: harness.clientAuthDelegate, serverAuthDelegate: harness.clientServerAuthDelegate, globalRequestDelegate: harness.clientGlobalRequestDelegate)),
+    func configureWithHarness(_ harness: TestHarness, maximumPacketSize: Int? = nil) throws {
+        var clientConfiguration = SSHClientConfiguration(userAuthDelegate: harness.clientAuthDelegate, serverAuthDelegate: harness.clientServerAuthDelegate, globalRequestDelegate: harness.clientGlobalRequestDelegate)
+
+        var serverConfiguration = SSHServerConfiguration(hostKeys: harness.serverHostKeys, userAuthDelegate: harness.serverAuthDelegate, globalRequestDelegate: harness.serverGlobalRequestDelegate)
+
+        if let maximumPacketSize = maximumPacketSize {
+            clientConfiguration.maximumPacketSize = maximumPacketSize
+            serverConfiguration.maximumPacketSize = maximumPacketSize
+        }
+
+        let clientHandler = NIOSSHHandler(role: .client(clientConfiguration),
                                           allocator: self.client.allocator,
                                           inboundChildChannelInitializer: nil)
-        let serverHandler = NIOSSHHandler(role: .server(.init(hostKeys: harness.serverHostKeys, userAuthDelegate: harness.serverAuthDelegate, globalRequestDelegate: harness.serverGlobalRequestDelegate)),
+        let serverHandler = NIOSSHHandler(role: .server(serverConfiguration),
                                           allocator: self.server.allocator) { channel, _ in
             self.activeServerChannels.append(channel)
             channel.closeFuture.whenComplete { _ in self.activeServerChannels.removeAll(where: { $0 === channel }) }
@@ -129,6 +138,8 @@ struct TestHarness {
     var serverGlobalRequestDelegate: GlobalRequestDelegate?
 
     var serverHostKeys: [NIOSSHPrivateKey] = [.init(ed25519Key: .init())]
+
+    var maximumPacketSize: Int? = nil
 }
 
 final class UserEventExpecter: ChannelInboundHandler {
@@ -240,6 +251,28 @@ class EndToEndTests: XCTestCase {
         helper(ChannelSuccessEvent())
         helper(ChannelFailureEvent())
     }
+
+    /// This test validates that all the channel requests roiund-trip appropriately.
+    func testChannelRejectsHugePacketsRequests() throws {
+        XCTAssertNoThrow(try self.channel.configureWithHarness(TestHarness(), maximumPacketSize: 32_768))
+        XCTAssertNoThrow(try self.channel.activate())
+        XCTAssertNoThrow(try self.channel.interactInMemory())
+
+        // Create a channel.
+        let clientChannel = try self.channel.createNewChannel()
+        XCTAssertNoThrow(try self.channel.interactInMemory())
+        guard let serverChannel = self.channel.activeServerChannels.first else {
+            XCTFail("Server channel not created")
+            return
+        }
+
+        let userEventRecorder = UserEventExpecter()
+        XCTAssertNoThrow(try serverChannel.pipeline.addHandler(userEventRecorder).wait())
+
+        let hugeCommand = String(repeating: "a", count: 32_768)
+        try clientChannel.triggerUserOutboundEvent(SSHChannelRequestEvent.ExecRequest(command: hugeCommand, wantReply: true)).wait()
+        XCTAssertThrowsError(try self.channel.interactInMemory())
+    }
 
     func testGlobalRequestWithDefaultDelegate() throws {
         XCTAssertNoThrow(try self.channel.configureWithHarness(TestHarness()))

Tests/NIOSSHTests/SSHPacketParserTests.swift

@@ -142,6 +142,15 @@ final class SSHPacketParserTests: XCTestCase {
         // Now we should have cleared up.
         XCTAssertEqual(parser._discardableBytes, 0)
     }
+
+    func testMaximumPacketSizeInVersion() throws {
+        var parser = SSHPacketParser(allocator: ByteBufferAllocator(), maximumPacketSize: 1 << 15)
+        let longVersionString = String(repeating: "z", count: 5000)
+        var version = ByteBuffer.of(string: longVersionString + "\r\n")
+        parser.append(bytes: &version)
+
+        XCTAssertThrowsError(try parser.nextPacket())
+    }
 }
 
 extension ByteBuffer {