AesGcmCipher uses BouncyCastle as a fallback if BCL does not support. (#1450)

scott-xu · WojciechNagorski · web-flow · commit 7fc88b570d1c · 2024-08-19T14:13:10.000+02:00
* [AesGcmCipher] Use BouncyCastle as a fallback if BCL does not support. * Switch back to collection initializer * Remove conditional compilation * Throw SshConnectionException with Reason MacError when authentication tag mismatch * Separate BCL and BouncyCastle implementation * Update AesGcmCipher.BclImpl.cs * Naming enhancement * Remove empty line * Disable S1199. See sshnet/SSH.NET#1371 (comment) * Set InnerException when MAC error. Remove Message check. * Store KeyParameter as private field * Use GcmCipher.ProcessAadBytes to avoid the copy of associated data * Move nonce to constructor to avoid creating AeadParameters each packet * Use const int for tag size --------- Co-authored-by: Wojciech Nagórski <wojtpl2@gmail.com>
diff --git a/.editorconfig b/.editorconfig
@@ -87,6 +87,10 @@ dotnet_diagnostic.S1144.severity = none
 # This is a duplicate of IDE0060.
 dotnet_diagnostic.S1172.severity = none
 
+# S1199: Nested code blocks should not be used
+# https://rules.sonarsource.com/csharp/RSPEC-1199
+dotnet_diagnostic.S1199.severity = none
+
 # S1481: Unused local variables should be removed
 # https://rules.sonarsource.com/csharp/RSPEC-1481
 #
diff --git a/README.md b/README.md
@@ -70,8 +70,8 @@ The main types provided by this library are:
 * aes128-ctr
 * aes192-ctr
 * aes256-ctr
-* aes128-gcm<span></span>@openssh.com (.NET 6 and higher)
-* aes256-gcm<span></span>@openssh.com (.NET 6 and higher)
+* aes128-gcm<span></span>@openssh.com
+* aes256-gcm<span></span>@openssh.com
 * chacha20-poly1305<span></span>@openssh.com
 * aes128-cbc
 * aes192-cbc
diff --git a/appveyor.yml b/appveyor.yml
@@ -32,7 +32,7 @@ for:
     # some coverage until a proper solution for running the .NET Framework integration tests in CI is found.
     # Running all the tests causes problems which are not worth solving in this rare configuration.
     # See https://github.com/sshnet/SSH.NET/pull/1462 and related links
-    - sh: dotnet test -f net48 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_integration_test_net_48_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_integration_test_net_48_coverage.xml --filter "Name~Ecdh|Name~Zlib" test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj
+    - sh: dotnet test -f net48 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_integration_test_net_48_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_integration_test_net_48_coverage.xml --filter "Name~Ecdh|Name~Zlib|Name~Gcm" test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj
 
 -
   matrix:
diff --git a/src/Renci.SshNet/ConnectionInfo.cs b/src/Renci.SshNet/ConnectionInfo.cs
@@ -381,22 +381,19 @@ public ConnectionInfo(string host, int port, string username, ProxyTypes proxyTy
                     { "diffie-hellman-group1-sha1", () => new KeyExchangeDiffieHellmanGroup1Sha1() },
                 };
 
-            Encryptions = new Dictionary<string, CipherInfo>();
-            Encryptions.Add("aes128-ctr", new CipherInfo(128, (key, iv) => new AesCipher(key, iv, AesCipherMode.CTR, pkcs7Padding: false)));
-            Encryptions.Add("aes192-ctr", new CipherInfo(192, (key, iv) => new AesCipher(key, iv, AesCipherMode.CTR, pkcs7Padding: false)));
-            Encryptions.Add("aes256-ctr", new CipherInfo(256, (key, iv) => new AesCipher(key, iv, AesCipherMode.CTR, pkcs7Padding: false)));
-#if NET6_0_OR_GREATER
-            if (AesGcm.IsSupported)
-            {
-                Encryptions.Add("aes128-gcm@openssh.com", new CipherInfo(128, (key, iv) => new AesGcmCipher(key, iv), isAead: true));
-                Encryptions.Add("aes256-gcm@openssh.com", new CipherInfo(256, (key, iv) => new AesGcmCipher(key, iv), isAead: true));
-            }
-#endif
-            Encryptions.Add("chacha20-poly1305@openssh.com", new CipherInfo(512, (key, iv) => new ChaCha20Poly1305Cipher(key), isAead: true));
-            Encryptions.Add("aes128-cbc", new CipherInfo(128, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)));
-            Encryptions.Add("aes192-cbc", new CipherInfo(192, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)));
-            Encryptions.Add("aes256-cbc", new CipherInfo(256, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)));
-            Encryptions.Add("3des-cbc", new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CbcCipherMode(iv), padding: null)));
+            Encryptions = new Dictionary<string, CipherInfo>
+                {
+                    { "aes128-ctr", new CipherInfo(128, (key, iv) => new AesCipher(key, iv, AesCipherMode.CTR, pkcs7Padding: false)) },
+                    { "aes192-ctr", new CipherInfo(192, (key, iv) => new AesCipher(key, iv, AesCipherMode.CTR, pkcs7Padding: false)) },
+                    { "aes256-ctr", new CipherInfo(256, (key, iv) => new AesCipher(key, iv, AesCipherMode.CTR, pkcs7Padding: false)) },
+                    { "aes128-gcm@openssh.com", new CipherInfo(128, (key, iv) => new AesGcmCipher(key, iv), isAead: true) },
+                    { "aes256-gcm@openssh.com", new CipherInfo(256, (key, iv) => new AesGcmCipher(key, iv), isAead: true) },
+                    { "chacha20-poly1305@openssh.com", new CipherInfo(512, (key, iv) => new ChaCha20Poly1305Cipher(key), isAead: true) },
+                    { "aes128-cbc", new CipherInfo(128, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)) },
+                    { "aes192-cbc", new CipherInfo(192, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)) },
+                    { "aes256-cbc", new CipherInfo(256, (key, iv) => new AesCipher(key, iv, AesCipherMode.CBC, pkcs7Padding: false)) },
+                    { "3des-cbc", new CipherInfo(192, (key, iv) => new TripleDesCipher(key, new CbcCipherMode(iv), padding: null)) },
+                };
 
             HmacAlgorithms = new Dictionary<string, HashInfo>
                 {
diff --git a/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BclImpl.cs b/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BclImpl.cs
@@ -0,0 +1,72 @@
+#if NET6_0_OR_GREATER
+using System;
+using System.Security.Cryptography;
+
+using Renci.SshNet.Common;
+using Renci.SshNet.Messages.Transport;
+
+namespace Renci.SshNet.Security.Cryptography.Ciphers
+{
+    internal partial class AesGcmCipher
+    {
+        private sealed class BclImpl : Impl
+        {
+            private readonly AesGcm _aesGcm;
+            private readonly byte[] _nonce;
+
+            public BclImpl(byte[] key, byte[] nonce)
+            {
+#if NET8_0_OR_GREATER
+                _aesGcm = new AesGcm(key, TagSizeInBytes);
+#else
+                _aesGcm = new AesGcm(key);
+#endif
+                _nonce = nonce;
+            }
+
+            public override void Encrypt(byte[] input, int plainTextOffset, int plainTextLength, int associatedDataOffset, int associatedDataLength, byte[] output, int cipherTextOffset)
+            {
+                var cipherTextLength = plainTextLength;
+                var plainText = new ReadOnlySpan<byte>(input, plainTextOffset, plainTextLength);
+                var cipherText = new Span<byte>(output, cipherTextOffset, cipherTextLength);
+                var tag = new Span<byte>(output, cipherTextOffset + cipherTextLength, TagSizeInBytes);
+                var associatedData = new ReadOnlySpan<byte>(input, associatedDataOffset, associatedDataLength);
+
+                _aesGcm.Encrypt(_nonce, plainText, cipherText, tag, associatedData);
+            }
+
+            public override void Decrypt(byte[] input, int cipherTextOffset, int cipherTextLength, int associatedDataOffset, int associatedDataLength, byte[] output, int plainTextOffset)
+            {
+                var plainTextLength = cipherTextLength;
+                var cipherText = new ReadOnlySpan<byte>(input, cipherTextOffset, cipherTextLength);
+                var tag = new ReadOnlySpan<byte>(input, cipherTextOffset + cipherTextLength, TagSizeInBytes);
+                var plainText = new Span<byte>(output, plainTextOffset, plainTextLength);
+                var associatedData = new ReadOnlySpan<byte>(input, associatedDataOffset, associatedDataLength);
+
+                try
+                {
+                    _aesGcm.Decrypt(_nonce, cipherText, tag, output, associatedData);
+                }
+#if NET8_0_OR_GREATER
+                catch (AuthenticationTagMismatchException ex)
+#else
+                catch (CryptographicException ex)
+#endif
+                {
+                    throw new SshConnectionException("MAC error", DisconnectReason.MacError, ex);
+                }
+            }
+
+            protected override void Dispose(bool disposing)
+            {
+                base.Dispose(disposing);
+
+                if (disposing)
+                {
+                    _aesGcm.Dispose();
+                }
+            }
+        }
+    }
+}
+#endif
diff --git a/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BouncyCastleImpl.cs b/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BouncyCastleImpl.cs
@@ -0,0 +1,48 @@
+﻿using Org.BouncyCastle.Crypto;
+using Org.BouncyCastle.Crypto.Engines;
+using Org.BouncyCastle.Crypto.Modes;
+using Org.BouncyCastle.Crypto.Parameters;
+
+using Renci.SshNet.Common;
+using Renci.SshNet.Messages.Transport;
+
+namespace Renci.SshNet.Security.Cryptography.Ciphers
+{
+    internal partial class AesGcmCipher
+    {
+        private sealed class BouncyCastleImpl : Impl
+        {
+            private readonly GcmBlockCipher _cipher;
+            private readonly AeadParameters _parameters;
+
+            public BouncyCastleImpl(byte[] key, byte[] nonce)
+            {
+                _cipher = new GcmBlockCipher(new AesEngine());
+                _parameters = new AeadParameters(new KeyParameter(key), TagSizeInBytes * 8, nonce);
+            }
+
+            public override void Encrypt(byte[] input, int plainTextOffset, int plainTextLength, int associatedDataOffset, int associatedDataLength, byte[] output, int cipherTextOffset)
+            {
+                _cipher.Init(forEncryption: true, _parameters);
+                _cipher.ProcessAadBytes(input, associatedDataOffset, associatedDataLength);
+                var cipherTextLength = _cipher.ProcessBytes(input, plainTextOffset, plainTextLength, output, cipherTextOffset);
+                _ = _cipher.DoFinal(output, cipherTextOffset + cipherTextLength);
+            }
+
+            public override void Decrypt(byte[] input, int cipherTextOffset, int cipherTextLength, int associatedDataOffset, int associatedDataLength, byte[] output, int plainTextOffset)
+            {
+                _cipher.Init(forEncryption: false, _parameters);
+                _cipher.ProcessAadBytes(input, associatedDataOffset, associatedDataLength);
+                var plainTextLength = _cipher.ProcessBytes(input, cipherTextOffset, cipherTextLength + TagSizeInBytes, output, plainTextOffset);
+                try
+                {
+                    _ = _cipher.DoFinal(output, plainTextLength);
+                }
+                catch (InvalidCipherTextException ex)
+                {
+                    throw new SshConnectionException("MAC error", DisconnectReason.MacError, ex);
+                }
+            }
+        }
+    }
+}
diff --git a/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.cs b/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.cs
@@ -1,8 +1,6 @@
-﻿#if NET6_0_OR_GREATER
-using System;
+﻿using System;
 using System.Buffers.Binary;
 using System.Diagnostics;
-using System.Security.Cryptography;
 
 using Renci.SshNet.Common;
 
@@ -12,10 +10,16 @@ namespace Renci.SshNet.Security.Cryptography.Ciphers
     /// AES GCM cipher implementation.
     /// <see href="https://datatracker.ietf.org/doc/html/rfc5647"/>.
     /// </summary>
-    internal sealed class AesGcmCipher : SymmetricCipher, IDisposable
+    internal sealed partial class AesGcmCipher : SymmetricCipher, IDisposable
     {
+        private const int PacketLengthFieldLength = 4;
+        private const int TagSizeInBytes = 16;
         private readonly byte[] _iv;
-        private readonly AesGcm _aesGcm;
+#if NET6_0_OR_GREATER
+        private readonly Impl _impl;
+#else
+        private readonly BouncyCastleImpl _impl;
+#endif
 
         /// <summary>
         /// Gets the minimun block size.
@@ -42,7 +46,7 @@ public override int TagSize
         {
             get
             {
-                return 16;
+                return TagSizeInBytes;
             }
         }
 
@@ -56,11 +60,16 @@ public AesGcmCipher(byte[] key, byte[] iv)
         {
             // SSH AES-GCM requires a 12-octet Initial IV
             _iv = iv.Take(12);
-#if NET8_0_OR_GREATER
-            _aesGcm = new AesGcm(key, TagSize);
-#else
-            _aesGcm = new AesGcm(key);
+#if NET6_0_OR_GREATER
+            if (System.Security.Cryptography.AesGcm.IsSupported)
+            {
+                _impl = new BclImpl(key, _iv);
+            }
+            else
 #endif
+            {
+                _impl = new BouncyCastleImpl(key, _iv);
+            }
         }
 
         /// <summary>
@@ -84,15 +93,17 @@ public AesGcmCipher(byte[] key, byte[] iv)
         /// </returns>
         public override byte[] Encrypt(byte[] input, int offset, int length)
         {
-            var packetLengthField = new ReadOnlySpan<byte>(input, offset, 4);
-            var plainText = new ReadOnlySpan<byte>(input, offset + 4, length - 4);
-
             var output = new byte[length + TagSize];
-            packetLengthField.CopyTo(output);
-            var cipherText = new Span<byte>(output, 4, length - 4);
-            var tag = new Span<byte>(output, length, TagSize);
+            Buffer.BlockCopy(input, offset, output, 0, PacketLengthFieldLength);
 
-            _aesGcm.Encrypt(nonce: _iv, plainText, cipherText, tag, associatedData: packetLengthField);
+            _impl.Encrypt(
+                input,
+                plainTextOffset: offset + PacketLengthFieldLength,
+                plainTextLength: length - PacketLengthFieldLength,
+                associatedDataOffset: offset,
+                associatedDataLength: PacketLengthFieldLength,
+                output,
+                cipherTextOffset: PacketLengthFieldLength);
 
             IncrementCounter();
 
@@ -122,14 +133,16 @@ public override byte[] Decrypt(byte[] input, int offset, int length)
         {
             Debug.Assert(offset == 8, "The offset must be 8");
 
-            var packetLengthField = new ReadOnlySpan<byte>(input, 4, 4);
-            var cipherText = new ReadOnlySpan<byte>(input, offset, length);
-            var tag = new ReadOnlySpan<byte>(input, offset + length, TagSize);
-
             var output = new byte[length];
-            var plainText = new Span<byte>(output);
 
-            _aesGcm.Decrypt(nonce: _iv, cipherText, tag, plainText, associatedData: packetLengthField);
+            _impl.Decrypt(
+                input,
+                cipherTextOffset: offset,
+                cipherTextLength: length,
+                associatedDataOffset: offset - PacketLengthFieldLength,
+                associatedDataLength: PacketLengthFieldLength,
+                output,
+                plainTextOffset: 0);
 
             IncrementCounter();
 
@@ -158,7 +171,7 @@ public void Dispose(bool disposing)
         {
             if (disposing)
             {
-                _aesGcm.Dispose();
+                _impl.Dispose();
             }
         }
 
@@ -169,6 +182,23 @@ public void Dispose()
             Dispose(disposing: true);
             GC.SuppressFinalize(this);
         }
+
+        private abstract class Impl : IDisposable
+        {
+            public abstract void Encrypt(byte[] input, int plainTextOffset, int plainTextLength, int associatedDataOffset, int associatedDataLength, byte[] output, int cipherTextOffset);
+
+            public abstract void Decrypt(byte[] input, int cipherTextOffset, int cipherTextLength, int associatedDataOffset, int associatedDataLength, byte[] output, int plainTextOffset);
+
+            protected virtual void Dispose(bool disposing)
+            {
+            }
+
+            public void Dispose()
+            {
+                // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
+                Dispose(disposing: true);
+                GC.SuppressFinalize(this);
+            }
+        }
     }
 }
-#endif
diff --git a/src/Renci.SshNet/Security/KeyExchangeECDH.cs b/src/Renci.SshNet/Security/KeyExchangeECDH.cs
@@ -46,12 +46,10 @@ public override void Start(Session session, KeyExchangeInitMessage message, bool
                 _impl = new BclImpl(Curve);
             }
             else
+#endif
             {
                 _impl = new BouncyCastleImpl(CurveParameter);
             }
-#else
-            _impl = new BouncyCastleImpl(CurveParameter);
-#endif
 
             _clientExchangeValue = _impl.GenerateClientECPoint();
 
diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs
@@ -1258,11 +1258,7 @@ private Message ReceiveMessage(Socket socket)
                 var plainFirstBlock = firstBlock;
 
                 // First block is not encrypted in AES GCM mode.
-                if (_serverCipher is not null
-#if NET6_0_OR_GREATER
-        and not Security.Cryptography.Ciphers.AesGcmCipher
-#endif
-                    )
+                if (_serverCipher is not null and not Security.Cryptography.Ciphers.AesGcmCipher)
                 {
                     _serverCipher.SetSequenceNumber(_inboundPacketSequence);
 
diff --git a/test/Renci.SshNet.IntegrationTests/CipherTests.cs b/test/Renci.SshNet.IntegrationTests/CipherTests.cs
@@ -64,7 +64,6 @@ public void Aes256Ctr()
             DoTest(Cipher.Aes256Ctr);
         }
 
-#if NET6_0_OR_GREATER
         [TestMethod]
         public void Aes128Gcm()
         {
@@ -76,7 +75,7 @@ public void Aes256Gcm()
         {
             DoTest(Cipher.Aes256Gcm);
         }
-#endif
+
         [TestMethod]
         public void ChaCha20Poly1305()
         {

Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ for:`
`32`	`32`	`# some coverage until a proper solution for running the .NET Framework integration tests in CI is found.`
`33`	`33`	`# Running all the tests causes problems which are not worth solving in this rare configuration.`
`34`	`34`	`# See https://github.com/sshnet/SSH.NET/pull/1462 and related links`
`35`		`- - sh: dotnet test -f net48 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_integration_test_net_48_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_integration_test_net_48_coverage.xml --filter "Name~Ecdh\|Name~Zlib" test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj`
	`35`	`+ - sh: dotnet test -f net48 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_integration_test_net_48_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_integration_test_net_48_coverage.xml --filter "Name~Ecdh\|Name~Zlib\|Name~Gcm" test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj`
`36`	`36`
`37`	`37`	`-`
`38`	`38`	`matrix:`
Original file line number	Diff line number	Diff line change
`@@ -46,12 +46,10 @@ public override void Start(Session session, KeyExchangeInitMessage message, bool`
`46`	`46`	`_impl = new BclImpl(Curve);`
`47`	`47`	`}`
`48`	`48`	`else`
	`49`	`+#endif`
`49`	`50`	`{`
`50`	`51`	`_impl = new BouncyCastleImpl(CurveParameter);`
`51`	`52`	`}`
`52`		`-#else`
`53`		`- _impl = new BouncyCastleImpl(CurveParameter);`
`54`		`-#endif`
`55`	`53`
`56`	`54`	`_clientExchangeValue = _impl.GenerateClientECPoint();`
`57`	`55`
Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,6 @@ public void Aes256Ctr()`
`64`	`64`	`DoTest(Cipher.Aes256Ctr);`
`65`	`65`	`}`
`66`	`66`
`67`		`-#if NET6_0_OR_GREATER`
`68`	`67`	`[TestMethod]`
`69`	`68`	`public void Aes128Gcm()`
`70`	`69`	`{`
`@@ -76,7 +75,7 @@ public void Aes256Gcm()`
`76`	`75`	`{`
`77`	`76`	`DoTest(Cipher.Aes256Gcm);`
`78`	`77`	`}`
`79`		`-#endif`
	`78`	`+`
`80`	`79`	`[TestMethod]`
`81`	`80`	`public void ChaCha20Poly1305()`
`82`	`81`	`{`