From ebb31bb16e1e95c446ae9652121c4f1b1b3761b9 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Mon, 19 Aug 2024 20:13:10 +0800 Subject: [PATCH] AesGcmCipher uses BouncyCastle as a fallback if BCL does not support. (#1450) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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 https://github.com/sshnet/SSH.NET/pull/1371#discussion_r1704293356 * 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 --- .editorconfig | 4 + README.md | 4 +- appveyor.yml | 2 +- src/Renci.SshNet/ConnectionInfo.cs | 29 +++---- .../Ciphers/AesGcmCipher.BclImpl.cs | 72 +++++++++++++++++ .../Ciphers/AesGcmCipher.BouncyCastleImpl.cs | 48 +++++++++++ .../Cryptography/Ciphers/AesGcmCipher.cs | 80 +++++++++++++------ src/Renci.SshNet/Security/KeyExchangeECDH.cs | 4 +- src/Renci.SshNet/Session.cs | 6 +- .../CipherTests.cs | 3 +- 10 files changed, 198 insertions(+), 54 deletions(-) create mode 100644 src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BclImpl.cs create mode 100644 src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BouncyCastleImpl.cs diff --git a/.editorconfig b/.editorconfig index 05a2600d0..20029dbf9 100644 --- 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 index b824f01f4..8c4d8ece5 100644 --- 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@openssh.com (.NET 6 and higher) -* aes256-gcm@openssh.com (.NET 6 and higher) +* aes128-gcm@openssh.com +* aes256-gcm@openssh.com * chacha20-poly1305@openssh.com * aes128-cbc * aes192-cbc diff --git a/appveyor.yml b/appveyor.yml index 636414f7d..ca7144ecb 100644 --- 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 index 1c923286c..51279f4a4 100644 --- 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(); - 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 + { + { "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 { diff --git a/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BclImpl.cs b/src/Renci.SshNet/Security/Cryptography/Ciphers/AesGcmCipher.BclImpl.cs new file mode 100644 index 000000000..4ed11cf51 --- /dev/null +++ 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(input, plainTextOffset, plainTextLength); + var cipherText = new Span(output, cipherTextOffset, cipherTextLength); + var tag = new Span(output, cipherTextOffset + cipherTextLength, TagSizeInBytes); + var associatedData = new ReadOnlySpan(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(input, cipherTextOffset, cipherTextLength); + var tag = new ReadOnlySpan(input, cipherTextOffset + cipherTextLength, TagSizeInBytes); + var plainText = new Span(output, plainTextOffset, plainTextLength); + var associatedData = new ReadOnlySpan(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 new file mode 100644 index 000000000..4c271211f --- /dev/null +++ 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 index cd673d04a..fb0926c5c 100644 --- 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. /// . /// - 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 /// /// 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); + } } /// @@ -84,15 +93,17 @@ public AesGcmCipher(byte[] key, byte[] iv) /// public override byte[] Encrypt(byte[] input, int offset, int length) { - var packetLengthField = new ReadOnlySpan(input, offset, 4); - var plainText = new ReadOnlySpan(input, offset + 4, length - 4); - var output = new byte[length + TagSize]; - packetLengthField.CopyTo(output); - var cipherText = new Span(output, 4, length - 4); - var tag = new Span(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(input, 4, 4); - var cipherText = new ReadOnlySpan(input, offset, length); - var tag = new ReadOnlySpan(input, offset + length, TagSize); - var output = new byte[length]; - var plainText = new Span(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 index e5ec28c5d..b5e887f60 100644 --- 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 index 7ff95e27b..0dbd03738 100644 --- 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 index c25c0da2e..1fb03f88d 100644 --- 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() {