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

Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift

@@ -34,7 +34,9 @@ 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
 
-    init(delegate: SSHMultiplexerDelegate, allocator: ByteBufferAllocator, childChannelInitializer: SSHChildChannel.Initializer?) {
+    private let maximumPacketSize: Int
+
+    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(self.maximumPacketSize),
+                                      initialOutboundWindowSize: 0,
+                                      maximumPacketSize: self.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

@@ -98,6 +98,8 @@ final class SSHChildChannel {
 
     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
@@ -470,15 +476,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(self.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(self.maximumPacketSize))
             self.processOutboundMessage(.channelOpen(message), promise: nil)
             self.writePendingToMultiplexer()
         } else {

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

@@ -691,7 +691,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

@@ -44,6 +44,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)
 
     internal static let invalidDecryptedPlaintextLength = NIOSSHError(type: .invalidDecryptedPlaintextLength, diagnostics: nil)
@@ -179,6 +181,7 @@ extension NIOSSHError {
             case invalidHostKeyForKeyExchange
             case invalidOpenSSHPublicKey
             case invalidCertificate
+            case excessiveVersionLength
         }
 
         private var base: Base
@@ -196,6 +199,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 version length sent by a client was excessively large.
+        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

@@ -23,6 +23,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 = SSHPacketParser.defaultMaximumPacketSize
+
     public init(userAuthDelegate: NIOSSHClientUserAuthenticationDelegate,
                 serverAuthDelegate: NIOSSHClientServerAuthenticationDelegate,
                 globalRequestDelegate: GlobalRequestDelegate? = nil) {

Sources/NIOSSH/SSHPacketParser.swift

@@ -25,15 +25,22 @@ struct SSHPacketParser {
 
     private var buffer: ByteBuffer
     private var state: State
+    private let maximumPacketSize: Int
+    internal static let defaultMaximumPacketSize = 1 << 17
 
     /// 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 = defaultMaximumPacketSize) {
+        // Assert that users don't provide a packet size lower than allowed by spec
+        precondition(maximumPacketSize >= 32768, "Maximum Packet Size is below minimum requirement as specified by RFC 4254")
+        precondition(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 +78,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
@@ -111,15 +122,30 @@ struct SSHPacketParser {
         }
     }
 
+    internal static let maximumAllowedVersionSize = 4096
     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 the defined maximum allowed size
+        // 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, Self.maximumAllowedVersionSize))
+
+        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 +158,14 @@ 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)!
+        let decryptedLength = packetLength + UInt32(protection.macBytes)
+
+        if decryptedLength >= self.maximumPacketSize {
+            throw NIOSSHError.invalidEncryptedPacketLength
+        }
+
+        return decryptedLength
     }
 
     private mutating func parsePlaintext(length: UInt32) throws -> SSHMessage? {

Sources/NIOSSH/SSHServerConfiguration.swift

@@ -23,6 +23,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 = SSHPacketParser.defaultMaximumPacketSize
+
     /// The ssh banner to display to clients upon authentication
     public var banner: UserAuthBanner?