Fixes client mode version parsing

Motivation: Server may send additional lines of data before version, but since we use version as part of key exchange, we need to filter out those lines, otherwise it will fail key exchange. Modifications: - Strip out lines of data before version in client role - Update tests
apple · Lukasa · Jul 21, 2023 · Jul 21, 2023 · Jul 21, 2023 · 6882f47e933a96fcd1a9f6b8c22d07eacb484c4c
commit 6882f47e933a96fcd1a9f6b8c22d07eacb484c4c
diff --git a/Sources/NIOSSH/Connection State Machine/States/SentVersionState.swift b/Sources/NIOSSH/Connection State Machine/States/SentVersionState.swift
@@ -35,7 +35,7 @@ extension SSHConnectionStateMachine {
             self.serializer = state.serializer
             self.protectionSchemes = state.protectionSchemes
 
-            self.parser = SSHPacketParser(allocator: allocator)
+            self.parser = SSHPacketParser(isServer: self.role.isServer, allocator: allocator)
             self.allocator = allocator
         }
 

diff --git a/Sources/NIOSSH/SSHPacketParser.swift b/Sources/NIOSSH/SSHPacketParser.swift
@@ -23,6 +23,7 @@ struct SSHPacketParser {
         case encryptedWaitingForBytes(UInt32, NIOSSHTransportProtection)
     }
 
+    private let isServer: Bool
     private var buffer: ByteBuffer
     private var state: State
     private(set) var sequenceNumber: UInt32
@@ -32,7 +33,8 @@ struct SSHPacketParser {
         self.buffer.readerIndex
     }
 
-    init(allocator: ByteBufferAllocator) {
+    init(isServer: Bool, allocator: ByteBufferAllocator) {
+        self.isServer = isServer
         self.buffer = allocator.buffer(capacity: 0)
         self.state = .initialized
         self.sequenceNumber = 0
@@ -121,18 +123,31 @@ struct SSHPacketParser {
         let carriageReturn = UInt8(ascii: "\r")
         let lineFeed = UInt8(ascii: "\n")
 
-        // Search for version line, which starts with "SSH-". Lines without this prefix may come before the version line.
-        var slice = self.buffer.readableBytesView
-        while let lfIndex = slice.firstIndex(of: lineFeed), lfIndex < slice.endIndex {
-            if slice.starts(with: "SSH-".utf8) {
-                // Return all data upto the last LF we found, excluding the last [CR]LF.
-                slice = self.buffer.readableBytesView
+        // Per RFC 4253 §4.2:
+        //  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 {
                 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: slice.startIndex.distance(to: lfIndex).advanced(by: 1))
                 return version
-            } else {
-                slice = slice[slice.index(after: lfIndex)...]
+            }
+        } 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) {
+                    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))
+                    return version
+                } else {
+                    slice = slice[slice.index(after: lfIndex)...]
+                }
             }
         }
         return nil

diff --git a/Sources/NIOSSHClient/ExecHandler.swift b/Sources/NIOSSHClient/ExecHandler.swift
@@ -48,7 +48,7 @@ final class ExampleExecHandler: ChannelDuplexHandler {
             DispatchQueue(label: "pipe bootstrap").async {
                 bootstrap.channelOption(ChannelOptions.allowRemoteHalfClosure, value: true).channelInitializer { channel in
                     channel.pipeline.addHandler(theirs)
-                }.withPipes(inputDescriptor: 0, outputDescriptor: 1).whenComplete { result in
+                }.takingOwnershipOfDescriptors(input: 0, output: 1).whenComplete { result in
                     switch result {
                     case .success:
                         // We need to exec a thing.

diff --git a/Sources/NIOSSHServer/ExecHandler.swift b/Sources/NIOSSHServer/ExecHandler.swift
@@ -120,7 +120,7 @@ final class ExampleExecHandler: ChannelDuplexHandler {
                     .channelOption(ChannelOptions.allowRemoteHalfClosure, value: true)
                     .channelInitializer { pipeChannel in
                         pipeChannel.pipeline.addHandler(theirs)
-                    }.withPipes(inputDescriptor: dup(outPipe.fileHandleForReading.fileDescriptor), outputDescriptor: dup(inPipe.fileHandleForWriting.fileDescriptor)).wait()
+                    }.takingOwnershipOfDescriptors(input: dup(outPipe.fileHandleForReading.fileDescriptor), output: dup(inPipe.fileHandleForWriting.fileDescriptor)).wait()
 
                 // Ok, great, we've sorted stdout and stdin. For stderr we need a different strategy: we just park a thread for this.
                 DispatchQueue(label: "stderrorwhatever").async {

diff --git a/Tests/NIOSSHTests/SSHEncryptedTrafficTests.swift b/Tests/NIOSSHTests/SSHEncryptedTrafficTests.swift
@@ -25,7 +25,7 @@ final class SSHEncryptedTrafficTests: XCTestCase {
 
     override func setUp() {
         self.serializer = SSHPacketSerializer()
-        self.parser = SSHPacketParser(allocator: .init())
+        self.parser = SSHPacketParser(isServer: false, allocator: .init())
 
         self.assertPacketRoundTrips(.version("SSH-2.0-SwiftSSH_1.0"))
     }

diff --git a/Tests/NIOSSHTests/SSHPackerSerializerTests.swift b/Tests/NIOSSHTests/SSHPackerSerializerTests.swift
@@ -51,7 +51,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         let message = SSHMessage.disconnect(.init(reason: 42, description: "description", tag: "tag"))
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 
@@ -74,7 +74,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         let message = SSHMessage.serviceRequest(.init(service: "ssh-userauth"))
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 
@@ -97,7 +97,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         let message = SSHMessage.serviceAccept(.init(service: "ssh-userauth"))
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 
@@ -133,7 +133,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         ))
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 
@@ -165,7 +165,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         let message = SSHMessage.keyExchangeInit(.init(publicKey: ByteBuffer.of(bytes: [42])))
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 
@@ -193,7 +193,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         ))
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 
@@ -226,7 +226,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         let message = SSHMessage.newKeys
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 
@@ -247,7 +247,7 @@ final class SSHPacketSerializerTests: XCTestCase {
         let message = SSHMessage.newKeys
         let allocator = ByteBufferAllocator()
         var serializer = SSHPacketSerializer()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
 
         self.runVersionHandshake(serializer: &serializer, parser: &parser)
 

diff --git a/Tests/NIOSSHTests/SSHPacketParserTests.swift b/Tests/NIOSSHTests/SSHPacketParserTests.swift
@@ -38,7 +38,7 @@ final class SSHPacketParserTests: XCTestCase {
     }
 
     func testReadVersion() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
 
         var part1 = ByteBuffer.of(string: "SSH-2.0-")
         parser.append(bytes: &part1)
@@ -57,8 +57,8 @@ final class SSHPacketParserTests: XCTestCase {
         }
     }
 
-    func testReadVersionWithExtraLines() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+    func testReadVersionWithExtraLinesOnClient() throws {
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
 
         var part1 = ByteBuffer.of(string: "xxxx\r\nyyyy\r\nSSH-2.0-")
         parser.append(bytes: &part1)
@@ -70,14 +70,33 @@ final class SSHPacketParserTests: XCTestCase {
 
         switch try parser.nextPacket() {
         case .version(let string):
-            XCTAssertEqual(string, "xxxx\r\nyyyy\r\nSSH-2.0-OpenSSH_7.9")
+            XCTAssertEqual(string, "SSH-2.0-OpenSSH_7.9")
+        default:
+            XCTFail("Expecting .version")
+        }
+    }
+
+    func testReadVersionWithExtraLinesOnServer() throws {
+        var parser = SSHPacketParser(isServer: true, allocator: ByteBufferAllocator())
+
+        var part1 = ByteBuffer.of(string: "xx")
+        parser.append(bytes: &part1)
+
+        XCTAssertNil(try parser.nextPacket())
+
+        var part2 = ByteBuffer.of(string: "xx\r\nyyyy\r\nSSH-2.0-OpenSSH_7.9\r\n")
+        parser.append(bytes: &part2)
+
+        switch try parser.nextPacket() {
+        case .version(let string):
+            XCTAssertEqual(string, "xxxx")
         default:
             XCTFail("Expecting .version")
         }
     }
 
     func testReadVersionWithoutCarriageReturn() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
 
         var part1 = ByteBuffer.of(string: "SSH-2.0-")
         parser.append(bytes: &part1)
@@ -95,8 +114,8 @@ final class SSHPacketParserTests: XCTestCase {
         }
     }
 
-    func testReadVersionWithExtraLinesWithoutCarriageReturn() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+    func testReadVersionWithExtraLinesWithoutCarriageReturnOnClient() throws {
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
 
         var part1 = ByteBuffer.of(string: "xxxx\nyyyy\nSSH-2.0-")
         parser.append(bytes: &part1)
@@ -108,14 +127,33 @@ final class SSHPacketParserTests: XCTestCase {
 
         switch try parser.nextPacket() {
         case .version(let string):
-            XCTAssertEqual(string, "xxxx\nyyyy\nSSH-2.0-OpenSSH_7.4")
+            XCTAssertEqual(string, "SSH-2.0-OpenSSH_7.4")
+        default:
+            XCTFail("Expecting .version")
+        }
+    }
+
+    func testReadVersionWithExtraLinesWithoutCarriageReturnOnServer() throws {
+        var parser = SSHPacketParser(isServer: true, allocator: ByteBufferAllocator())
+
+        var part1 = ByteBuffer.of(string: "xx")
+        parser.append(bytes: &part1)
+
+        XCTAssertNil(try parser.nextPacket())
+
+        var part2 = ByteBuffer.of(string: "xx\nyyyy\nSSH-2.0-OpenSSH_7.4\n")
+        parser.append(bytes: &part2)
+
+        switch try parser.nextPacket() {
+        case .version(let string):
+            XCTAssertEqual(string, "xxxx")
         default:
             XCTFail("Expecting .version")
         }
     }
 
     func testBinaryInParts() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
         self.feedVersion(to: &parser)
 
         var part1 = ByteBuffer.of(bytes: [0, 0, 0])
@@ -148,7 +186,7 @@ final class SSHPacketParserTests: XCTestCase {
     }
 
     func testBinaryFull() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
         self.feedVersion(to: &parser)
 
         var part1 = ByteBuffer.of(bytes: [0, 0, 0, 28, 10, 5, 0, 0, 0, 12, 115, 115, 104, 45, 117, 115, 101, 114, 97, 117, 116, 104, 42, 111, 216, 12, 226, 248, 144, 175, 157, 207])
@@ -164,7 +202,7 @@ final class SSHPacketParserTests: XCTestCase {
     }
 
     func testBinaryTwoMessages() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
         self.feedVersion(to: &parser)
 
         var part = ByteBuffer.of(bytes: [0, 0, 0, 28, 10, 5, 0, 0, 0, 12, 115, 115, 104, 45, 117, 115, 101, 114, 97, 117, 116, 104, 42, 111, 216, 12, 226, 248, 144, 175, 157, 207, 0, 0, 0, 28, 10, 5, 0, 0, 0, 12, 115, 115, 104, 45, 117, 115, 101, 114, 97, 117, 116, 104, 42, 111, 216, 12, 226, 248, 144, 175, 157, 207])
@@ -187,7 +225,7 @@ final class SSHPacketParserTests: XCTestCase {
     }
 
     func testWeReclaimStorage() throws {
-        var parser = SSHPacketParser(allocator: ByteBufferAllocator())
+        var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
         self.feedVersion(to: &parser)
         XCTAssertNoThrow(try parser.nextPacket())
 
@@ -213,7 +251,7 @@ final class SSHPacketParserTests: XCTestCase {
 
     func testSequencePreservedBetweenPlainAndCypher() throws {
         let allocator = ByteBufferAllocator()
-        var parser = SSHPacketParser(allocator: allocator)
+        var parser = SSHPacketParser(isServer: false, allocator: allocator)
         self.feedVersion(to: &parser)
 
         var part = ByteBuffer(bytes: [0, 0, 0, 12, 10, 21, 41, 114, 125, 250, 3, 79, 3, 217, 166, 136])