Skip to content

Commit ded5e5c

Browse files
artemredkinartemredkin
authored
Fixes client mode version parsing (#153)
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
1 parent d7279ea commit ded5e5c

File tree

7 files changed

+87
-34
lines changed

7 files changed

+87
-34
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ extension SSHConnectionStateMachine {
3535
self.serializer = state.serializer
3636
self.protectionSchemes = state.protectionSchemes
3737

38-
self.parser = SSHPacketParser(allocator: allocator)
38+
self.parser = SSHPacketParser(isServer: self.role.isServer, allocator: allocator)
3939
self.allocator = allocator
4040
}
4141

‎Sources/NIOSSH/SSHPacketParser.swift‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ struct SSHPacketParser {
2323
case encryptedWaitingForBytes(UInt32, NIOSSHTransportProtection)
2424
}
2525

26+
private let isServer: Bool
2627
private var buffer: ByteBuffer
2728
private var state: State
2829
private(set) var sequenceNumber: UInt32
@@ -32,7 +33,8 @@ struct SSHPacketParser {
3233
self.buffer.readerIndex
3334
}
3435

35-
init(allocator: ByteBufferAllocator) {
36+
init(isServer: Bool, allocator: ByteBufferAllocator) {
37+
self.isServer = isServer
3638
self.buffer = allocator.buffer(capacity: 0)
3739
self.state = .initialized
3840
self.sequenceNumber = 0
@@ -121,18 +123,31 @@ struct SSHPacketParser {
121123
let carriageReturn = UInt8(ascii: "\r")
122124
let lineFeed = UInt8(ascii: "\n")
123125

124-
// Search for version line, which starts with "SSH-". Lines without this prefix may come before the version line.
125-
var slice = self.buffer.readableBytesView
126-
while let lfIndex = slice.firstIndex(of: lineFeed), lfIndex < slice.endIndex {
127-
if slice.starts(with: "SSH-".utf8) {
128-
// Return all data upto the last LF we found, excluding the last [CR]LF.
129-
slice = self.buffer.readableBytesView
126+
// Per RFC 4253 §4.2:
127+
// The server MAY send other lines of data before sending the version string.
128+
// This means that server does not expect any lines before version so we will return all data before first line feed
129+
if self.isServer {
130+
// Looking for a string ending with \r\n
131+
let slice = self.buffer.readableBytesView
132+
if let lfIndex = slice.firstIndex(of: lineFeed), lfIndex < slice.endIndex {
130133
let versionEndIndex = slice[lfIndex.advanced(by: -1)] == carriageReturn ? lfIndex.advanced(by: -1) : lfIndex
131134
let version = String(decoding: slice[slice.startIndex ..< versionEndIndex], as: UTF8.self)
132135
self.buffer.moveReaderIndex(forwardBy: slice.startIndex.distance(to: lfIndex).advanced(by: 1))
133136
return version
134-
} else {
135-
slice = slice[slice.index(after: lfIndex)...]
137+
}
138+
} else {
139+
// Search for version line, which starts with "SSH-". Lines without this prefix may come before the version line.
140+
var slice = self.buffer.readableBytesView
141+
let startIndex = slice.startIndex
142+
while let lfIndex = slice.firstIndex(of: lineFeed), lfIndex < slice.endIndex {
143+
if slice.starts(with: "SSH-".utf8) {
144+
let versionEndIndex = slice[lfIndex.advanced(by: -1)] == carriageReturn ? lfIndex.advanced(by: -1) : lfIndex
145+
let version = String(decoding: slice[slice.startIndex ..< versionEndIndex], as: UTF8.self)
146+
self.buffer.moveReaderIndex(forwardBy: startIndex.distance(to: lfIndex).advanced(by: 1))
147+
return version
148+
} else {
149+
slice = slice[slice.index(after: lfIndex)...]
150+
}
136151
}
137152
}
138153
return nil

‎Sources/NIOSSHClient/ExecHandler.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ final class ExampleExecHandler: ChannelDuplexHandler {
4848
DispatchQueue(label: "pipe bootstrap").async {
4949
bootstrap.channelOption(ChannelOptions.allowRemoteHalfClosure, value: true).channelInitializer { channel in
5050
channel.pipeline.addHandler(theirs)
51-
}.withPipes(inputDescriptor: 0, outputDescriptor: 1).whenComplete { result in
51+
}.takingOwnershipOfDescriptors(input: 0, output: 1).whenComplete { result in
5252
switch result {
5353
case .success:
5454
// We need to exec a thing.

‎Sources/NIOSSHServer/ExecHandler.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ final class ExampleExecHandler: ChannelDuplexHandler {
120120
.channelOption(ChannelOptions.allowRemoteHalfClosure, value: true)
121121
.channelInitializer { pipeChannel in
122122
pipeChannel.pipeline.addHandler(theirs)
123-
}.withPipes(inputDescriptor: dup(outPipe.fileHandleForReading.fileDescriptor), outputDescriptor: dup(inPipe.fileHandleForWriting.fileDescriptor)).wait()
123+
}.takingOwnershipOfDescriptors(input: dup(outPipe.fileHandleForReading.fileDescriptor), output: dup(inPipe.fileHandleForWriting.fileDescriptor)).wait()
124124

125125
// Ok, great, we've sorted stdout and stdin. For stderr we need a different strategy: we just park a thread for this.
126126
DispatchQueue(label: "stderrorwhatever").async {

‎Tests/NIOSSHTests/SSHEncryptedTrafficTests.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ final class SSHEncryptedTrafficTests: XCTestCase {
2525

2626
override func setUp() {
2727
self.serializer = SSHPacketSerializer()
28-
self.parser = SSHPacketParser(allocator: .init())
28+
self.parser = SSHPacketParser(isServer: false, allocator: .init())
2929

3030
self.assertPacketRoundTrips(.version("SSH-2.0-SwiftSSH_1.0"))
3131
}

‎Tests/NIOSSHTests/SSHPackerSerializerTests.swift‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ final class SSHPacketSerializerTests: XCTestCase {
5151
let message = SSHMessage.disconnect(.init(reason: 42, description: "description", tag: "tag"))
5252
let allocator = ByteBufferAllocator()
5353
var serializer = SSHPacketSerializer()
54-
var parser = SSHPacketParser(allocator: allocator)
54+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
5555

5656
self.runVersionHandshake(serializer: &serializer, parser: &parser)
5757

@@ -74,7 +74,7 @@ final class SSHPacketSerializerTests: XCTestCase {
7474
let message = SSHMessage.serviceRequest(.init(service: "ssh-userauth"))
7575
let allocator = ByteBufferAllocator()
7676
var serializer = SSHPacketSerializer()
77-
var parser = SSHPacketParser(allocator: allocator)
77+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
7878

7979
self.runVersionHandshake(serializer: &serializer, parser: &parser)
8080

@@ -97,7 +97,7 @@ final class SSHPacketSerializerTests: XCTestCase {
9797
let message = SSHMessage.serviceAccept(.init(service: "ssh-userauth"))
9898
let allocator = ByteBufferAllocator()
9999
var serializer = SSHPacketSerializer()
100-
var parser = SSHPacketParser(allocator: allocator)
100+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
101101

102102
self.runVersionHandshake(serializer: &serializer, parser: &parser)
103103

@@ -133,7 +133,7 @@ final class SSHPacketSerializerTests: XCTestCase {
133133
))
134134
let allocator = ByteBufferAllocator()
135135
var serializer = SSHPacketSerializer()
136-
var parser = SSHPacketParser(allocator: allocator)
136+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
137137

138138
self.runVersionHandshake(serializer: &serializer, parser: &parser)
139139

@@ -165,7 +165,7 @@ final class SSHPacketSerializerTests: XCTestCase {
165165
let message = SSHMessage.keyExchangeInit(.init(publicKey: ByteBuffer.of(bytes: [42])))
166166
let allocator = ByteBufferAllocator()
167167
var serializer = SSHPacketSerializer()
168-
var parser = SSHPacketParser(allocator: allocator)
168+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
169169

170170
self.runVersionHandshake(serializer: &serializer, parser: &parser)
171171

@@ -193,7 +193,7 @@ final class SSHPacketSerializerTests: XCTestCase {
193193
))
194194
let allocator = ByteBufferAllocator()
195195
var serializer = SSHPacketSerializer()
196-
var parser = SSHPacketParser(allocator: allocator)
196+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
197197

198198
self.runVersionHandshake(serializer: &serializer, parser: &parser)
199199

@@ -226,7 +226,7 @@ final class SSHPacketSerializerTests: XCTestCase {
226226
let message = SSHMessage.newKeys
227227
let allocator = ByteBufferAllocator()
228228
var serializer = SSHPacketSerializer()
229-
var parser = SSHPacketParser(allocator: allocator)
229+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
230230

231231
self.runVersionHandshake(serializer: &serializer, parser: &parser)
232232

@@ -247,7 +247,7 @@ final class SSHPacketSerializerTests: XCTestCase {
247247
let message = SSHMessage.newKeys
248248
let allocator = ByteBufferAllocator()
249249
var serializer = SSHPacketSerializer()
250-
var parser = SSHPacketParser(allocator: allocator)
250+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
251251

252252
self.runVersionHandshake(serializer: &serializer, parser: &parser)
253253

‎Tests/NIOSSHTests/SSHPacketParserTests.swift‎

Lines changed: 51 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ final class SSHPacketParserTests: XCTestCase {
3838
}
3939

4040
func testReadVersion() throws {
41-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
41+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
4242

4343
var part1 = ByteBuffer.of(string: "SSH-2.0-")
4444
parser.append(bytes: &part1)
@@ -57,8 +57,8 @@ final class SSHPacketParserTests: XCTestCase {
5757
}
5858
}
5959

60-
func testReadVersionWithExtraLines() throws {
61-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
60+
func testReadVersionWithExtraLinesOnClient() throws {
61+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
6262

6363
var part1 = ByteBuffer.of(string: "xxxx\r\nyyyy\r\nSSH-2.0-")
6464
parser.append(bytes: &part1)
@@ -70,14 +70,33 @@ final class SSHPacketParserTests: XCTestCase {
7070

7171
switch try parser.nextPacket() {
7272
case .version(let string):
73-
XCTAssertEqual(string, "xxxx\r\nyyyy\r\nSSH-2.0-OpenSSH_7.9")
73+
XCTAssertEqual(string, "SSH-2.0-OpenSSH_7.9")
74+
default:
75+
XCTFail("Expecting .version")
76+
}
77+
}
78+
79+
func testReadVersionWithExtraLinesOnServer() throws {
80+
var parser = SSHPacketParser(isServer: true, allocator: ByteBufferAllocator())
81+
82+
var part1 = ByteBuffer.of(string: "xx")
83+
parser.append(bytes: &part1)
84+
85+
XCTAssertNil(try parser.nextPacket())
86+
87+
var part2 = ByteBuffer.of(string: "xx\r\nyyyy\r\nSSH-2.0-OpenSSH_7.9\r\n")
88+
parser.append(bytes: &part2)
89+
90+
switch try parser.nextPacket() {
91+
case .version(let string):
92+
XCTAssertEqual(string, "xxxx")
7493
default:
7594
XCTFail("Expecting .version")
7695
}
7796
}
7897

7998
func testReadVersionWithoutCarriageReturn() throws {
80-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
99+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
81100

82101
var part1 = ByteBuffer.of(string: "SSH-2.0-")
83102
parser.append(bytes: &part1)
@@ -95,8 +114,8 @@ final class SSHPacketParserTests: XCTestCase {
95114
}
96115
}
97116

98-
func testReadVersionWithExtraLinesWithoutCarriageReturn() throws {
99-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
117+
func testReadVersionWithExtraLinesWithoutCarriageReturnOnClient() throws {
118+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
100119

101120
var part1 = ByteBuffer.of(string: "xxxx\nyyyy\nSSH-2.0-")
102121
parser.append(bytes: &part1)
@@ -108,14 +127,33 @@ final class SSHPacketParserTests: XCTestCase {
108127

109128
switch try parser.nextPacket() {
110129
case .version(let string):
111-
XCTAssertEqual(string, "xxxx\nyyyy\nSSH-2.0-OpenSSH_7.4")
130+
XCTAssertEqual(string, "SSH-2.0-OpenSSH_7.4")
131+
default:
132+
XCTFail("Expecting .version")
133+
}
134+
}
135+
136+
func testReadVersionWithExtraLinesWithoutCarriageReturnOnServer() throws {
137+
var parser = SSHPacketParser(isServer: true, allocator: ByteBufferAllocator())
138+
139+
var part1 = ByteBuffer.of(string: "xx")
140+
parser.append(bytes: &part1)
141+
142+
XCTAssertNil(try parser.nextPacket())
143+
144+
var part2 = ByteBuffer.of(string: "xx\nyyyy\nSSH-2.0-OpenSSH_7.4\n")
145+
parser.append(bytes: &part2)
146+
147+
switch try parser.nextPacket() {
148+
case .version(let string):
149+
XCTAssertEqual(string, "xxxx")
112150
default:
113151
XCTFail("Expecting .version")
114152
}
115153
}
116154

117155
func testBinaryInParts() throws {
118-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
156+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
119157
self.feedVersion(to: &parser)
120158

121159
var part1 = ByteBuffer.of(bytes: [0, 0, 0])
@@ -148,7 +186,7 @@ final class SSHPacketParserTests: XCTestCase {
148186
}
149187

150188
func testBinaryFull() throws {
151-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
189+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
152190
self.feedVersion(to: &parser)
153191

154192
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 {
164202
}
165203

166204
func testBinaryTwoMessages() throws {
167-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
205+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
168206
self.feedVersion(to: &parser)
169207

170208
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 {
187225
}
188226

189227
func testWeReclaimStorage() throws {
190-
var parser = SSHPacketParser(allocator: ByteBufferAllocator())
228+
var parser = SSHPacketParser(isServer: false, allocator: ByteBufferAllocator())
191229
self.feedVersion(to: &parser)
192230
XCTAssertNoThrow(try parser.nextPacket())
193231

@@ -213,7 +251,7 @@ final class SSHPacketParserTests: XCTestCase {
213251

214252
func testSequencePreservedBetweenPlainAndCypher() throws {
215253
let allocator = ByteBufferAllocator()
216-
var parser = SSHPacketParser(allocator: allocator)
254+
var parser = SSHPacketParser(isServer: false, allocator: allocator)
217255
self.feedVersion(to: &parser)
218256

219257
var part = ByteBuffer(bytes: [0, 0, 0, 12, 10, 21, 41, 114, 125, 250, 3, 79, 3, 217, 166, 136])

0 commit comments

Comments
 (0)