Test and fix using AEAD ciphers after disposal

jkotas · Oct 7, 2022 · e53a3d9 · e53a3d9
1 parent 3c99def
commit e53a3d9
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 2 deletions.
diff --git a/...libraries/System.Security.Cryptography/src/System/Security/Cryptography/AesCcm.OpenSsl.cs b/...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<byte> 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<byte>(key.Length, pinned: true);
+            key.CopyTo(_key);
         }
 
         private void EncryptCore(
@@ -28,6 +30,8 @@ private void EncryptCore(
             Span<byte> tag,
             ReadOnlySpan<byte> associatedData = default)
         {
+            CheckDisposed();
+
             using (SafeEvpCipherCtxHandle ctx = Interop.Crypto.EvpCipherCreatePartial(GetCipher(_key.Length * 8)))
             {
                 Interop.Crypto.CheckValidOpenSslHandle(ctx);
@@ -82,6 +86,8 @@ private void DecryptCore(
             Span<byte> plaintext,
             ReadOnlySpan<byte> 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
@@ -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>();
+            byte[] ciphertext = Array.Empty<byte>();
+            byte[] tag = "469c90bb".HexToByteArray();
+
+            AesCcm aesCcm = new AesCcm(key);
+            aesCcm.Dispose();
+
+            Assert.Throws<ObjectDisposedException>(() => aesCcm.Encrypt(nonce, plaintext, ciphertext, new byte[tag.Length]));
+            Assert.Throws<ObjectDisposedException>(() => aesCcm.Decrypt(nonce, ciphertext, tag, plaintext));
+        }
+
         public static IEnumerable<object[]> 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
@@ -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>();
+            byte[] ciphertext = Array.Empty<byte>();
+            byte[] tag = "58e2fccefa7e3061367f1d57a4e7455a".HexToByteArray();
+
+            AesGcm aesGcm = new AesGcm(key);
+            aesGcm.Dispose();
+
+            Assert.Throws<ObjectDisposedException>(() => aesGcm.Encrypt(nonce, plaintext, ciphertext, new byte[tag.Length]));
+            Assert.Throws<ObjectDisposedException>(() => aesGcm.Decrypt(nonce, ciphertext, tag, plaintext));
+        }
+
         public static IEnumerable<object[]> 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
@@ -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>();
+            byte[] ciphertext = Array.Empty<byte>();
+            byte[] tag = "4eb972c9a8fb3a1b382bb4d36f5ffad1".HexToByteArray();
+
+            ChaCha20Poly1305 chaChaPoly = new ChaCha20Poly1305(key);
+            chaChaPoly.Dispose();
+
+            Assert.Throws<ObjectDisposedException>(() => chaChaPoly.Encrypt(nonce, plaintext, ciphertext, new byte[tag.Length]));
+            Assert.Throws<ObjectDisposedException>(() => chaChaPoly.Decrypt(nonce, ciphertext, tag, plaintext));
+        }
+
         public static IEnumerable<object[]> GetInvalidNonceSizes()
         {
             yield return new object[] { 0 };