Check host key algorithms before continuing key exchange (#1642)

The library currently does not check for matching host key algorithms until needed
at the end of the key exchange, in contrast to other algorithm types which are checked
beforehand. This leads to confusing or uninformative errors, normally from the server
(correctly) closing the connection.

This change moves that check alongside the rest of them, and also improves the error
messages that arise from no matching algorithms.

Author: Rob-Hague

src/Renci.SshNet/Security/KeyExchange.cs

@@ -19,6 +19,7 @@ namespace Renci.SshNet.Security
     public abstract class KeyExchange : Algorithm, IKeyExchange
     {
         private readonly ILogger _logger;
+        private Func<byte[], KeyHostAlgorithm> _hostKeyAlgorithmFactory;
         private CipherInfo _clientCipherInfo;
         private CipherInfo _serverCipherInfo;
         private HashInfo _clientHashInfo;
@@ -81,6 +82,33 @@ public virtual void Start(Session session, KeyExchangeInitMessage message, bool
                 SendMessage(session.ClientInitMessage);
             }
 
+            // Determine host key algorithm
+            var hostKeyAlgorithmName = (from b in session.ConnectionInfo.HostKeyAlgorithms.Keys
+                                        from a in message.ServerHostKeyAlgorithms
+                                        where a == b
+                                        select a).FirstOrDefault();
+
+            if (_logger.IsEnabled(LogLevel.Trace))
+            {
+                _logger.LogTrace("[{SessionId}] Host key algorithm: we offer {WeOffer}",
+                    Session.SessionIdHex,
+                    session.ConnectionInfo.HostKeyAlgorithms.Keys.Join(","));
+
+                _logger.LogTrace("[{SessionId}] Host key algorithm: they offer {TheyOffer}",
+                    Session.SessionIdHex,
+                    message.ServerHostKeyAlgorithms.Join(","));
+            }
+
+            if (hostKeyAlgorithmName is null)
+            {
+                throw new SshConnectionException(
+                    $"No matching host key algorithm (server offers {message.ServerHostKeyAlgorithms.Join(",")})",
+                    DisconnectReason.KeyExchangeFailed);
+            }
+
+            session.ConnectionInfo.CurrentHostKeyAlgorithm = hostKeyAlgorithmName;
+            _hostKeyAlgorithmFactory = session.ConnectionInfo.HostKeyAlgorithms[hostKeyAlgorithmName];
+
             // Determine client encryption algorithm
             var clientEncryptionAlgorithmName = (from b in session.ConnectionInfo.Encryptions.Keys
                                                  from a in message.EncryptionAlgorithmsClientToServer
@@ -98,9 +126,11 @@ from a in message.EncryptionAlgorithmsClientToServer
                     message.EncryptionAlgorithmsClientToServer.Join(","));
             }
 
-            if (string.IsNullOrEmpty(clientEncryptionAlgorithmName))
+            if (clientEncryptionAlgorithmName is null)
             {
-                throw new SshConnectionException("Client encryption algorithm not found", DisconnectReason.KeyExchangeFailed);
+                throw new SshConnectionException(
+                    $"No matching client encryption algorithm (server offers {message.EncryptionAlgorithmsClientToServer.Join(",")})",
+                    DisconnectReason.KeyExchangeFailed);
             }
 
             session.ConnectionInfo.CurrentClientEncryption = clientEncryptionAlgorithmName;
@@ -123,9 +153,11 @@ from a in message.EncryptionAlgorithmsServerToClient
                     message.EncryptionAlgorithmsServerToClient.Join(","));
             }
 
-            if (string.IsNullOrEmpty(serverDecryptionAlgorithmName))
+            if (serverDecryptionAlgorithmName is null)
             {
-                throw new SshConnectionException("Server decryption algorithm not found", DisconnectReason.KeyExchangeFailed);
+                throw new SshConnectionException(
+                    $"No matching server encryption algorithm (server offers {message.EncryptionAlgorithmsServerToClient.Join(",")})",
+                    DisconnectReason.KeyExchangeFailed);
             }
 
             session.ConnectionInfo.CurrentServerEncryption = serverDecryptionAlgorithmName;
@@ -150,9 +182,11 @@ from a in message.MacAlgorithmsClientToServer
                         message.MacAlgorithmsClientToServer.Join(","));
                 }
 
-                if (string.IsNullOrEmpty(clientHmacAlgorithmName))
+                if (clientHmacAlgorithmName is null)
                 {
-                    throw new SshConnectionException("Client HMAC algorithm not found", DisconnectReason.KeyExchangeFailed);
+                    throw new SshConnectionException(
+                        $"No matching client MAC algorithm (server offers {message.MacAlgorithmsClientToServer.Join(",")})",
+                        DisconnectReason.KeyExchangeFailed);
                 }
 
                 session.ConnectionInfo.CurrentClientHmacAlgorithm = clientHmacAlgorithmName;
@@ -178,9 +212,11 @@ from a in message.MacAlgorithmsServerToClient
                         message.MacAlgorithmsServerToClient.Join(","));
                 }
 
-                if (string.IsNullOrEmpty(serverHmacAlgorithmName))
+                if (serverHmacAlgorithmName is null)
                 {
-                    throw new SshConnectionException("Server HMAC algorithm not found", DisconnectReason.KeyExchangeFailed);
+                    throw new SshConnectionException(
+                        $"No matching server MAC algorithm (server offers {message.MacAlgorithmsServerToClient.Join(",")})",
+                        DisconnectReason.KeyExchangeFailed);
                 }
 
                 session.ConnectionInfo.CurrentServerHmacAlgorithm = serverHmacAlgorithmName;
@@ -204,9 +240,11 @@ from a in message.CompressionAlgorithmsClientToServer
                     message.CompressionAlgorithmsClientToServer.Join(","));
             }
 
-            if (string.IsNullOrEmpty(compressionAlgorithmName))
+            if (compressionAlgorithmName is null)
             {
-                throw new SshConnectionException("Compression algorithm not found", DisconnectReason.KeyExchangeFailed);
+                throw new SshConnectionException(
+                    $"No matching client compression algorithm (server offers {message.CompressionAlgorithmsClientToServer.Join(",")})",
+                    DisconnectReason.KeyExchangeFailed);
             }
 
             session.ConnectionInfo.CurrentClientCompressionAlgorithm = compressionAlgorithmName;
@@ -229,9 +267,11 @@ from a in message.CompressionAlgorithmsServerToClient
                     message.CompressionAlgorithmsServerToClient.Join(","));
             }
 
-            if (string.IsNullOrEmpty(decompressionAlgorithmName))
+            if (decompressionAlgorithmName is null)
             {
-                throw new SshConnectionException("Decompression algorithm not found", DisconnectReason.KeyExchangeFailed);
+                throw new SshConnectionException(
+                    $"No matching server compression algorithm (server offers {message.CompressionAlgorithmsServerToClient.Join(",")})",
+                    DisconnectReason.KeyExchangeFailed);
             }
 
             session.ConnectionInfo.CurrentServerCompressionAlgorithm = decompressionAlgorithmName;
@@ -245,7 +285,7 @@ public virtual void Finish()
         {
             if (!ValidateExchangeHash())
             {
-                throw new SshConnectionException("Key exchange negotiation failed.", DisconnectReason.KeyExchangeFailed);
+                throw new SshConnectionException("Host key could not be verified.", DisconnectReason.KeyExchangeFailed);
             }
 
             SendMessage(new NewKeysMessage());
@@ -449,40 +489,9 @@ private protected bool ValidateExchangeHash(byte[] encodedKey, byte[] encodedSig
         {
             var exchangeHash = CalculateHash();
 
-            // We need to inspect both the key and signature format identifers to find the correct
-            // HostAlgorithm instance. Example cases:
-
-            // Key identifier                Signature identifier  | Algorithm name
-            // ssh-rsa                       ssh-rsa               | ssh-rsa
-            // ssh-rsa                       rsa-sha2-256          | rsa-sha2-256
-            // [email protected]  ssh-rsa               | [email protected]
-            // [email protected]  rsa-sha2-256          | [email protected]
-
-            var signatureData = new KeyHostAlgorithm.SignatureKeyData();
-            signatureData.Load(encodedSignature);
-
-            string keyName;
-            using (var keyReader = new SshDataStream(encodedKey))
-            {
-                keyName = keyReader.ReadString();
-            }
-
-            string algorithmName;
-
-            if (signatureData.AlgorithmName.StartsWith("rsa-sha2", StringComparison.Ordinal))
-            {
-                algorithmName = keyName.Replace("ssh-rsa", signatureData.AlgorithmName);
-            }
-            else
-            {
-                algorithmName = keyName;
-            }
-
-            var keyAlgorithm = Session.ConnectionInfo.HostKeyAlgorithms[algorithmName](encodedKey);
-
-            Session.ConnectionInfo.CurrentHostKeyAlgorithm = algorithmName;
+            var keyAlgorithm = _hostKeyAlgorithmFactory(encodedKey);
 
-            return keyAlgorithm.VerifySignatureBlob(exchangeHash, signatureData.Signature) && CanTrustHostKey(keyAlgorithm);
+            return keyAlgorithm.VerifySignature(exchangeHash, encodedSignature) && CanTrustHostKey(keyAlgorithm);
         }
 
         /// <summary>

src/Renci.SshNet/ServiceFactory.cs

@@ -101,7 +101,7 @@ from s in serverAlgorithms
 
             if (keyExchangeAlgorithmFactory is null)
             {
-                throw new SshConnectionException("Failed to negotiate key exchange algorithm.", DisconnectReason.KeyExchangeFailed);
+                throw new SshConnectionException($"No matching key exchange algorithm (server offers {serverAlgorithms.Join(",")})", DisconnectReason.KeyExchangeFailed);
             }
 
             return keyExchangeAlgorithmFactory();

test/Renci.SshNet.IntegrationTests/ConnectivityTests.cs

@@ -415,16 +415,24 @@ public void Common_HostKeyValidation_Failure()
             {
                 client.HostKeyReceived += (sender, e) => { e.CanTrust = false; };
 
-                try
-                {
-                    client.Connect();
-                    Assert.Fail();
-                }
-                catch (SshConnectionException ex)
-                {
-                    Assert.IsNull(ex.InnerException);
-                    Assert.AreEqual("Key exchange negotiation failed.", ex.Message);
-                }
+                var ex = Assert.Throws<SshConnectionException>(client.Connect);
+
+                Assert.AreEqual(DisconnectReason.KeyExchangeFailed, ex.DisconnectReason);
+                Assert.AreEqual("Host key could not be verified.", ex.Message);
+            }
+        }
+
+        [TestMethod]
+        public void Common_HostKeyAlgorithms_NoMatch()
+        {
+            using (var client = new SshClient(_connectionInfoFactory.Create()))
+            {
+                client.ConnectionInfo.HostKeyAlgorithms.Clear();
+
+                var ex = Assert.Throws<SshConnectionException>(client.Connect);
+
+                Assert.AreEqual(DisconnectReason.KeyExchangeFailed, ex.DisconnectReason);
+                Assert.IsTrue(ex.Message.StartsWith("No matching host key algorithm"), ex.Message);
             }
         }