From 97bef1b20672e4064afeeb5c10fc0549b00f999c Mon Sep 17 00:00:00 2001 From: Joannis Orlandos Date: Fri, 20 Nov 2020 12:15:46 +0100 Subject: [PATCH 1/6] Set a maximum packet size of 32768 bytes for version and cleartext packets - including those wrapping encrypted packets --- Sources/NIOSSH/SSHPacketParser.swift | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/Sources/NIOSSH/SSHPacketParser.swift b/Sources/NIOSSH/SSHPacketParser.swift index 68f3727..a857fae 100644 --- a/Sources/NIOSSH/SSHPacketParser.swift +++ b/Sources/NIOSSH/SSHPacketParser.swift @@ -25,6 +25,7 @@ struct SSHPacketParser { private var buffer: ByteBuffer private var state: State + private let maximumPacketSize = 32768 /// Testing only: the number of bytes we can discard from this buffer. internal var _discardableBytes: Int { @@ -71,6 +72,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,7 +119,13 @@ 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 { + + // Prevent the consumed bytes for a version from exceeding the maximum packet 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 these might be multiple packets + let maximumVersionSize = min(slice.endIndex, self.maximumPacketSize) + + if let cr = slice.firstIndex(of: 13), cr.advanced(by: 1) < maximumVersionSize, 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)) @@ -132,7 +143,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? { From 13c7a3d996be076bdb2641dba11ba2fb99943963 Mon Sep 17 00:00:00 2001 From: Joannis Orlandos Date: Tue, 16 Nov 2021 16:51:24 +0100 Subject: [PATCH 2/6] Make maximumPacketSize configurable and test if large packets are rejected --- .../SSHChannelMultiplexer.swift | 13 +++++-- .../Child Channels/SSHChildChannel.swift | 16 +++++--- .../SSHConnectionStateMachine.swift | 2 +- .../States/SentVersionState.swift | 4 +- Sources/NIOSSH/NIOSSHError.swift | 6 +++ Sources/NIOSSH/NIOSSHHandler.swift | 2 +- Sources/NIOSSH/Role.swift | 9 +++++ Sources/NIOSSH/SSHClientConfiguration.swift | 3 ++ Sources/NIOSSH/SSHPacketParser.swift | 33 +++++++++++----- Sources/NIOSSH/SSHServerConfiguration.swift | 3 ++ Tests/NIOSSHTests/EndToEndTests.swift | 39 +++++++++++++++++-- Tests/NIOSSHTests/SSHPacketParserTests.swift | 9 +++++ 12 files changed, 113 insertions(+), 26 deletions(-) diff --git a/Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift b/Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift index 1638460..73a58ff 100644 --- a/Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift +++ b/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 diff --git a/Sources/NIOSSH/Child Channels/SSHChildChannel.swift b/Sources/NIOSSH/Child Channels/SSHChildChannel.swift index 11c27c8..5be2fc0 100644 --- a/Sources/NIOSSH/Child Channels/SSHChildChannel.swift +++ b/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? @@ -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,7 +474,7 @@ 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 { @@ -476,7 +482,7 @@ extension SSHChildChannel: Channel, ChannelCore { 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 { diff --git a/Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift b/Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift index f8373f9..c2564e2 100644 --- a/Sources/NIOSSH/Connection State Machine/SSHConnectionStateMachine.swift +++ b/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) diff --git a/Sources/NIOSSH/Connection State Machine/States/SentVersionState.swift b/Sources/NIOSSH/Connection State Machine/States/SentVersionState.swift index a7a83fb..8257aa7 100644 --- a/Sources/NIOSSH/Connection State Machine/States/SentVersionState.swift +++ b/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 } } diff --git a/Sources/NIOSSH/NIOSSHError.swift b/Sources/NIOSSH/NIOSSHError.swift index 9090dd2..0544fc5 100644 --- a/Sources/NIOSSH/NIOSSHError.swift +++ b/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) diff --git a/Sources/NIOSSH/NIOSSHHandler.swift b/Sources/NIOSSH/NIOSSHHandler.swift index ca00b15..6854fb9 100644 --- a/Sources/NIOSSH/NIOSSHHandler.swift +++ b/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) } } diff --git a/Sources/NIOSSH/Role.swift b/Sources/NIOSSH/Role.swift index 2af2818..11d452b 100644 --- a/Sources/NIOSSH/Role.swift +++ b/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 + } + } } diff --git a/Sources/NIOSSH/SSHClientConfiguration.swift b/Sources/NIOSSH/SSHClientConfiguration.swift index 7ecbbc4..ca675be 100644 --- a/Sources/NIOSSH/SSHClientConfiguration.swift +++ b/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, diff --git a/Sources/NIOSSH/SSHPacketParser.swift b/Sources/NIOSSH/SSHPacketParser.swift index a857fae..6c81960 100644 --- a/Sources/NIOSSH/SSHPacketParser.swift +++ b/Sources/NIOSSH/SSHPacketParser.swift @@ -25,16 +25,21 @@ struct SSHPacketParser { private var buffer: ByteBuffer private var state: State - private let maximumPacketSize = 32768 + 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) { @@ -120,17 +125,25 @@ struct SSHPacketParser { // Looking for a string ending with \r\n let slice = self.buffer.readableBytesView - // Prevent the consumed bytes for a version from exceeding the maximum packet size + // 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 these might be multiple packets - let maximumVersionSize = min(slice.endIndex, self.maximumPacketSize) + // 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)) - if let cr = slice.firstIndex(of: 13), cr.advanced(by: 1) < maximumVersionSize, 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 + 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 } diff --git a/Sources/NIOSSH/SSHServerConfiguration.swift b/Sources/NIOSSH/SSHServerConfiguration.swift index d9bd179..7c9b7de 100644 --- a/Sources/NIOSSH/SSHServerConfiguration.swift +++ b/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 diff --git a/Tests/NIOSSHTests/EndToEndTests.swift b/Tests/NIOSSHTests/EndToEndTests.swift index d28dfe9..08fa96c 100644 --- a/Tests/NIOSSHTests/EndToEndTests.swift +++ b/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())) diff --git a/Tests/NIOSSHTests/SSHPacketParserTests.swift b/Tests/NIOSSHTests/SSHPacketParserTests.swift index 0a727dd..02f6ad9 100644 --- a/Tests/NIOSSHTests/SSHPacketParserTests.swift +++ b/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 { From ffbf2dfcbd8f5318f05db687239e4a39a03f9ca3 Mon Sep 17 00:00:00 2001 From: Joannis Orlandos Date: Tue, 23 Nov 2021 17:36:29 +0100 Subject: [PATCH 3/6] Implement Cory's feedback --- Sources/NIOSSH/NIOSSHError.swift | 2 +- Sources/NIOSSH/SSHPacketParser.swift | 14 ++++++++------ Tests/NIOSSHTests/SSHPacketParserTests.swift | 2 +- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/Sources/NIOSSH/NIOSSHError.swift b/Sources/NIOSSH/NIOSSHError.swift index 0544fc5..d34248a 100644 --- a/Sources/NIOSSH/NIOSSHError.swift +++ b/Sources/NIOSSH/NIOSSHError.swift @@ -199,7 +199,7 @@ 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. + /// 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 diff --git a/Sources/NIOSSH/SSHPacketParser.swift b/Sources/NIOSSH/SSHPacketParser.swift index 6c81960..bdefca1 100644 --- a/Sources/NIOSSH/SSHPacketParser.swift +++ b/Sources/NIOSSH/SSHPacketParser.swift @@ -34,8 +34,8 @@ struct SSHPacketParser { 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") + precondition(maximumPacketSize >= 32_768, "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 @@ -121,14 +121,15 @@ struct SSHPacketParser { } } + internal static let maximumAllowedVersionSize = 4_096 private mutating func readVersion() throws -> String? { // Looking for a string ending with \r\n let slice = self.buffer.readableBytesView - // Prevent the consumed bytes for a version from exceeding 4_096 + // 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, 4_096)) + let maxIndex = slice.index(slice.startIndex, offsetBy: min(slice.count, Self.maximumAllowedVersionSize)) for index in slice.startIndex ..< slice.endIndex { if index > maxIndex { @@ -157,12 +158,13 @@ struct SSHPacketParser { // 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. let packetLength = self.buffer.getInteger(at: self.buffer.readerIndex, as: UInt32.self)! + let decryptedLength = packetLength + UInt32(protection.macBytes) - if packetLength >= self.maximumPacketSize { + if decryptedLength >= self.maximumPacketSize { throw NIOSSHError.invalidEncryptedPacketLength } - return packetLength + UInt32(protection.macBytes) + return decryptedLength } private mutating func parsePlaintext(length: UInt32) throws -> SSHMessage? { diff --git a/Tests/NIOSSHTests/SSHPacketParserTests.swift b/Tests/NIOSSHTests/SSHPacketParserTests.swift index 02f6ad9..fb2a8d6 100644 --- a/Tests/NIOSSHTests/SSHPacketParserTests.swift +++ b/Tests/NIOSSHTests/SSHPacketParserTests.swift @@ -145,7 +145,7 @@ final class SSHPacketParserTests: XCTestCase { func testMaximumPacketSizeInVersion() throws { var parser = SSHPacketParser(allocator: ByteBufferAllocator(), maximumPacketSize: 1 << 15) - let longVersionString = String(repeating: "z", count: 5000) + let longVersionString = String(repeating: "z", count: SSHPacketParser.maximumAllowedVersionSize + 256) var version = ByteBuffer.of(string: longVersionString + "\r\n") parser.append(bytes: &version) From 2379347fe5477591c86e5fcccdba9eefa899f5c6 Mon Sep 17 00:00:00 2001 From: Joannis Orlandos Date: Thu, 25 Nov 2021 10:19:53 +0100 Subject: [PATCH 4/6] Define a default maximumPacketSize --- Sources/NIOSSH/SSHClientConfiguration.swift | 2 +- Sources/NIOSSH/SSHPacketParser.swift | 3 +- Sources/NIOSSH/SSHServerConfiguration.swift | 2 +- .../ChildChannelMultiplexerTests.swift | 34 +++++++++---------- Tests/NIOSSHTests/EndToEndTests.swift | 4 +-- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/Sources/NIOSSH/SSHClientConfiguration.swift b/Sources/NIOSSH/SSHClientConfiguration.swift index ca675be..23e3f42 100644 --- a/Sources/NIOSSH/SSHClientConfiguration.swift +++ b/Sources/NIOSSH/SSHClientConfiguration.swift @@ -24,7 +24,7 @@ public struct SSHClientConfiguration { public var globalRequestDelegate: GlobalRequestDelegate /// The maximum packet size that this NIOSSH client will accept - public var maximumPacketSize = 1 << 17 + public var maximumPacketSize = SSHPacketParser.defaultMaximumPacketSize public init(userAuthDelegate: NIOSSHClientUserAuthenticationDelegate, serverAuthDelegate: NIOSSHClientServerAuthenticationDelegate, diff --git a/Sources/NIOSSH/SSHPacketParser.swift b/Sources/NIOSSH/SSHPacketParser.swift index bdefca1..c956d08 100644 --- a/Sources/NIOSSH/SSHPacketParser.swift +++ b/Sources/NIOSSH/SSHPacketParser.swift @@ -26,13 +26,14 @@ 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, maximumPacketSize: Int = 1 << 17) { + init(allocator: ByteBufferAllocator, maximumPacketSize: Int = defaultMaximumPacketSize) { // Assert that users don't provide a packet size lower than allowed by spec precondition(maximumPacketSize >= 32_768, "Maximum Packet Size is below minimum requirement as specified by RFC 4254") precondition(maximumPacketSize <= (1 << 24), "Maximum Packet Size is set abnormally high") diff --git a/Sources/NIOSSH/SSHServerConfiguration.swift b/Sources/NIOSSH/SSHServerConfiguration.swift index 22205ca..7928fb0 100644 --- a/Sources/NIOSSH/SSHServerConfiguration.swift +++ b/Sources/NIOSSH/SSHServerConfiguration.swift @@ -24,7 +24,7 @@ public struct SSHServerConfiguration { public var hostKeys: [NIOSSHPrivateKey] /// The maximum packet size that this NIOSSH server will accept - public var maximumPacketSize = 1 << 17 + public var maximumPacketSize = SSHPacketParser.defaultMaximumPacketSize /// The ssh banner to display to clients upon authentication public var banner: UserAuthBanner? diff --git a/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift b/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift index 0f816c2..20ba2b3 100644 --- a/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift +++ b/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift @@ -193,14 +193,14 @@ final class ChildChannelMultiplexerTests: XCTestCase { } } - private func openRequest(channelID: UInt32, initialWindowSize: UInt32 = 1 << 24, maxPacketSize: UInt32 = 1 << 24) -> SSHMessage { + private func openRequest(channelID: UInt32, initialWindowSize: UInt32 = UInt32(SSHPacketParser.defaultMaximumPacketSize), maxPacketSize: UInt32 = UInt32(SSHPacketParser.defaultMaximumPacketSize)) -> SSHMessage { .channelOpen(.init(type: .session, senderChannel: channelID, initialWindowSize: initialWindowSize, maximumPacketSize: maxPacketSize)) } - private func openConfirmation(originalChannelID: UInt32, peerChannelID: UInt32, initialWindowSize: UInt32 = 1 << 24, maxPacketSize: UInt32 = 1 << 24) -> SSHMessage { + private func openConfirmation(originalChannelID: UInt32, peerChannelID: UInt32, initialWindowSize: UInt32 = UInt32(SSHPacketParser.defaultMaximumPacketSize), maxPacketSize: UInt32 = UInt32(SSHPacketParser.defaultMaximumPacketSize)) -> SSHMessage { .channelOpenConfirmation(.init(recipientChannel: originalChannelID, senderChannel: peerChannelID, initialWindowSize: initialWindowSize, @@ -231,8 +231,8 @@ final class ChildChannelMultiplexerTests: XCTestCase { func assertChannelOpen(_ message: SSHMessage?) -> UInt32? { switch message { case .some(.channelOpen(let message)): - XCTAssertEqual(message.maximumPacketSize, 1 << 24) - XCTAssertEqual(message.initialWindowSize, 1 << 24) + XCTAssertEqual(message.maximumPacketSize, UInt32(SSHPacketParser.defaultMaximumPacketSize)) + XCTAssertEqual(message.initialWindowSize, UInt32(SSHPacketParser.defaultMaximumPacketSize)) return message.senderChannel case let fallback: @@ -246,8 +246,8 @@ final class ChildChannelMultiplexerTests: XCTestCase { switch message { case .some(.channelOpenConfirmation(let message)): XCTAssertEqual(message.recipientChannel, recipientChannel) - XCTAssertEqual(message.maximumPacketSize, 1 << 24) - XCTAssertEqual(message.initialWindowSize, 1 << 24) + XCTAssertEqual(message.maximumPacketSize, UInt32(SSHPacketParser.defaultMaximumPacketSize)) + XCTAssertEqual(message.initialWindowSize, UInt32(SSHPacketParser.defaultMaximumPacketSize)) return message.senderChannel case let fallback: @@ -1191,13 +1191,13 @@ final class ChildChannelMultiplexerTests: XCTestCase { let channelID = self.assertChannelOpen(harness.flushedMessages.first) XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.openConfirmation(originalChannelID: channelID!, peerChannelID: 1))) - // The default window size is 1<<24 bytes. Sadly, we need a buffer that size. + // Make a buffer the size of the default window size let buffer = ByteBuffer.bigBuffer // We're going to write one byte short. XCTAssertEqual(harness.flushedMessages.count, 1) XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.data(peerChannelID: channelID!, - data: buffer.getSlice(at: buffer.readerIndex, length: (1 << 23) - 1)!))) + data: buffer.getSlice(at: buffer.readerIndex, length: SSHPacketParser.defaultMaximumPacketSize - 1)!))) // Auto read is off, so nothing happens. XCTAssertEqual(harness.flushedMessages.count, 1) @@ -1214,17 +1214,17 @@ final class ChildChannelMultiplexerTests: XCTestCase { // Now issue a read. This triggers an outbound message. channel.read() XCTAssertEqual(harness.flushedMessages.count, 2) - self.assertWindowAdjust(harness.flushedMessages.last, recipientChannel: 1, delta: 1 << 23) + self.assertWindowAdjust(harness.flushedMessages.last, recipientChannel: 1, delta: SSHPacketParser.defaultMaximumPacketSize) // Now issue a really big read. Again, there's no autoread, so this does nothing. XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.data(peerChannelID: channelID!, - data: buffer.getSlice(at: buffer.readerIndex, length: 1 << 24)!))) + data: buffer.getSlice(at: buffer.readerIndex, length: SSHPacketParser.defaultMaximumPacketSize)!))) XCTAssertEqual(harness.flushedMessages.count, 2) // Issue the read. A new outbound message with a bigger window increment. channel.read() XCTAssertEqual(harness.flushedMessages.count, 3) - self.assertWindowAdjust(harness.flushedMessages.last, recipientChannel: 1, delta: 1 << 24) + self.assertWindowAdjust(harness.flushedMessages.last, recipientChannel: 1, delta: UInt32(SSHPacketParser.defaultMaximumPacketSize)) // Finally, issue an excessively large read. This should cause an error. XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.data(peerChannelID: channelID!, data: buffer))) @@ -1253,13 +1253,13 @@ final class ChildChannelMultiplexerTests: XCTestCase { let channelID = self.assertChannelOpen(harness.flushedMessages.first) XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.openConfirmation(originalChannelID: channelID!, peerChannelID: 1))) - // The default window size is 1<<24 bytes. Sadly, we need a buffer that size. + // Make a buffer the size of the default window size let buffer = ByteBuffer.bigBuffer // We're going to write the whole window. XCTAssertEqual(harness.flushedMessages.count, 1) XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.data(peerChannelID: channelID!, - data: buffer.getSlice(at: buffer.readerIndex, length: 1 << 24)!))) + data: buffer.getSlice(at: buffer.readerIndex, length: SSHPacketParser.defaultMaximumPacketSize)!))) // Auto read is off, so nothing happens. XCTAssertEqual(harness.flushedMessages.count, 1) @@ -1561,7 +1561,7 @@ final class ChildChannelMultiplexerTests: XCTestCase { ] for (channelID, channelType) in channelTypes.enumerated() { - let message = SSHMessage.channelOpen(.init(type: .init(channelType), senderChannel: UInt32(channelID), initialWindowSize: 1 << 24, maximumPacketSize: 1 << 24)) + let message = SSHMessage.channelOpen(.init(type: .init(channelType), senderChannel: UInt32(channelID), initialWindowSize: UInt32(SSHPacketParser.defaultMaximumPacketSize), maximumPacketSize: UInt32(SSHPacketParser.defaultMaximumPacketSize))) XCTAssertNoThrow(try harness.multiplexer.receiveMessage(message)) } @@ -1757,10 +1757,10 @@ final class ChildChannelMultiplexerTests: XCTestCase { } extension ByteBuffer { - /// A buffer `(1 << 24) + 1` bytes large. + /// A buffer `(SSHPacketParser.defaultMaximumPacketSize) + 1` bytes large. fileprivate static let bigBuffer: ByteBuffer = { - // The default window size is 1<<24 bytes. Sadly, we need a buffer that size. + // Make a buffer the size of the default window size // We store it in a static so that we don't have to re-create it for every test. - ByteBuffer(repeating: 0, count: (1 << 24) + 1) + ByteBuffer(repeating: 0, count: (SSHPacketParser.defaultMaximumPacketSize) + 1) }() } diff --git a/Tests/NIOSSHTests/EndToEndTests.swift b/Tests/NIOSSHTests/EndToEndTests.swift index ed093fd..78ad0d7 100644 --- a/Tests/NIOSSHTests/EndToEndTests.swift +++ b/Tests/NIOSSHTests/EndToEndTests.swift @@ -82,7 +82,7 @@ class BackToBackEmbeddedChannel { 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) + var serverConfiguration = SSHServerConfiguration(hostKeys: harness.serverHostKeys, userAuthDelegate: harness.serverAuthDelegate, globalRequestDelegate: harness.serverGlobalRequestDelegate, banner: harness.serverAuthBanner) if let maximumPacketSize = maximumPacketSize { clientConfiguration.maximumPacketSize = maximumPacketSize @@ -92,7 +92,7 @@ class BackToBackEmbeddedChannel { 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, banner: harness.serverAuthBanner)), + 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 }) } From 8345d00794c86964254f4660e4155c231fb05320 Mon Sep 17 00:00:00 2001 From: Joannis Orlandos Date: Thu, 25 Nov 2021 16:05:19 +0100 Subject: [PATCH 5/6] Update `testWeDealWithFlowControlProperly` to configurable maxPacketSize --- Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift b/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift index 20ba2b3..339c892 100644 --- a/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift +++ b/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift @@ -1197,7 +1197,7 @@ final class ChildChannelMultiplexerTests: XCTestCase { // We're going to write one byte short. XCTAssertEqual(harness.flushedMessages.count, 1) XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.data(peerChannelID: channelID!, - data: buffer.getSlice(at: buffer.readerIndex, length: SSHPacketParser.defaultMaximumPacketSize - 1)!))) + data: buffer.getSlice(at: buffer.readerIndex, length: (SSHPacketParser.defaultMaximumPacketSize >> 1) - 1)!))) // Auto read is off, so nothing happens. XCTAssertEqual(harness.flushedMessages.count, 1) @@ -1214,7 +1214,7 @@ final class ChildChannelMultiplexerTests: XCTestCase { // Now issue a read. This triggers an outbound message. channel.read() XCTAssertEqual(harness.flushedMessages.count, 2) - self.assertWindowAdjust(harness.flushedMessages.last, recipientChannel: 1, delta: SSHPacketParser.defaultMaximumPacketSize) + self.assertWindowAdjust(harness.flushedMessages.last, recipientChannel: 1, delta: UInt32(SSHPacketParser.defaultMaximumPacketSize >> 1)) // Now issue a really big read. Again, there's no autoread, so this does nothing. XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.data(peerChannelID: channelID!, From 10aa6efcbeaf863fdf0a25dbed05e8fd38068b78 Mon Sep 17 00:00:00 2001 From: Gwynne Raskind Date: Sat, 4 Dec 2021 14:03:27 -0600 Subject: [PATCH 6/6] Run soundness script for code formatting. --- .../SSHChannelMultiplexer.swift | 6 +++--- .../Child Channels/SSHChildChannel.swift | 6 +++--- Sources/NIOSSH/NIOSSHError.swift | 4 ++-- Sources/NIOSSH/Role.swift | 2 +- Sources/NIOSSH/SSHClientConfiguration.swift | 2 +- Sources/NIOSSH/SSHPacketParser.swift | 20 +++++++++---------- Sources/NIOSSH/SSHServerConfiguration.swift | 2 +- .../ChildChannelMultiplexerTests.swift | 2 +- Tests/NIOSSHTests/EndToEndTests.swift | 16 +++++++-------- Tests/NIOSSHTests/SSHPacketParserTests.swift | 2 +- 10 files changed, 31 insertions(+), 31 deletions(-) diff --git a/Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift b/Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift index 73a58ff..f07e99f 100644 --- a/Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift +++ b/Sources/NIOSSH/Child Channels/SSHChannelMultiplexer.swift @@ -33,7 +33,7 @@ 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?, maximumPacketSize: Int = 1 << 17) { @@ -200,9 +200,9 @@ extension SSHChannelMultiplexer { multiplexer: self, initializer: initializer, localChannelID: channelID, - targetWindowSize: Int32(maximumPacketSize), + targetWindowSize: Int32(self.maximumPacketSize), initialOutboundWindowSize: 0, - maximumPacketSize: maximumPacketSize) // The initial outbound window size is presumed to be 0 until we're told otherwise. + maximumPacketSize: self.maximumPacketSize) // The initial outbound window size is presumed to be 0 until we're told otherwise. self.channels[channelID] = channel return channel diff --git a/Sources/NIOSSH/Child Channels/SSHChildChannel.swift b/Sources/NIOSSH/Child Channels/SSHChildChannel.swift index 5fbb602..4864f5c 100644 --- a/Sources/NIOSSH/Child Channels/SSHChildChannel.swift +++ b/Sources/NIOSSH/Child Channels/SSHChildChannel.swift @@ -97,7 +97,7 @@ 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. @@ -476,7 +476,7 @@ extension SSHChildChannel: Channel, ChannelCore { let message = SSHMessage.ChannelOpenConfirmationMessage(recipientChannel: self.state.remoteChannelIdentifier!, senderChannel: self.state.localChannelIdentifier, initialWindowSize: self.windowManager.targetWindowSize, - maximumPacketSize: UInt32(maximumPacketSize)) + maximumPacketSize: UInt32(self.maximumPacketSize)) self.processOutboundMessage(.channelOpenConfirmation(message), promise: nil) self.writePendingToMultiplexer() } else if !self.state.isClosed { @@ -484,7 +484,7 @@ extension SSHChildChannel: Channel, ChannelCore { let message = SSHMessage.ChannelOpenMessage(type: .init(self.type!), senderChannel: self.state.localChannelIdentifier, initialWindowSize: self.windowManager.targetWindowSize, - maximumPacketSize: UInt32(maximumPacketSize)) + maximumPacketSize: UInt32(self.maximumPacketSize)) self.processOutboundMessage(.channelOpen(message), promise: nil) self.writePendingToMultiplexer() } else { diff --git a/Sources/NIOSSH/NIOSSHError.swift b/Sources/NIOSSH/NIOSSHError.swift index d34248a..9d1085c 100644 --- a/Sources/NIOSSH/NIOSSHError.swift +++ b/Sources/NIOSSH/NIOSSHError.swift @@ -43,7 +43,7 @@ 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) @@ -198,7 +198,7 @@ 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) diff --git a/Sources/NIOSSH/Role.swift b/Sources/NIOSSH/Role.swift index 11d452b..4c0424b 100644 --- a/Sources/NIOSSH/Role.swift +++ b/Sources/NIOSSH/Role.swift @@ -34,7 +34,7 @@ public enum SSHConnectionRole { return true } } - + internal var maximumPacketSize: Int { switch self { case .client(let client): diff --git a/Sources/NIOSSH/SSHClientConfiguration.swift b/Sources/NIOSSH/SSHClientConfiguration.swift index 23e3f42..b91d765 100644 --- a/Sources/NIOSSH/SSHClientConfiguration.swift +++ b/Sources/NIOSSH/SSHClientConfiguration.swift @@ -22,7 +22,7 @@ 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 diff --git a/Sources/NIOSSH/SSHPacketParser.swift b/Sources/NIOSSH/SSHPacketParser.swift index c956d08..c5a1b6c 100644 --- a/Sources/NIOSSH/SSHPacketParser.swift +++ b/Sources/NIOSSH/SSHPacketParser.swift @@ -35,9 +35,9 @@ struct SSHPacketParser { init(allocator: ByteBufferAllocator, maximumPacketSize: Int = defaultMaximumPacketSize) { // Assert that users don't provide a packet size lower than allowed by spec - precondition(maximumPacketSize >= 32_768, "Maximum Packet Size is below minimum requirement as specified by RFC 4254") + 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 @@ -81,7 +81,7 @@ struct SSHPacketParser { if length >= self.maximumPacketSize { throw NIOSSHError.invalidEncryptedPacketLength } - + if let message = try self.parsePlaintext(length: length) { self.state = .cleartextWaitingForLength return message @@ -122,22 +122,22 @@ struct SSHPacketParser { } } - internal static let maximumAllowedVersionSize = 4_096 + internal static let maximumAllowedVersionSize = 4096 private mutating func readVersion() throws -> String? { // Looking for a string ending with \r\n let slice = self.buffer.readableBytesView - + // 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 @@ -145,7 +145,7 @@ struct SSHPacketParser { return version } } - + return nil } @@ -160,11 +160,11 @@ struct SSHPacketParser { // 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. 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 } diff --git a/Sources/NIOSSH/SSHServerConfiguration.swift b/Sources/NIOSSH/SSHServerConfiguration.swift index 7928fb0..f33af07 100644 --- a/Sources/NIOSSH/SSHServerConfiguration.swift +++ b/Sources/NIOSSH/SSHServerConfiguration.swift @@ -22,7 +22,7 @@ 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 diff --git a/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift b/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift index 339c892..d87c8a3 100644 --- a/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift +++ b/Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift @@ -1761,6 +1761,6 @@ extension ByteBuffer { fileprivate static let bigBuffer: ByteBuffer = { // Make a buffer the size of the default window size // We store it in a static so that we don't have to re-create it for every test. - ByteBuffer(repeating: 0, count: (SSHPacketParser.defaultMaximumPacketSize) + 1) + ByteBuffer(repeating: 0, count: SSHPacketParser.defaultMaximumPacketSize + 1) }() } diff --git a/Tests/NIOSSHTests/EndToEndTests.swift b/Tests/NIOSSHTests/EndToEndTests.swift index 78ad0d7..8febdfa 100644 --- a/Tests/NIOSSHTests/EndToEndTests.swift +++ b/Tests/NIOSSHTests/EndToEndTests.swift @@ -81,14 +81,14 @@ class BackToBackEmbeddedChannel { 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, banner: harness.serverAuthBanner) - + if let maximumPacketSize = maximumPacketSize { clientConfiguration.maximumPacketSize = maximumPacketSize serverConfiguration.maximumPacketSize = maximumPacketSize } - + let clientHandler = NIOSSHHandler(role: .client(clientConfiguration), allocator: self.client.allocator, inboundChildChannelInitializer: nil) @@ -138,8 +138,8 @@ struct TestHarness { var serverGlobalRequestDelegate: GlobalRequestDelegate? var serverHostKeys: [NIOSSHPrivateKey] = [.init(ed25519Key: .init())] - - var maximumPacketSize: Int? = nil + + var maximumPacketSize: Int? var serverAuthBanner: SSHServerConfiguration.UserAuthBanner? } @@ -253,10 +253,10 @@ 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.configureWithHarness(TestHarness(), maximumPacketSize: 32768)) XCTAssertNoThrow(try self.channel.activate()) XCTAssertNoThrow(try self.channel.interactInMemory()) @@ -271,7 +271,7 @@ class EndToEndTests: XCTestCase { let userEventRecorder = UserEventExpecter() XCTAssertNoThrow(try serverChannel.pipeline.addHandler(userEventRecorder).wait()) - let hugeCommand = String(repeating: "a", count: 32_768) + let hugeCommand = String(repeating: "a", count: 32768) try clientChannel.triggerUserOutboundEvent(SSHChannelRequestEvent.ExecRequest(command: hugeCommand, wantReply: true)).wait() XCTAssertThrowsError(try self.channel.interactInMemory()) } diff --git a/Tests/NIOSSHTests/SSHPacketParserTests.swift b/Tests/NIOSSHTests/SSHPacketParserTests.swift index fb2a8d6..9e6db8e 100644 --- a/Tests/NIOSSHTests/SSHPacketParserTests.swift +++ b/Tests/NIOSSHTests/SSHPacketParserTests.swift @@ -142,7 +142,7 @@ 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: SSHPacketParser.maximumAllowedVersionSize + 256)