Allow AEAD ciphers to ignore compatibility MAC negotiation

Sources/NIOSSH/Key Exchange/SSHKeyExchangeStateMachine.swift

@@ -384,34 +384,25 @@ 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
         }
 
         // Great, we have a protection scheme. Build the negotiation result.
         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,38 @@ 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
+            if let aeadScheme = self.protectionSchemes.first(where: {
+                $0.cipherName == encryption && $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 = self.protectionSchemes.first(where: {
+                $0.cipherName == encryption && $0.macName == String(mac)
+            }) else {
+                throw NIOSSHError.keyExchangeNegotiationFailure
+            }
+
+            return scheme
         }
 
-        return (encryption, mac)
+        throw NIOSSHError.keyExchangeNegotiationFailure
     }
 
     private mutating func addKeyExchangeInitMessagesToExchangeBytes(
@@ -576,9 +586,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 {

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
@@ -693,6 +713,125 @@ 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 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.

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)
     }