From e53a3d9a07b269229c677be584fa8c03dcfb9484 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Fri, 7 Oct 2022 14:01:23 -0400 Subject: [PATCH] Test and fix using AEAD ciphers after disposal --- .../Security/Cryptography/AesCcm.OpenSsl.cs | 17 +++++++++++++++-- .../tests/AesCcmTests.cs | 16 ++++++++++++++++ .../tests/AesGcmTests.cs | 16 ++++++++++++++++ .../tests/ChaCha20Poly1305Tests.cs | 16 ++++++++++++++++ 4 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesCcm.OpenSsl.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesCcm.OpenSsl.cs index 0aedd34fe2d1..8f96682ca325 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesCcm.OpenSsl.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesCcm.OpenSsl.cs @@ -9,7 +9,7 @@ namespace System.Security.Cryptography { public sealed partial class AesCcm { - private byte[] _key; + private byte[]? _key; public static bool IsSupported { get; } = Interop.OpenSslNoInit.OpenSslIsAvailable; @@ -18,7 +18,9 @@ private void ImportKey(ReadOnlySpan key) { // OpenSSL does not allow setting nonce length after setting the key // we need to store it as bytes instead - _key = key.ToArray(); + // Pin the array on the POH so that the GC doesn't move it around to allow zeroing to be more effective. + _key = GC.AllocateArray(key.Length, pinned: true); + key.CopyTo(_key); } private void EncryptCore( @@ -28,6 +30,8 @@ private void ImportKey(ReadOnlySpan key) Span tag, ReadOnlySpan associatedData = default) { + CheckDisposed(); + using (SafeEvpCipherCtxHandle ctx = Interop.Crypto.EvpCipherCreatePartial(GetCipher(_key.Length * 8))) { Interop.Crypto.CheckValidOpenSslHandle(ctx); @@ -82,6 +86,8 @@ private void ImportKey(ReadOnlySpan key) Span plaintext, ReadOnlySpan associatedData) { + CheckDisposed(); + using (SafeEvpCipherCtxHandle ctx = Interop.Crypto.EvpCipherCreatePartial(GetCipher(_key.Length * 8))) { Interop.Crypto.CheckValidOpenSslHandle(ctx); @@ -134,6 +140,13 @@ private static IntPtr GetCipher(int keySizeInBits) public void Dispose() { CryptographicOperations.ZeroMemory(_key); + _key = null; + } + + [MemberNotNull(nameof(_key))] + private void CheckDisposed() + { + ObjectDisposedException.ThrowIf(_key is null, this); } } } diff --git a/src/libraries/System.Security.Cryptography/tests/AesCcmTests.cs b/src/libraries/System.Security.Cryptography/tests/AesCcmTests.cs index 8b68d7f1d61e..d47e0dea5ad3 100644 --- a/src/libraries/System.Security.Cryptography/tests/AesCcmTests.cs +++ b/src/libraries/System.Security.Cryptography/tests/AesCcmTests.cs @@ -376,6 +376,22 @@ public static void AesCcmNistTestsTamperCiphertext(AEADTest testCase) } } + [Fact] + public static void UseAfterDispose() + { + byte[] key = "eda32f751456e33195f1f499cf2dc7c97ea127b6d488f211ccc5126fbb24afa6".HexToByteArray(); + byte[] nonce = "a544218dadd3c1".HexToByteArray(); + byte[] plaintext = Array.Empty(); + byte[] ciphertext = Array.Empty(); + byte[] tag = "469c90bb".HexToByteArray(); + + AesCcm aesCcm = new AesCcm(key); + aesCcm.Dispose(); + + Assert.Throws(() => aesCcm.Encrypt(nonce, plaintext, ciphertext, new byte[tag.Length])); + Assert.Throws(() => aesCcm.Decrypt(nonce, ciphertext, tag, plaintext)); + } + public static IEnumerable GetValidNonceSizes() { return GetValidSizes(AesCcm.NonceByteSizes); diff --git a/src/libraries/System.Security.Cryptography/tests/AesGcmTests.cs b/src/libraries/System.Security.Cryptography/tests/AesGcmTests.cs index 6ef5391e40e5..0da0f45b31a2 100644 --- a/src/libraries/System.Security.Cryptography/tests/AesGcmTests.cs +++ b/src/libraries/System.Security.Cryptography/tests/AesGcmTests.cs @@ -383,6 +383,22 @@ public static void AesGcmNistTestsTamperCiphertext(AEADTest testCase) } } + [Fact] + public static void UseAfterDispose() + { + byte[] key = new byte[16]; + byte[] nonce = new byte[12]; + byte[] plaintext = Array.Empty(); + byte[] ciphertext = Array.Empty(); + byte[] tag = "58e2fccefa7e3061367f1d57a4e7455a".HexToByteArray(); + + AesGcm aesGcm = new AesGcm(key); + aesGcm.Dispose(); + + Assert.Throws(() => aesGcm.Encrypt(nonce, plaintext, ciphertext, new byte[tag.Length])); + Assert.Throws(() => aesGcm.Decrypt(nonce, ciphertext, tag, plaintext)); + } + public static IEnumerable GetValidNonceSizes() { return GetValidSizes(AesGcm.NonceByteSizes); diff --git a/src/libraries/System.Security.Cryptography/tests/ChaCha20Poly1305Tests.cs b/src/libraries/System.Security.Cryptography/tests/ChaCha20Poly1305Tests.cs index 40f17690cdff..ced6edcf3509 100644 --- a/src/libraries/System.Security.Cryptography/tests/ChaCha20Poly1305Tests.cs +++ b/src/libraries/System.Security.Cryptography/tests/ChaCha20Poly1305Tests.cs @@ -316,6 +316,22 @@ public static void Rfc8439TestsTamperTag(AEADTest testCase) } } + [Fact] + public static void UseAfterDispose() + { + byte[] key = new byte[32]; + byte[] nonce = new byte[12]; + byte[] plaintext = Array.Empty(); + byte[] ciphertext = Array.Empty(); + byte[] tag = "4eb972c9a8fb3a1b382bb4d36f5ffad1".HexToByteArray(); + + ChaCha20Poly1305 chaChaPoly = new ChaCha20Poly1305(key); + chaChaPoly.Dispose(); + + Assert.Throws(() => chaChaPoly.Encrypt(nonce, plaintext, ciphertext, new byte[tag.Length])); + Assert.Throws(() => chaChaPoly.Decrypt(nonce, ciphertext, tag, plaintext)); + } + public static IEnumerable GetInvalidNonceSizes() { yield return new object[] { 0 };