Support ETM (Encrypt-then-MAC) variants for HMAC (#1316)

scott-xu · web-flow · commit 7cfe62b9f12a · 2024-02-16T13:24:54.000+01:00
* Support ETM (Encrypt-then-MAC) variants for HMAC

* Support ETM (Encrypt-then-MAC) variants for HMAC

* Remove `ETM` property from `HashInfo`

* Add support for HmacMd5Etm, HmacMd5_96_Etm, HmacSha1Etm and HmacSha1_96_Etm
Explicitly specify etm even if false

* Add Encrypt-then-MAC variants to README.md

* Add back empty span to prevent auto link

* Store ETM in `HashInfo`;
Change `HMAC Create[...]Hash()` to `HashAlgorithm Create[...]Hash(out bool isEncryptThenMAC)`
diff --git a/README.md b/README.md
@@ -115,6 +115,12 @@ Private keys can be encrypted using one of the following cipher methods:
 * hmac-sha2-512-96
 * hmac-ripemd160
 * hmac-ripemd160<span></span>@openssh.com
+* hmac-md5-etm<span></span>@openssh.com
+* hmac-md5-96-etm<span></span>@openssh.com
+* hmac-sha1-etm<span></span>@openssh.com
+* hmac-sha1-96-etm<span></span>@openssh.com
+* hmac-sha2-256-etm<span></span>@openssh.com
+* hmac-sha2-512-etm<span></span>@openssh.com
 
 ## Framework Support
 **SSH.NET** supports the following target frameworks:
diff --git a/src/Renci.SshNet/ConnectionInfo.cs b/src/Renci.SshNet/ConnectionInfo.cs
@@ -376,16 +376,24 @@ public ConnectionInfo(string host, int port, string username, ProxyTypes proxyTy
 #pragma warning disable IDE0200 // Remove unnecessary lambda expression; We want to prevent instantiating the HashAlgorithm objects.
             HmacAlgorithms = new Dictionary<string, HashInfo>
                 {
-                    { "hmac-sha2-256", new HashInfo(32*8, key => CryptoAbstraction.CreateHMACSHA256(key)) },
-                    { "hmac-sha2-512", new HashInfo(64 * 8, key => CryptoAbstraction.CreateHMACSHA512(key)) },
-                    { "hmac-sha2-512-96", new HashInfo(64 * 8,  key => CryptoAbstraction.CreateHMACSHA512(key, 96)) },
-                    { "hmac-sha2-256-96", new HashInfo(32*8, key => CryptoAbstraction.CreateHMACSHA256(key, 96)) },
-                    { "hmac-ripemd160", new HashInfo(160, key => CryptoAbstraction.CreateHMACRIPEMD160(key)) },
-                    { "hmac-ripemd160@openssh.com", new HashInfo(160, key => CryptoAbstraction.CreateHMACRIPEMD160(key)) },
-                    { "hmac-sha1", new HashInfo(20*8, key => CryptoAbstraction.CreateHMACSHA1(key)) },
-                    { "hmac-sha1-96", new HashInfo(20*8, key => CryptoAbstraction.CreateHMACSHA1(key, 96)) },
-                    { "hmac-md5", new HashInfo(16*8, key => CryptoAbstraction.CreateHMACMD5(key)) },
-                    { "hmac-md5-96", new HashInfo(16*8, key => CryptoAbstraction.CreateHMACMD5(key, 96)) },
+                    /* Encrypt-and-MAC (encrypt-and-authenticate) variants */
+                    { "hmac-sha2-256", new HashInfo(32*8, key => CryptoAbstraction.CreateHMACSHA256(key), isEncryptThenMAC: false) },
+                    { "hmac-sha2-512", new HashInfo(64*8, key => CryptoAbstraction.CreateHMACSHA512(key), isEncryptThenMAC: false) },
+                    { "hmac-sha2-512-96", new HashInfo(64*8,  key => CryptoAbstraction.CreateHMACSHA512(key, 96), isEncryptThenMAC: false) },
+                    { "hmac-sha2-256-96", new HashInfo(32*8, key => CryptoAbstraction.CreateHMACSHA256(key, 96), isEncryptThenMAC: false) },
+                    { "hmac-ripemd160", new HashInfo(160, key => CryptoAbstraction.CreateHMACRIPEMD160(key), isEncryptThenMAC: false) },
+                    { "hmac-ripemd160@openssh.com", new HashInfo(160, key => CryptoAbstraction.CreateHMACRIPEMD160(key), isEncryptThenMAC: false) },
+                    { "hmac-sha1", new HashInfo(20*8, key => CryptoAbstraction.CreateHMACSHA1(key), isEncryptThenMAC: false) },
+                    { "hmac-sha1-96", new HashInfo(20*8, key => CryptoAbstraction.CreateHMACSHA1(key, 96), isEncryptThenMAC: false) },
+                    { "hmac-md5", new HashInfo(16*8, key => CryptoAbstraction.CreateHMACMD5(key), isEncryptThenMAC: false) },
+                    { "hmac-md5-96", new HashInfo(16*8, key => CryptoAbstraction.CreateHMACMD5(key, 96), isEncryptThenMAC: false) },
+                    /* Encrypt-then-MAC variants */
+                    { "hmac-sha2-256-etm@openssh.com", new HashInfo(32*8, key => CryptoAbstraction.CreateHMACSHA256(key), isEncryptThenMAC: true) },
+                    { "hmac-sha2-512-etm@openssh.com", new HashInfo(64*8, key => CryptoAbstraction.CreateHMACSHA512(key), isEncryptThenMAC: true) },
+                    { "hmac-sha1-etm@openssh.com", new HashInfo(20*8, key => CryptoAbstraction.CreateHMACSHA1(key), isEncryptThenMAC: true) },
+                    { "hmac-sha1-96-etm@openssh.com", new HashInfo(20*8, key => CryptoAbstraction.CreateHMACSHA1(key, 96), isEncryptThenMAC: true) },
+                    { "hmac-md5-etm@openssh.com", new HashInfo(16*8, key => CryptoAbstraction.CreateHMACMD5(key), isEncryptThenMAC: true) },
+                    { "hmac-md5-96-etm@openssh.com", new HashInfo(16*8, key => CryptoAbstraction.CreateHMACMD5(key, 96), isEncryptThenMAC: true) },
                 };
 #pragma warning restore IDE0200 // Remove unnecessary lambda expression
 
diff --git a/src/Renci.SshNet/HashInfo.cs b/src/Renci.SshNet/HashInfo.cs
@@ -1,5 +1,6 @@
 ﻿using System;
 using System.Security.Cryptography;
+
 using Renci.SshNet.Common;
 
 namespace Renci.SshNet
@@ -17,6 +18,14 @@ public class HashInfo
         /// </value>
         public int KeySize { get; private set; }
 
+        /// <summary>
+        /// Gets a value indicating whether enable encrypt-then-MAC or use encrypt-and-MAC.
+        /// </summary>
+        /// <value>
+        /// <see langword="true"/> to enable encrypt-then-MAC, <see langword="false"/> to use encrypt-and-MAC.
+        /// </value>
+        public bool IsEncryptThenMAC { get; private set; }
+
         /// <summary>
         /// Gets the cipher.
         /// </summary>
@@ -27,10 +36,12 @@ public class HashInfo
         /// </summary>
         /// <param name="keySize">Size of the key.</param>
         /// <param name="hash">The hash algorithm to use for a given key.</param>
-        public HashInfo(int keySize, Func<byte[], HashAlgorithm> hash)
+        /// <param name="isEncryptThenMAC"><see langword="true"/> to enable encrypt-then-MAC, <see langword="false"/> to use encrypt-and-MAC.</param>
+        public HashInfo(int keySize, Func<byte[], HashAlgorithm> hash, bool isEncryptThenMAC = false)
         {
             KeySize = keySize;
             HashAlgorithm = key => hash(key.Take(KeySize / 8));
+            IsEncryptThenMAC = isEncryptThenMAC;
         }
     }
 }
diff --git a/src/Renci.SshNet/Messages/Message.cs b/src/Renci.SshNet/Messages/Message.cs
@@ -37,7 +37,7 @@ protected override void WriteBytes(SshDataStream stream)
             base.WriteBytes(stream);
         }
 
-        internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor)
+        internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor, bool isEncryptThenMAC = false)
         {
             const int outboundPacketSequenceSize = 4;
 
@@ -78,7 +78,9 @@ internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor)
                     var packetLength = messageLength + 4 + 1;
 
                     // determine the padding length
-                    var paddingLength = GetPaddingLength(paddingMultiplier, packetLength);
+                    // in Encrypt-then-MAC mode, the length field is not encrypted, so we should keep it out of the
+                    // padding length calculation
+                    var paddingLength = GetPaddingLength(paddingMultiplier, isEncryptThenMAC ? packetLength - 4 : packetLength);
 
                     // add padding bytes
                     var paddingBytes = new byte[paddingLength];
@@ -104,7 +106,9 @@ internal byte[] GetPacket(byte paddingMultiplier, Compressor compressor)
                 var packetLength = messageLength + 4 + 1;
 
                 // determine the padding length
-                var paddingLength = GetPaddingLength(paddingMultiplier, packetLength);
+                // in Encrypt-then-MAC mode, the length field is not encrypted, so we should keep it out of the
+                // padding length calculation
+                var paddingLength = GetPaddingLength(paddingMultiplier, isEncryptThenMAC ? packetLength - 4 : packetLength);
 
                 var packetDataLength = GetPacketDataLength(messageLength, paddingLength);
 
diff --git a/src/Renci.SshNet/Security/IKeyExchange.cs b/src/Renci.SshNet/Security/IKeyExchange.cs
@@ -66,18 +66,20 @@ public interface IKeyExchange : IDisposable
         /// <summary>
         /// Creates the server-side hash algorithm to use.
         /// </summary>
+        /// <param name="isEncryptThenMAC"><see langword="true"/> to enable encrypt-then-MAC, <see langword="false"/> to use encrypt-and-MAC.</param>
         /// <returns>
         /// The server hash algorithm.
         /// </returns>
-        HashAlgorithm CreateServerHash();
+        HashAlgorithm CreateServerHash(out bool isEncryptThenMAC);
 
         /// <summary>
         /// Creates the client-side hash algorithm to use.
         /// </summary>
+        /// <param name="isEncryptThenMAC"><see langword="true"/> to enable encrypt-then-MAC, <see langword="false"/> to use encrypt-and-MAC.</param>
         /// <returns>
         /// The client hash algorithm.
         /// </returns>
-        HashAlgorithm CreateClientHash();
+        HashAlgorithm CreateClientHash(out bool isEncryptThenMAC);
 
         /// <summary>
         /// Creates the compression algorithm to use to deflate data.
diff --git a/src/Renci.SshNet/Security/KeyExchange.cs b/src/Renci.SshNet/Security/KeyExchange.cs
@@ -103,7 +103,7 @@ from a in message.MacAlgorithmsClientToServer
                                            select a).FirstOrDefault();
             if (string.IsNullOrEmpty(clientHmacAlgorithmName))
             {
-                throw new SshConnectionException("Server HMAC algorithm not found", DisconnectReason.KeyExchangeFailed);
+                throw new SshConnectionException("Client HMAC algorithm not found", DisconnectReason.KeyExchangeFailed);
             }
 
             session.ConnectionInfo.CurrentClientHmacAlgorithm = clientHmacAlgorithmName;
@@ -218,11 +218,14 @@ public Cipher CreateClientCipher()
         /// <summary>
         /// Creates the server side hash algorithm to use.
         /// </summary>
+        /// <param name="isEncryptThenMAC"><see langword="true"/> to enable encrypt-then-MAC, <see langword="false"/> to use encrypt-and-MAC.</param>
         /// <returns>
         /// The server-side hash algorithm.
         /// </returns>
-        public HashAlgorithm CreateServerHash()
+        public HashAlgorithm CreateServerHash(out bool isEncryptThenMAC)
         {
+            isEncryptThenMAC = _serverHashInfo.IsEncryptThenMAC;
+
             // Resolve Session ID
             var sessionId = Session.SessionId ?? ExchangeHash;
 
@@ -241,11 +244,14 @@ public HashAlgorithm CreateServerHash()
         /// <summary>
         /// Creates the client side hash algorithm to use.
         /// </summary>
+        /// <param name="isEncryptThenMAC"><see langword="true"/> to enable encrypt-then-MAC, <see langword="false"/> to use encrypt-and-MAC.</param>
         /// <returns>
         /// The client-side hash algorithm.
         /// </returns>
-        public HashAlgorithm CreateClientHash()
+        public HashAlgorithm CreateClientHash(out bool isEncryptThenMAC)
         {
+            isEncryptThenMAC = _clientHashInfo.IsEncryptThenMAC;
+
             // Resolve Session ID
             var sessionId = Session.SessionId ?? ExchangeHash;
 
diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs
@@ -176,6 +176,10 @@ public class Session : ISession
 
         private HashAlgorithm _clientMac;
 
+        private bool _serverEtm;
+
+        private bool _clientEtm;
+
         private Cipher _clientCipher;
 
         private Cipher _serverCipher;
@@ -1054,7 +1058,7 @@ internal void SendMessage(Message message)
             DiagnosticAbstraction.Log(string.Format("[{0}] Sending message '{1}' to server: '{2}'.", ToHex(SessionId), message.GetType().Name, message));
 
             var paddingMultiplier = _clientCipher is null ? (byte) 8 : Math.Max((byte) 8, _serverCipher.MinimumSize);
-            var packetData = message.GetPacket(paddingMultiplier, _clientCompression);
+            var packetData = message.GetPacket(paddingMultiplier, _clientCompression, _clientMac != null && _clientEtm);
 
             // take a write lock to ensure the outbound packet sequence number is incremented
             // atomically, and only after the packet has actually been sent
@@ -1063,7 +1067,7 @@ internal void SendMessage(Message message)
                 byte[] hash = null;
                 var packetDataOffset = 4; // first four bytes are reserved for outbound packet sequence
 
-                if (_clientMac != null)
+                if (_clientMac != null && !_clientEtm)
                 {
                     // write outbound packet sequence to start of packet data
                     Pack.UInt32ToBigEndian(_outboundPacketSequence, packetData);
@@ -1075,8 +1079,29 @@ internal void SendMessage(Message message)
                 // Encrypt packet data
                 if (_clientCipher != null)
                 {
-                    packetData = _clientCipher.Encrypt(packetData, packetDataOffset, packetData.Length - packetDataOffset);
-                    packetDataOffset = 0;
+                    if (_clientMac != null && _clientEtm)
+                    {
+                        // The length of the "packet length" field in bytes
+                        const int packetLengthFieldLength = 4;
+
+                        var encryptedData = _clientCipher.Encrypt(packetData, packetDataOffset + packetLengthFieldLength, packetData.Length - packetDataOffset - packetLengthFieldLength);
+
+                        Array.Resize(ref packetData, packetDataOffset + packetLengthFieldLength + encryptedData.Length);
+
+                        // write outbound packet sequence to start of packet data
+                        Pack.UInt32ToBigEndian(_outboundPacketSequence, packetData);
+
+                        // write encrypted data
+                        Buffer.BlockCopy(encryptedData, 0, packetData, packetDataOffset + packetLengthFieldLength, encryptedData.Length);
+
+                        // calculate packet hash
+                        hash = _clientMac.ComputeHash(packetData);
+                    }
+                    else
+                    {
+                        packetData = _clientCipher.Encrypt(packetData, packetDataOffset, packetData.Length - packetDataOffset);
+                        packetDataOffset = 0;
+                    }
                 }
 
                 if (packetData.Length > MaximumSshPacketSize)
@@ -1194,8 +1219,22 @@ private Message ReceiveMessage(Socket socket)
             // The length of the "padding length" field in bytes
             const int paddingLengthFieldLength = 1;
 
-            // Determine the size of the first block, which is 8 or cipher block size (whichever is larger) bytes
-            var blockSize = _serverCipher is null ? (byte) 8 : Math.Max((byte) 8, _serverCipher.MinimumSize);
+            int blockSize;
+
+            // Determine the size of the first block which is 8 or cipher block size (whichever is larger) bytes
+            // The "packet length" field is not encrypted in ETM.
+            if (_serverMac != null && _serverEtm)
+            {
+                blockSize = (byte) 4;
+            }
+            else if (_serverCipher != null)
+            {
+                blockSize = Math.Max((byte) 8, _serverCipher.MinimumSize);
+            }
+            else
+            {
+                blockSize = (byte) 8;
+            }
 
             var serverMacLength = _serverMac != null ? _serverMac.HashSize/8 : 0;
 
@@ -1215,7 +1254,7 @@ private Message ReceiveMessage(Socket socket)
                     return null;
                 }
 
-                if (_serverCipher != null)
+                if (_serverCipher != null && (_serverMac == null || !_serverEtm))
                 {
                     firstBlock = _serverCipher.Decrypt(firstBlock);
                 }
@@ -1257,6 +1296,20 @@ private Message ReceiveMessage(Socket socket)
                 }
             }
 
+            // validate encrypted message against MAC
+            if (_serverMac != null && _serverEtm)
+            {
+                var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength);
+                var serverHash = data.Take(data.Length - serverMacLength, serverMacLength);
+
+                // TODO Add IsEqualTo overload that takes left+right index and number of bytes to compare.
+                // TODO That way we can eliminate the extra allocation of the Take above.
+                if (!serverHash.IsEqualTo(clientHash))
+                {
+                    throw new SshConnectionException("MAC error", DisconnectReason.MacError);
+                }
+            }
+
             if (_serverCipher != null)
             {
                 var numberOfBytesToDecrypt = data.Length - (blockSize + inboundPacketSequenceLength + serverMacLength);
@@ -1271,8 +1324,8 @@ private Message ReceiveMessage(Socket socket)
             var messagePayloadLength = (int) packetLength - paddingLength - paddingLengthFieldLength;
             var messagePayloadOffset = inboundPacketSequenceLength + packetLengthFieldLength + paddingLengthFieldLength;
 
-            // validate message against MAC
-            if (_serverMac != null)
+            // validate decrypted message against MAC
+            if (_serverMac != null && !_serverEtm)
             {
                 var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength);
                 var serverHash = data.Take(data.Length - serverMacLength, serverMacLength);
@@ -1472,8 +1525,8 @@ internal void OnNewKeysReceived(NewKeysMessage message)
             // Update negotiated algorithms
             _serverCipher = _keyExchange.CreateServerCipher();
             _clientCipher = _keyExchange.CreateClientCipher();
-            _serverMac = _keyExchange.CreateServerHash();
-            _clientMac = _keyExchange.CreateClientHash();
+            _serverMac = _keyExchange.CreateServerHash(out _serverEtm);
+            _clientMac = _keyExchange.CreateClientHash(out _clientEtm);
             _clientCompression = _keyExchange.CreateCompressor();
             _serverDecompression = _keyExchange.CreateDecompressor();
 
diff --git a/test/Renci.SshNet.IntegrationTests/HmacTests.cs b/test/Renci.SshNet.IntegrationTests/HmacTests.cs
@@ -58,6 +58,42 @@ public void HmacSha2_512()
             DoTest(MessageAuthenticationCodeAlgorithm.HmacSha2_512);
         }
 
+        [TestMethod]
+        public void HmacMd5_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacMd5Etm);
+        }
+
+        [TestMethod]
+        public void HmacMd5_96_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacMd5_96_Etm);
+        }
+
+        [TestMethod]
+        public void HmacSha1_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacSha1Etm);
+        }
+
+        [TestMethod]
+        public void HmacSha1_96_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacSha1_96_Etm);
+        }
+
+        [TestMethod]
+        public void HmacSha2_256_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacSha2_256_Etm);
+        }
+
+        [TestMethod]
+        public void HmacSha2_512_Etm()
+        {
+            DoTest(MessageAuthenticationCodeAlgorithm.HmacSha2_512_Etm);
+        }
+
         private void DoTest(MessageAuthenticationCodeAlgorithm macAlgorithm)
         {
             _remoteSshdConfig.ClearMessageAuthenticationCodeAlgorithms()
diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs
@@ -215,10 +215,18 @@ private void SetupMocks()
                                 .Returns((Cipher) null);
             _ = _keyExchangeMock.Setup(p => p.CreateClientCipher())
                                 .Returns((Cipher) null);
-            _ = _keyExchangeMock.Setup(p => p.CreateServerHash())
-                                .Returns((HashAlgorithm) null);
-            _ = _keyExchangeMock.Setup(p => p.CreateClientHash())
-                                .Returns((HashAlgorithm) null);
+            _ = _keyExchangeMock.Setup(p => p.CreateServerHash(out It.Ref<bool>.IsAny))
+                                .Returns((ref bool serverEtm) =>
+                                {
+                                    serverEtm = false;
+                                    return (HashAlgorithm) null;
+                                });
+            _ = _keyExchangeMock.Setup(p => p.CreateClientHash(out It.Ref<bool>.IsAny))
+                                .Returns((ref bool clientEtm) =>
+                                {
+                                    clientEtm = false;
+                                    return (HashAlgorithm) null;
+                                });
             _ = _keyExchangeMock.Setup(p => p.CreateCompressor())
                                 .Returns((Compressor) null);
             _ = _keyExchangeMock.Setup(p => p.CreateDecompressor())
diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerAndClientDisconnectRace.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerAndClientDisconnectRace.cs
