Does this AES cryptography code have a double-dispose problem?

Question

Another year, another change to the way the good folks at Microsoft want us to write encryption/decryption code. Now (Feb. 2022, .Net 6) we are supposed to use the new Aes class instead of the deprecated RijndaelManaged class and its cousins.

So, here is my symmetrical encryption/decryption code:

/// <summary> Encrypts a byte array using the AES encryption algorithm. </summary>
/// <param name="clearData"> The data to encrypt. </param>
/// <param name="key"> The encryption key. </param>
/// <param name="iv"> The initialization vector. </param>
/// <returns> The encrypted data. </returns>
public static byte[] Encrypt(byte[] clearData, byte[] key, byte[] iv)
{
    Contract.Requires(clearData != null);
    Contract.Requires(key != null);
    Contract.Requires(iv != null);

    using (var aes = Aes.Create()) {
        aes.Key = key;
        aes.IV = iv;
        using (var encryptor = aes.CreateEncryptor(aes.Key, aes.IV)) {
            return _PerformCryptography(clearData, encryptor);
        }
    }
}

/// <summary> Decrypts a byte array using the AES encryption algorithm. </summary>
/// <param name="encryptedData"> The encrypted data. </param>
/// <param name="key"> The encryption key. </param>
/// <param name="iv"> The initialization vector. </param>
/// <returns> The decrypted data. </returns>
public static byte[] Decrypt(byte[] encryptedData, byte[] key, byte[] iv)
{
    Contract.Requires(encryptedData != null);
    Contract.Requires(key != null);
    Contract.Requires(iv != null);

    using (var aes = Aes.Create()) {
        aes.Key = key;
        aes.IV = iv;
        using (var decryptor = aes.CreateDecryptor(aes.Key, aes.IV)) {
            return _PerformCryptography(encryptedData, decryptor);
        }
    }
}

/// <summary> Performs symmetrical encryption or decryption using a specified transformation algorithm. </summary>
/// <param name="data"> The data to encrypt or decrypt. </param>
/// <param name="cryptoTransform"> The crypto algorithm. </param>
/// <returns> The encrypted or decrypted data. </returns>
private static byte[] _PerformCryptography(byte[] data, ICryptoTransform cryptoTransform)
{
    using var ms = new MemoryStream();
    using var cs = new CryptoStream(ms, cryptoTransform, CryptoStreamMode.Write);
    cs.Write(data, 0, data.Length);
    cs.FlushFinalBlock();

    return ms.ToArray();
}

My specific question has to do with the using statements in _PerformCryptography. Rather than relying on using statements to dispose objects, my previous implementation used a try/finally construct to avoid disposing the MemoryStream twice:

            // Get a SymmetricAlgorithm object
            using var algorithm = new RijndaelManaged();

            // Set the key and the initialization vector
            algorithm.Key = key;
            algorithm.GenerateIV();

            // Encrypt the information to a MemoryStream
            MemoryStream ms = null;
            CryptoStream cs = null;
            try {
                // See comment below re: "multiple dispose" issue
                ms = new MemoryStream();

                // Encrypt the data
                cs = new CryptoStream(ms, algorithm.CreateEncryptor(), CryptoStreamMode.Write);
                cs.Write(clearBytes, 0, clearBytes.Length);
                cs.FlushFinalBlock();

                // Return the encrypted stream of data as a byte array
                return ms.ToArray();
            }

            finally {
                if (cs != null) {
                    // Dispose the CryptoStream, which disposes the MemoryStream that it contains
                    ms = null;
                    cs.Dispose();
                }

                if (ms != null) ms.Dispose();
            }

Is this still required, or has something changed so the "multiple dispose" is no longer an issue? (Or maybe it was never really an issue to begin with?)

Not sure where you got the idea you needed to be so careful. See [this answer](https://stackoverflow.com/questions/5306860/should-idisposable-dispose-be-made-safe-to-call-multiple-times#:~:text=Yes%2C%20your%20implementations%20of%20IDisposable,calls%20can%20just%20return%20immediately.) for how it's documented and why calling it twice ought to be harmless. — Kirk Woll, Feb 16 '22 at 20:42
Yeah, I know it *should* be safe to dispose more than once. I either had a problem or (maybe) I stumbled across an SO post the said this was a problem for some reason. I'm pretty sure I wouldn't have gone to all that work for no good reason. — Bob.at.Indigo.Health, Feb 16 '22 at 21:16
If you’re using .NET 6, you can drop all the CryptoStream code and just use `aes.EncryptCbc(plaintext, iv)` after setting the key. — bartonjs, Mar 20 '22 at 19:46

score 2 · Accepted Answer · answered Feb 16 '22 at 21:09

A couple of things...

It's easy enough to test the code you're asking about and see what happens.
You can F12 from Visual Studio on nearly any .NET class and go directly to the source code, which would let you investigate how (for instance) the CryptoStream.Dispose() method is implemented.
A quick check of available CryptoStream constructors shows that there's an overload taking a bool leaveOpen parameter, which will leave the stream open when the CryptoStream is disposed. But you don't need that parameter, because...
If you go to the source for MemoryStream.Dispose(), you'll find that it doesn't do much other than set some internal field values - and explicitly leaves the _buffer field intact. So even if you do call it twice, it's not going to misbehave - and you can still safely call .ToArray() on the MemoryStream even after it's been disposed.

Does this AES cryptography code have a double-dispose problem?

1 Answers1