From e7e665e8601dc95ce61370d9d7d80f80b1d07d0d Mon Sep 17 00:00:00 2001 From: Mika Cohen Date: Mon, 25 May 2026 16:55:23 -0600 Subject: [PATCH 1/2] Allow AEAD ciphers to ignore compatibility MAC negotiation Motivation: OpenSSH-style AEAD ciphers such as aes128-gcm@openssh.com ignore SSH MAC negotiation, but SwiftNIO SSH still requires KEXINIT MAC name-lists to be non-empty. When a peer offers an AEAD cipher with only MAC algorithms that are unused by AEAD, negotiation should not fail because the MAC lists do not overlap. Modifications: Negotiate transport protection as a concrete cipher/MAC scheme. AEAD schemes with no MAC name are selected by cipher and ignore the peer MAC list, while non-AEAD schemes still use the RFC first mutual MAC and fail if that cipher/MAC pair is unsupported. Keep the symmetric-protection requirement by comparing the selected protection types in both directions. Add AEAD regression coverage for the compatibility MAC proposal and for completing a handshake when the peer offers only unused ETM MACs. Result: OpenSSH-style AEAD negotiation succeeds without requiring MAC-list overlap, while non-AEAD MAC negotiation remains strict and symmetric. --- .../SSHKeyExchangeStateMachine.swift | 61 ++++++++------- .../SSHKeyExchangeStateMachineTests.swift | 74 +++++++++++++++++++ 2 files changed, 108 insertions(+), 27 deletions(-) diff --git a/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift b/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift index 3ef830dc..13e3da85 100644 --- a/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift +++ b/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift @@ -384,26 +384,17 @@ struct SSHKeyExchangeStateMachine { peerKeyExchangeAlgorithms: message.keyExchangeAlgorithms, peerHostKeyAlgorithms: message.serverHostKeyAlgorithms ) - let (clientEncryption, clientMAC) = try self.negotiatedTransportProtection( + let clientProtection = try self.negotiatedTransportProtection( peerEncryptionAlgorithms: message.encryptionAlgorithmsClientToServer, peerMacAlgorithms: message.macAlgorithmsClientToServer ) - let (serverEncryption, serverMAC) = try self.negotiatedTransportProtection( + let serverProtection = try self.negotiatedTransportProtection( peerEncryptionAlgorithms: message.encryptionAlgorithmsServerToClient, peerMacAlgorithms: message.macAlgorithmsServerToClient ) // We only support symmetrical negotiation results. - guard clientEncryption == serverEncryption, clientMAC == serverMAC else { - throw NIOSSHError.keyExchangeNegotiationFailure - } - - // Ok, now we need to find the right transport protection scheme. This can technically fail. - guard - let scheme = self.protectionSchemes.first(where: { - $0.cipherName == clientEncryption && ($0.macName == nil || $0.macName! == clientMAC) - }) - else { + guard ObjectIdentifier(clientProtection) == ObjectIdentifier(serverProtection) else { throw NIOSSHError.keyExchangeNegotiationFailure } @@ -411,7 +402,7 @@ struct SSHKeyExchangeStateMachine { return NegotiationResult( negotiatedKeyExchangeAlgorithm: keyExchange, negotiatedHostKeyAlgorithm: hostKey, - negotiatedProtection: scheme + negotiatedProtection: clientProtection ) } @@ -487,7 +478,7 @@ struct SSHKeyExchangeStateMachine { private func negotiatedTransportProtection( peerEncryptionAlgorithms: [Substring], peerMacAlgorithms: [Substring] - ) throws -> (encryption: Substring, mac: Substring) { + ) throws -> NIOSSHTransportProtection.Type { // Ok, rephrase as client and server instead of us and them. let clientEncryptionAlgorithms: [Substring] let serverEncryptionAlgorithms: [Substring] @@ -507,19 +498,36 @@ struct SSHKeyExchangeStateMachine { serverMACAlgorithms = self.supportedMacAlgorithms } - // Ok, the algorithm is that we choose the first encryption and MAC algorithm in the client's list that - // is in the server's list as well. - guard let encryption = clientEncryptionAlgorithms.first(where: { serverEncryptionAlgorithms.contains($0) }) - else { - throw NIOSSHError.keyExchangeNegotiationFailure - } + // Ok, the algorithm is that we choose the first encryption algorithm in the client's list that + // is in the server's list as well. For OpenSSH-style AEAD ciphers the MAC negotiation is ignored. + // Otherwise, we choose the first mutually-supported MAC and then check that we have a scheme for + // that cipher/MAC pair. + for encryption in clientEncryptionAlgorithms { + guard serverEncryptionAlgorithms.contains(encryption) else { + continue + } - // Ok great, now work out what we negotiated as a MAC. - guard let mac = clientMACAlgorithms.first(where: { serverMACAlgorithms.contains($0) }) else { - throw NIOSSHError.keyExchangeNegotiationFailure + let candidateSchemes = self.protectionSchemes.filter { $0.cipherName == encryption } + + if let aeadScheme = candidateSchemes.first(where: { $0.macName == nil }) { + return aeadScheme + } + + guard let mac = clientMACAlgorithms.first(where: { serverMACAlgorithms.contains($0) }) else { + throw NIOSSHError.keyExchangeNegotiationFailure + } + + // MAC negotiation is independent in RFC 4253. Once the first mutual MAC is selected, both + // peers will use it, so do not skip ahead to a later MAC just because this side lacks a + // registered scheme for the selected cipher/MAC pair. + guard let scheme = candidateSchemes.first(where: { $0.macName == String(mac) }) else { + throw NIOSSHError.keyExchangeNegotiationFailure + } + + return scheme } - return (encryption, mac) + throw NIOSSHError.keyExchangeNegotiationFailure } private mutating func addKeyExchangeInitMessagesToExchangeBytes( @@ -576,9 +584,8 @@ struct SSHKeyExchangeStateMachine { private var supportedMacAlgorithms: [Substring] { let schemes = self.protectionSchemes.compactMap { $0.macName.map { Substring($0) } } - // We do a weird thing here: if there are no MAC schemes, we lie and put one in. This is - // because some schemes (such as AES-GCM in OpenSSH mode) ignore the MAC negotiation. - // Worse case, we fail out later in the handshake because the peer actually wanted it. + // RFC 4253 requires KEXINIT algorithm name-lists to contain at least one algorithm name. + // OpenSSH-style AEAD ciphers ignore MAC negotiation, but still need a compatibility MAC proposal. if schemes.isEmpty { return ["hmac-sha2-256"] } else { diff --git a/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift b/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift index 55efa887..8157303f 100644 --- a/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift +++ b/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift @@ -693,6 +693,80 @@ final class SSHKeyExchangeStateMachineTests: XCTestCase { } } + func testAEADOnlyKeyExchangeAdvertisesCompatibilityMACs() throws { + let allocator = ByteBufferAllocator() + let loop = EmbeddedEventLoop() + + let client = SSHKeyExchangeStateMachine( + allocator: allocator, + loop: loop, + role: .client( + .init(userAuthDelegate: ExplodingAuthDelegate(), serverAuthDelegate: AcceptAllHostKeysDelegate()) + ), + remoteVersion: Constants.version, + protectionSchemes: [AES128GCMOpenSSHTransportProtection.self], + previousSessionIdentifier: nil + ) + + let clientMessage = client.createKeyExchangeMessage() + XCTAssertEqual(["aes128-gcm@openssh.com"], clientMessage.encryptionAlgorithmsClientToServer) + XCTAssertEqual(["aes128-gcm@openssh.com"], clientMessage.encryptionAlgorithmsServerToClient) + XCTAssertEqual(["hmac-sha2-256"], clientMessage.macAlgorithmsClientToServer) + XCTAssertEqual(["hmac-sha2-256"], clientMessage.macAlgorithmsServerToClient) + } + + func testAEADNegotiatesWhenPeerOffersOnlyUnusedMACs() throws { + let allocator = ByteBufferAllocator() + let loop = EmbeddedEventLoop() + + var client = SSHKeyExchangeStateMachine( + allocator: allocator, + loop: loop, + role: .client( + .init(userAuthDelegate: ExplodingAuthDelegate(), serverAuthDelegate: AcceptAllHostKeysDelegate()) + ), + remoteVersion: Constants.version, + protectionSchemes: [AES128GCMOpenSSHTransportProtection.self], + previousSessionIdentifier: nil + ) + var server = SSHKeyExchangeStateMachine( + allocator: allocator, + loop: loop, + role: .server(.init(hostKeys: [.init(ed25519Key: .init())], userAuthDelegate: DenyAllServerAuthDelegate())), + remoteVersion: Constants.version, + protectionSchemes: [AES128GCMOpenSSHTransportProtection.self], + previousSessionIdentifier: nil + ) + + var serverMessage = server.createKeyExchangeMessage() + serverMessage.macAlgorithmsClientToServer = ["hmac-sha2-512-etm@openssh.com", "hmac-sha2-256-etm@openssh.com"] + serverMessage.macAlgorithmsServerToClient = ["hmac-sha2-512-etm@openssh.com", "hmac-sha2-256-etm@openssh.com"] + + let clientMessage = client.createKeyExchangeMessage() + server.send(keyExchange: serverMessage) + client.send(keyExchange: clientMessage) + + try self.assertGeneratesNoMessage(server.handle(keyExchange: clientMessage)) + let ecdhInit = try assertGeneratesECDHKeyExchangeInit(client.handle(keyExchange: serverMessage)) + client.send(keyExchangeInit: ecdhInit) + + let ecdhReply = try assertGeneratesECDHKeyExchangeReplyAndNewKeys(server.handle(keyExchangeInit: ecdhInit)) + XCTAssertNoThrow(try server.send(keyExchangeReply: ecdhReply)) + let serverOutboundProtection = server.sendNewKeys() + + try self.assertGeneratesNewKeysSynchronously(client.handle(keyExchangeReply: ecdhReply)) + let clientOutboundProtection = client.sendNewKeys() + + let clientInboundProtection = try assertNoThrowWithValue(client.handleNewKeys()) + let serverInboundProtection = try assertNoThrowWithValue(server.handleNewKeys()) + + XCTAssertTrue(clientInboundProtection === clientOutboundProtection) + XCTAssertTrue(serverInboundProtection === serverOutboundProtection) + + self.assertCompatibleProtection(client: clientInboundProtection, server: serverInboundProtection) + XCTAssertTrue(clientInboundProtection is AES128GCMOpenSSHTransportProtection) + } + func testWeNegotiateTheClientsFirstPreference() throws { // Happy path key exchange test, but where the client would prefer AES128 and the server would prefer AES256. // We expect AES128, but the negotiation should be smooth. From b1feaef10479b8761ac055af79793d8983f54bc3 Mon Sep 17 00:00:00 2001 From: Mika Cohen Date: Mon, 25 May 2026 18:19:42 -0600 Subject: [PATCH 2/2] Avoid allocation during protection negotiation Motivation: Transport protection negotiation checks each mutually supported cipher while selecting a concrete protection scheme. The first version of the AEAD negotiation fix built an intermediate array of candidate schemes for each cipher, which is unnecessary work during key exchange. Modifications: Search the configured transport protection schemes directly when looking for AEAD and selected cipher/MAC schemes. Add test-only transport protection metadata overrides and a regression test that advertises a first mutual MAC for a different cipher, then verifies negotiation fails instead of skipping to a later MAC. Result: Transport protection negotiation avoids the extra allocation and the intended RFC-driven MAC selection behavior is covered by a focused regression test. --- .../SSHKeyExchangeStateMachine.swift | 10 +-- .../SSHKeyExchangeStateMachineTests.swift | 65 +++++++++++++++++++ Tests/NIOSSHTests/Utilities.swift | 6 +- 3 files changed, 74 insertions(+), 7 deletions(-) diff --git a/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift b/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift index 13e3da85..9a20a233 100644 --- a/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift +++ b/Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift @@ -507,9 +507,9 @@ struct SSHKeyExchangeStateMachine { continue } - let candidateSchemes = self.protectionSchemes.filter { $0.cipherName == encryption } - - if let aeadScheme = candidateSchemes.first(where: { $0.macName == nil }) { + if let aeadScheme = self.protectionSchemes.first(where: { + $0.cipherName == encryption && $0.macName == nil + }) { return aeadScheme } @@ -520,7 +520,9 @@ struct SSHKeyExchangeStateMachine { // MAC negotiation is independent in RFC 4253. Once the first mutual MAC is selected, both // peers will use it, so do not skip ahead to a later MAC just because this side lacks a // registered scheme for the selected cipher/MAC pair. - guard let scheme = candidateSchemes.first(where: { $0.macName == String(mac) }) else { + guard let scheme = self.protectionSchemes.first(where: { + $0.cipherName == encryption && $0.macName == String(mac) + }) else { throw NIOSSHError.keyExchangeNegotiationFailure } diff --git a/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift b/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift index 8157303f..505f21cf 100644 --- a/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift +++ b/Tests/NIOSSHTests/SSHKeyExchangeStateMachineTests.swift @@ -20,6 +20,26 @@ import XCTest @testable import NIOSSH final class SSHKeyExchangeStateMachineTests: XCTestCase { + final class ReviewSelectedCipherSupportedMACTransportProtection: TestTransportProtection { + override class var cipherName: String { + "review-selected-cipher" + } + + override class var macName: String? { + "review-supported-mac" + } + } + + final class ReviewOtherCipherFirstMACTransportProtection: TestTransportProtection { + override class var cipherName: String { + "review-other-cipher" + } + + override class var macName: String? { + "review-first-mac" + } + } + enum AssertionFailure: Error { case invalidMessageType case unexpectedMultipleMessages @@ -767,6 +787,51 @@ final class SSHKeyExchangeStateMachineTests: XCTestCase { XCTAssertTrue(clientInboundProtection is AES128GCMOpenSSHTransportProtection) } + func testMACNegotiationFailsWhenFirstMutualMACHasNoSchemeForSelectedCipher() throws { + let allocator = ByteBufferAllocator() + let loop = EmbeddedEventLoop() + let protectionSchemes: [NIOSSHTransportProtection.Type] = [ + ReviewSelectedCipherSupportedMACTransportProtection.self, + ReviewOtherCipherFirstMACTransportProtection.self, + ] + + var client = SSHKeyExchangeStateMachine( + allocator: allocator, + loop: loop, + role: .client( + .init(userAuthDelegate: ExplodingAuthDelegate(), serverAuthDelegate: AcceptAllHostKeysDelegate()) + ), + remoteVersion: Constants.version, + protectionSchemes: protectionSchemes, + previousSessionIdentifier: nil + ) + var server = SSHKeyExchangeStateMachine( + allocator: allocator, + loop: loop, + role: .server(.init(hostKeys: [.init(ed25519Key: .init())], userAuthDelegate: DenyAllServerAuthDelegate())), + remoteVersion: Constants.version, + protectionSchemes: protectionSchemes, + previousSessionIdentifier: nil + ) + + var serverMessage = server.createKeyExchangeMessage() + var clientMessage = client.createKeyExchangeMessage() + serverMessage.macAlgorithmsClientToServer = ["review-first-mac", "review-supported-mac"] + serverMessage.macAlgorithmsServerToClient = ["review-first-mac", "review-supported-mac"] + clientMessage.macAlgorithmsClientToServer = ["review-first-mac", "review-supported-mac"] + clientMessage.macAlgorithmsServerToClient = ["review-first-mac", "review-supported-mac"] + server.send(keyExchange: serverMessage) + client.send(keyExchange: clientMessage) + + XCTAssertEqual("review-selected-cipher", clientMessage.encryptionAlgorithmsClientToServer.first) + XCTAssertEqual("review-first-mac", clientMessage.macAlgorithmsClientToServer.first) + XCTAssertEqual("review-supported-mac", clientMessage.macAlgorithmsClientToServer.dropFirst().first) + + XCTAssertThrowsError(try server.handle(keyExchange: clientMessage)) { error in + XCTAssertEqual((error as? NIOSSHError)?.type, .keyExchangeNegotiationFailure) + } + } + func testWeNegotiateTheClientsFirstPreference() throws { // Happy path key exchange test, but where the client would prefer AES128 and the server would prefer AES256. // We expect AES128, but the negotiation should be smooth. diff --git a/Tests/NIOSSHTests/Utilities.swift b/Tests/NIOSSHTests/Utilities.swift index c821fe15..ceebfcc1 100644 --- a/Tests/NIOSSHTests/Utilities.swift +++ b/Tests/NIOSSHTests/Utilities.swift @@ -127,15 +127,15 @@ class TestTransportProtection: NIOSSHTransportProtection { true } - static var cipherName: String { + class var cipherName: String { "insecure-tiny-encription-cipher" } - static var macName: String? { + class var macName: String? { nil } - static var keySizes: ExpectedKeySizes { + class var keySizes: ExpectedKeySizes { .init(ivSize: 12, encryptionKeySize: 16, macKeySize: 16) }