Limit buffered state

Benchmarks/Thresholds/6.1/NIOSSHBenchmarks.ManySmallCommandsPerConnection.p90.json

@@ -1,3 +1,3 @@
 {
-  "mallocCountTotal": 190655
+  "mallocCountTotal": 190652
 }

Benchmarks/Thresholds/6.1/NIOSSHBenchmarks.OneCommandPerConnection.p90.json

@@ -1,3 +1,3 @@
 {
-  "mallocCountTotal": 792051
+  "mallocCountTotal": 789051
 }

Benchmarks/Thresholds/6.2/NIOSSHBenchmarks.OneCommandPerConnection.p90.json

@@ -1,3 +1,3 @@
 {
-  "mallocCountTotal": 752035
+  "mallocCountTotal": 751035
 }

Benchmarks/Thresholds/6.3/NIOSSHBenchmarks.ManySmallCommandsPerConnection.p90.json

@@ -1,3 +1,3 @@
 {
-  "mallocCountTotal": 190567
+  "mallocCountTotal": 190566
 }

Benchmarks/Thresholds/6.3/NIOSSHBenchmarks.OneCommandPerConnection.p90.json

@@ -1,3 +1,3 @@
 {
-  "mallocCountTotal": 755999
+  "mallocCountTotal": 754999
 }

Benchmarks/Thresholds/6.3/NIOSSHBenchmarks.StreamingLargeMessageInSmallChunks.p90.json

@@ -1,3 +1,3 @@
 {
-  "mallocCountTotal": 40736
+  "mallocCountTotal": 40735
 }

Sources/NIOSSH/Child Channels/SSHChildChannel.swift

@@ -490,8 +490,8 @@ extension SSHChildChannel: Channel, ChannelCore {
                 recipientChannel: self.state.remoteChannelIdentifier!,
                 senderChannel: self.state.localChannelIdentifier,
                 initialWindowSize: self.windowManager.targetWindowSize,
-                maximumPacketSize: 1 << 24
-            )  // This is a weirdly hard-coded choice.
+                maximumPacketSize: Constants.maximumChannelPacketSize
+            )
             self.processOutboundMessage(.channelOpenConfirmation(message), promise: nil)
             self.writePendingToMultiplexer()
         } else if !self.state.isClosed {
@@ -500,7 +500,7 @@ extension SSHChildChannel: Channel, ChannelCore {
                 type: .init(self.type!),
                 senderChannel: self.state.localChannelIdentifier,
                 initialWindowSize: self.windowManager.targetWindowSize,
-                maximumPacketSize: 1 << 24
+                maximumPacketSize: Constants.maximumChannelPacketSize
             )
             self.processOutboundMessage(.channelOpen(message), promise: nil)
             self.writePendingToMultiplexer()

Sources/NIOSSH/Connection State Machine/Operations/AcceptsVersionMessages.swift

@@ -15,62 +15,12 @@
 protocol AcceptsVersionMessages {}
 
 extension AcceptsVersionMessages {
-    func receiveVersionMessage(_ banner: String, role: SSHConnectionRole) throws {
-        try self.validateBanner(banner, role: role)
-    }
-
-    // From RFC 4253:
-    //
-    // > Protocol Version Exchange
-    // >
-    // > When the connection has been established, both sides MUST send an
-    // > identification string.  This identification string MUST be
-    // >   SSH-protoversion-softwareversion SP comments CR LF
-    // > Since the protocol being defined in this set of documents is version
-    // > 2.0, the 'protoversion' MUST be "2.0".  The 'comments' string is
-    // > OPTIONAL.  If the 'comments' string is included, a 'space' character
-    // > (denoted above as SP, ASCII 32) MUST separate the 'softwareversion'
-    // > and 'comments' strings.  The identification MUST be terminated by a
-    // > single Carriage Return (CR) and a single Line Feed (LF) character
-    // > (ASCII 13 and 10, respectively).  Implementers who wish to maintain
-    // > compatibility with older, undocumented versions of this protocol may
-    // > want to process the identification string without expecting the
-    // > presence of the carriage return character for reasons described in
-    // > Section 5 of this document.  The null character MUST NOT be sent.
-    // > The maximum length of the string is 255 characters, including the
-    // > Carriage Return and Line Feed.
-    // >
-    // > The part of the identification string preceding the Carriage Return
-    // > and Line Feed is used in the Diffie-Hellman key exchange (see Section
-    // > 8).
-    // >
-    // > The server MAY send other lines of data before sending the version
-    // > string.  Each line SHOULD be terminated by a Carriage Return and Line
-    // > Feed.  Such lines MUST NOT begin with "SSH-", and SHOULD be encoded
-    // > in ISO-10646 UTF-8 [RFC3629] (language is not specified).  Clients
-    // > MUST be able to process such lines.  Such lines MAY be silently
-    // > ignored, or MAY be displayed to the client user.  If they are
-    // > displayed, control character filtering, as discussed in [SSH-ARCH],
-    // > SHOULD be used.  The primary use of this feature is to allow TCP-
-    // > wrappers to display an error message before disconnecting
-    private func validateBanner(_ banner: String, role: SSHConnectionRole) throws {
-        switch role {
-        case .client:
-            // split by \n
-            let lineFeed = UInt8(ascii: "\n")
-            for line in banner.utf8.split(separator: lineFeed) {
-                if try self.validateVersion(line) {
-                    return
-                }
-            }
-            throw NIOSSHError.protocolViolation(protocolName: "version exchange", violation: "version string not found")
-        case .server:
-            guard try self.validateVersion(Substring(banner).utf8) else {
-                throw NIOSSHError.protocolViolation(
-                    protocolName: "version exchange",
-                    violation: "version string not found"
-                )
-            }
+    func receiveVersionMessage(_ banner: String) throws {
+        guard try self.validateVersion(Substring(banner).utf8) else {
+            throw NIOSSHError.protocolViolation(
+                protocolName: "version exchange",
+                violation: "version string not found"
+            )
         }
     }
 

Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift

@@ -115,7 +115,7 @@ struct SSHConnectionStateMachine {
 
                     switch message {
                     case .version(let version):
-                        try state.receiveVersionMessage(version, role: state.role)
+                        try state.receiveVersionMessage(version)
                         let newState = KeyExchangeState(
                             sentVersionState: state,
                             allocator: allocator,

Sources/NIOSSH/Constants.swift

@@ -15,6 +15,11 @@
 public enum Constants: Sendable {
     static let version = "SSH-2.0-SwiftNIOSSH_1.0"
 
+    /// The maximum size, in bytes, of a channel data payload that we advertise we are willing to
+    /// receive (the "maximum packet size" of an SSH channel, RFC 4254 §5.1).
+    /// - Note: "This is a weirdly hard-coded choice."
+    static let maximumChannelPacketSize: UInt32 = 1 << 24
+
     public static let bundledTransportProtectionSchemes: [(NIOSSHTransportProtection & _NIOSSHSendableMetatype).Type] =
         [
             AES256GCMOpenSSHTransportProtection.self, AES128GCMOpenSSHTransportProtection.self,

Sources/NIOSSH/SSHPacketParser.swift

@@ -23,11 +23,32 @@ struct SSHPacketParser {
         case encryptedWaitingForBytes(UInt32, NIOSSHTransportProtection)
     }
 
+    /// RFC 4253 §4.2 limits the SSH version string ("SSH-...") to 255 bytes, includes CR and LF.
+    static var maximumVersionStringLength: Int { 255 }
+
+    /// This bounds each preamble line a client may receive before the version string, matching OpenSSH.
+    static var maximumBannerLineLength: Int { 8192 }
+
+    /// The maximum number of preamble lines a client accepts before the version string, matching OpenSSH.
+    static var maximumPreambleLineCount: Int { 1024 }
+
+    /// The maximum length of a cleartext (pre-encryption) packet, i.e. during version and key
+    /// exchange. The encrypted path negotiates its own (larger) packet bound.
+    static var maximumPlaintextPacketLength: UInt32 { 256 * 1024 }
+
+    /// The maximum length of an encrypted packet, i.e. its `packet_length` field once key
+    /// exchange has completed. Includes headroom for the SSH framing and padding (RFC 4253 §6).
+    static var maximumEncryptedPacketLength: UInt32 { Constants.maximumChannelPacketSize + 1024 }
+
     private let isServer: Bool
     private var buffer: ByteBuffer
     private var state: State
     private(set) var sequenceNumber: UInt32
 
+    /// The number of preamble lines a client has discarded while waiting for the identification
+    /// string, bounded by `maximumPreambleLineCount`.
+    private var preambleLineCount: Int
+
     /// Testing only: the number of bytes we can discard from this buffer.
     internal var _discardableBytes: Int {
         self.buffer.readerIndex
@@ -38,6 +59,7 @@ struct SSHPacketParser {
         self.buffer = allocator.buffer(capacity: 0)
         self.state = .initialized
         self.sequenceNumber = 0
+        self.preambleLineCount = 0
     }
 
     mutating func append(bytes: inout ByteBuffer) {
@@ -119,17 +141,57 @@ struct SSHPacketParser {
         }
     }
 
+    // From RFC 4253:
+    //
+    // > Protocol Version Exchange
+    // >
+    // > When the connection has been established, both sides MUST send an
+    // > identification string.  This identification string MUST be
+    // >   SSH-protoversion-softwareversion SP comments CR LF
+    // > Since the protocol being defined in this set of documents is version
+    // > 2.0, the 'protoversion' MUST be "2.0".  The 'comments' string is
+    // > OPTIONAL.  If the 'comments' string is included, a 'space' character
+    // > (denoted above as SP, ASCII 32) MUST separate the 'softwareversion'
+    // > and 'comments' strings.  The identification MUST be terminated by a
+    // > single Carriage Return (CR) and a single Line Feed (LF) character
+    // > (ASCII 13 and 10, respectively).  Implementers who wish to maintain
+    // > compatibility with older, undocumented versions of this protocol may
+    // > want to process the identification string without expecting the
+    // > presence of the carriage return character for reasons described in
+    // > Section 5 of this document.  The null character MUST NOT be sent.
+    // > The maximum length of the string is 255 characters, including the
+    // > Carriage Return and Line Feed.
+    // >
+    // > The part of the identification string preceding the Carriage Return
+    // > and Line Feed is used in the Diffie-Hellman key exchange (see Section
+    // > 8).
+    // >
+    // > The server MAY send other lines of data before sending the version
+    // > string.  Each line SHOULD be terminated by a Carriage Return and Line
+    // > Feed.  Such lines MUST NOT begin with "SSH-", and SHOULD be encoded
+    // > in ISO-10646 UTF-8 [RFC3629] (language is not specified).  Clients
+    // > MUST be able to process such lines.  Such lines MAY be silently
+    // > ignored, or MAY be displayed to the client user.  If they are
+    // > displayed, control character filtering, as discussed in [SSH-ARCH],
+    // > SHOULD be used.  The primary use of this feature is to allow TCP-
+    // > wrappers to display an error message before disconnecting
     private mutating func readVersion() throws -> String? {
         let carriageReturn = UInt8(ascii: "\r")
         let lineFeed = UInt8(ascii: "\n")
+        let sshPrefix = "SSH-".utf8
 
         // Per RFC 4253 §4.2:
-        //  The server MAY send other lines of data before sending the version string.
+        // The server MAY send other lines of data before sending the version string.
         // This means that server does not expect any lines before version so we will return all data before first line feed
         if self.isServer {
             // Looking for a string ending with \r\n
             let slice = self.buffer.readableBytesView
             if let lfIndex = slice.firstIndex(of: lineFeed), lfIndex < slice.endIndex {
+                // This must be the version: check the length. The version itself will be validated later.
+                try self.enforceIdentificationAndBannerLineLimit(
+                    length: slice.startIndex.distance(to: lfIndex) + 1,  // add one for line feed
+                    isVersion: true
+                )
                 // Guard the `-1` lookup: when the LF is the very first byte of
                 // the slice (e.g., a client sends a bare LF as its first byte
                 // after the TCP handshake), `lfIndex.advanced(by: -1)` would
@@ -143,24 +205,76 @@ struct SSHPacketParser {
                 return version
             }
         } else {
-            // Search for version line, which starts with "SSH-". Lines without this prefix may come before the version line.
-            var slice = self.buffer.readableBytesView
-            let startIndex = slice.startIndex
-            while let lfIndex = slice.firstIndex(of: lineFeed), lfIndex < slice.endIndex {
-                if slice.starts(with: "SSH-".utf8) {
+            // Search for version line, which starts with "SSH-". Lines without this
+            // prefix may come before the version line. Discard them to avoid accumulating state.
+            while true {
+                // Find the next line and check its length.
+                let slice = self.buffer.readableBytesView
+                guard let lfIndex = slice.firstIndex(of: lineFeed) else {
+                    break
+                }
+                let lineLength = slice.startIndex.distance(to: lfIndex)
+                let isVersion = slice.starts(with: sshPrefix)
+                try self.enforceIdentificationAndBannerLineLimit(
+                    length: lineLength + 1,  // add one for line feed
+                    isVersion: isVersion
+                )
+
+                // Per RFC other lines in the preamble are not allowed to start with "SSH-".
+                // This must be the version.
+                if isVersion {
                     let versionEndIndex =
                         slice[lfIndex.advanced(by: -1)] == carriageReturn ? lfIndex.advanced(by: -1) : lfIndex
                     let version = String(decoding: slice[slice.startIndex..<versionEndIndex], as: UTF8.self)
-                    self.buffer.moveReaderIndex(forwardBy: startIndex.distance(to: lfIndex).advanced(by: 1))
+                    self.buffer.moveReaderIndex(forwardBy: lineLength + 1)
                     return version
-                } else {
-                    slice = slice[slice.index(after: lfIndex)...]
                 }
+
+                // A preamble line will be silently ignored: count it and discard it so it cannot accumulate.
+                self.preambleLineCount += 1
+                if self.preambleLineCount > Self.maximumPreambleLineCount {
+                    throw NIOSSHError.protocolViolation(
+                        protocolName: "version exchange",
+                        violation: "received more than \(Self.maximumPreambleLineCount) lines before the version string"
+                    )
+                }
+                // This just advances the reader index. The calling function (`nextPacket`) will
+                // free up bytes with `reclaimBytes` when a threshold of read bytes is reached.
+                self.buffer.moveReaderIndex(forwardBy: lineLength + 1)
             }
         }
+
+        // Ensure the buffered data has not grown past the limit, e.g., if the peer sends data without an LF.
+        let pending = self.buffer.readableBytesView
+        try self.enforceIdentificationAndBannerLineLimit(
+            length: pending.count,
+            isVersion: pending.starts(with: sshPrefix)
+        )
         return nil
     }
 
+    /// Enforces the version-exchange line-length limits used by ``readVersion()``. Identification lines are
+    /// limited to ``maximumIdentificationStringLength``, others to ``maximumBannerLineLength``.
+    ///
+    /// - Throws: NIOSSHError.protocolViolation if the line crosses its length limit.
+    private func enforceIdentificationAndBannerLineLimit(length: Int, isVersion: Bool) throws {
+        if isVersion {
+            if length > Self.maximumVersionStringLength {
+                throw NIOSSHError.protocolViolation(
+                    protocolName: "version exchange",
+                    violation: "version line exceeded \(Self.maximumVersionStringLength) bytes"
+                )
+            }
+        } else {
+            if length > Self.maximumBannerLineLength {
+                throw NIOSSHError.protocolViolation(
+                    protocolName: "version exchange",
+                    violation: "banner line exceeded \(Self.maximumBannerLineLength) bytes"
+                )
+            }
+        }
+    }
+
     private mutating func decryptLength(protection: NIOSSHTransportProtection) throws -> UInt32? {
         let blockSize = protection.cipherBlockSize
         guard self.buffer.readableBytes >= blockSize else {
@@ -170,11 +284,29 @@ 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 length = self.buffer.getInteger(at: self.buffer.readerIndex, as: UInt32.self)!
+
+        // Reject an oversized encrypted packet before we commit to buffering toward it.
+        guard length <= Self.maximumEncryptedPacketLength else {
+            throw NIOSSHError.protocolViolation(
+                protocolName: "transport",
+                violation: "encrypted packet length \(length) exceeds \(Self.maximumEncryptedPacketLength) bytes"
+            )
+        }
+
+        return length + UInt32(protection.macBytes)
     }
 
     private mutating func parsePlaintext(length: UInt32) throws -> SSHMessage? {
-        try self.buffer.rewindReaderOnError { buffer in
+        // Reject an oversized cleartext packet before committing to buffering it.
+        guard length <= Self.maximumPlaintextPacketLength else {
+            throw NIOSSHError.protocolViolation(
+                protocolName: "transport",
+                violation: "cleartext packet length \(length) exceeds \(Self.maximumPlaintextPacketLength) bytes"
+            )
+        }
+
+        return try self.buffer.rewindReaderOnError { buffer in
             guard var buffer = buffer.readSlice(length: Int(length) + MemoryLayout<UInt32>.size) else {
                 return nil
             }