Is it safe to reuse an AES object when encrypting strings?

Question

I'm having some performance problems in an app that encrypts lots of strings. Most of the CPU use happens when I call the private method getAes() from a public method called Encrypt():

public static class CryptKeeper
{
    const int HASH_SIZE = 32; //SHA256

    /// <summary>
    /// Encrypts a string message. Includes integrity checking.
    /// </summary>
    public static string Encrypt(string messageToEncrypt, string sharedSecret, string salt)
    {
        // Prepare message with hash
        var messageBytes = Encoding.UTF8.GetBytes(messageToEncrypt);
        var hashedMessageBytes = new byte[HASH_SIZE + messageBytes.Length];
        var hash = Utilities.GenerateSha256Hash(messageBytes, 0, messageBytes.Length);
        Buffer.BlockCopy(hash, 0, hashedMessageBytes, 0, HASH_SIZE);
        Buffer.BlockCopy(messageBytes, 0, hashedMessageBytes, HASH_SIZE, messageBytes.Length);

        // Encrypt message
        using (var aes = getAes(sharedSecret, Encoding.UTF8.GetBytes(salt)))
        {
            aes.GenerateIV();
            using (var encryptor = aes.CreateEncryptor())
            {
                var encryptedBytes = encryptor.TransformFinalBlock(hashedMessageBytes, 0, hashedMessageBytes.Length);
                // Add the initialization vector
                var result = new byte[aes.IV.Length + encryptedBytes.Length];
                Buffer.BlockCopy(aes.IV, 0, result, 0, aes.IV.Length);
                Buffer.BlockCopy(encryptedBytes, 0, result, aes.IV.Length, encryptedBytes.Length);
                return Convert.ToBase64String(result);
            }
        }
    }

    public static string Decrypt(string encryptedMessage, string sharedSecret, string salt)
    {
        if (encryptedMessage == null) return null;

        using (var aes = getAes(sharedSecret, Encoding.UTF8.GetBytes(salt)))
        {
            var iv = new byte[aes.IV.Length];
            Buffer.BlockCopy(Convert.FromBase64String(encryptedMessage), 0, iv, 0, iv.Length);
            aes.IV = iv;

            using (var decryptor = aes.CreateDecryptor())
            {
                var decryptedBytes = decryptor.TransformFinalBlock(Convert.FromBase64String(encryptedMessage), iv.Length, Convert.FromBase64String(encryptedMessage).Length - iv.Length);

                // Check hash
                var hash = Utilities.GenerateSha256Hash(decryptedBytes, HASH_SIZE, decryptedBytes.Length - HASH_SIZE);
                var existingHash = new byte[HASH_SIZE];
                Buffer.BlockCopy(decryptedBytes, 0, existingHash, 0, HASH_SIZE);
                if (!existingHash.compareBytesTo(hash))
                {
                    throw new CryptographicException("Message hash invalid.");
                }

                // Hash is valid, we're done
                var res = new byte[decryptedBytes.Length - HASH_SIZE];
                Buffer.BlockCopy(decryptedBytes, HASH_SIZE, res, 0, res.Length);
                return Encoding.UTF8.GetString(res);
            }
        }
    }

    private static Aes getAes(string sharedSecret, byte[] salt)
    {
        var aes = Aes.Create();
        aes.Mode = CipherMode.CBC;
        aes.Key = new Rfc2898DeriveBytes(sharedSecret, salt, 129).GetBytes(aes.KeySize / 8);
        return aes;
    }
}

I tried to improve performance by caching the AES object, but I am getting into unfamiliar territory:

public static class CryptKeeper
{
    const int HASH_SIZE = 32; //SHA256

    private static Aes aes;

    /// <summary>
    /// Encrypts a string message. Includes integrity checking.
    /// </summary>
    public static string Encrypt(string messageToEncrypt, string sharedSecret, string salt)
    {
        // unchanged
    }

    public static string Decrypt(string encryptedMessage, string sharedSecret, string salt)
    {
        // unchanged
    }

    private static Aes getAes(string sharedSecret, byte[] salt)
    {
        if (aes != null) return aes;

        var aesNew = Aes.Create();
        aesNew.Mode = CipherMode.CBC;
        aesNew.Key = new Rfc2898DeriveBytes(sharedSecret, salt, 129).GetBytes(aesNew.KeySize / 8);
        return aes = aesNew;
    }
}

I get this error:

Safe handle has been closed at System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean& success) at System.StubHelpers.StubHelpers.SafeHandleAddRef(SafeHandle pHandle, Boolean& success) at System.Security.Cryptography.CapiNative.UnsafeNativeMethods.CryptGenRandom(SafeCspHandle hProv, Int32 dwLen, Byte[] pbBuffer) at System.Security.Cryptography.AesCryptoServiceProvider.GenerateIV() at Obr.Lib.CryptKeeper.Encrypt(String messageToEncrypt, String sharedSecret, String salt) in ...CryptKeeper.cs:line 28 at Obr.Lib.HtmlRenderer.renderLawCitation(RenderContext renderContext, XElement xElement) in ...HtmlRenderer.cs:line 1472

I understand that the using() statement in Encrypt() is going to dispose of AES and that is causing it to break. I don't want to troubleshoot further unless I know it's safe to reuse. If it is safe to reuse, what is the best way to do this?

Update: I have solved the performance problem by keeping the AES object around longer. I've removed the static keywords, and made the class disposable. Here's how it looks now:

public class CryptKeeper : IDisposable
{
    const int HASH_SIZE = 32; //SHA256
    private readonly Aes aes;

    public CryptKeeper(string sharedSecret, string salt)
    {
        aes = Aes.Create();
        aes.Mode = CipherMode.CBC;
        aes.Key = new Rfc2898DeriveBytes(sharedSecret, Encoding.UTF8.GetBytes(salt), 129).GetBytes(aes.KeySize / 8);
    }

    /// <summary>
    /// Encrypts a string message. Includes integrity checking.
    /// </summary>
    public string Encrypt(string messageToEncrypt)
    {
        // Prepare message with hash
        var messageBytes = Encoding.UTF8.GetBytes(messageToEncrypt);
        var hashedMessageBytes = new byte[HASH_SIZE + messageBytes.Length];
        var hash = Utilities.GenerateSha256Hash(messageBytes, 0, messageBytes.Length);
        Buffer.BlockCopy(hash, 0, hashedMessageBytes, 0, HASH_SIZE);
        Buffer.BlockCopy(messageBytes, 0, hashedMessageBytes, HASH_SIZE, messageBytes.Length);

        // Encrypt message
        aes.GenerateIV();
        using (var encryptor = aes.CreateEncryptor())
        {
            var encryptedBytes = encryptor.TransformFinalBlock(hashedMessageBytes, 0, hashedMessageBytes.Length);
            // Add the initialization vector
            var result = new byte[aes.IV.Length + encryptedBytes.Length];
            Buffer.BlockCopy(aes.IV, 0, result, 0, aes.IV.Length);
            Buffer.BlockCopy(encryptedBytes, 0, result, aes.IV.Length, encryptedBytes.Length);
            return Convert.ToBase64String(result);
        }
    }

    public string Decrypt(string encryptedMessage)
    {
        if (encryptedMessage == null) return null;

        var iv = new byte[aes.IV.Length];
        Buffer.BlockCopy(Convert.FromBase64String(encryptedMessage), 0, iv, 0, iv.Length);
        aes.IV = iv;

        using (var decryptor = aes.CreateDecryptor())
        {
            var decryptedBytes = decryptor.TransformFinalBlock(Convert.FromBase64String(encryptedMessage), iv.Length, Convert.FromBase64String(encryptedMessage).Length - iv.Length);

            // Check hash
            var hash = Utilities.GenerateSha256Hash(decryptedBytes, HASH_SIZE, decryptedBytes.Length - HASH_SIZE);
            var existingHash = new byte[HASH_SIZE];
            Buffer.BlockCopy(decryptedBytes, 0, existingHash, 0, HASH_SIZE);
            if (!existingHash.compareBytesTo(hash))
            {
                throw new CryptographicException("Message hash invalid.");
            }

            // Hash is valid, we're done
            var res = new byte[decryptedBytes.Length - HASH_SIZE];
            Buffer.BlockCopy(decryptedBytes, HASH_SIZE, res, 0, res.Length);
            return Encoding.UTF8.GetString(res);
        }
    }

    bool disposed;

    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            if (disposing)
            {
                aes.Dispose();
            }
        }
        disposed = true;
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
}

I invoke it like this:

using (cryptKeeper = new CryptKeeper(Repository.AppSettings["SharedSecret"], Repository.AppSettings["Salt"]))
{
    renderingReport.Rendering = renderSegmentNav(currentUser.UserOwnsProduct(productId), book, renderingReport, currentSegment);
}

This has improved performance tremendously. A previous call to an MVC controller that needed to build many encrypted links in its result took 2.7 seconds total. With the new code where AES is reused, it takes 0.3 seconds total.

I can confirm that the code works and is much, much faster. I just want to confirm that reuse of AES in this manner is not a BAD IDEA for security reasons. According to a little google searching, the fact that I am calling GenerateIV() each time is good, and I can't find anything saying I should not re-use the AES for as long as I like.

Answer 1

In general you can reuse objects that implement cryptographic algorithms in both Java and C#. You should however be sure that you always leave the encryptors and decryptors in the correct state. You should not use these classes for multi-threading purposes unless specifically specified.

Note that the reason that you are experiencing a slowdown is because of the PBKDF2 function within Rfc2898DeriveBytes. This method is deliberately slow. You may reuse the key that you get from Rfc2898DeriveBytes but you should be sure that you do not reuse any IV, the IV should be random. It does of course not make sense to call Rfc2898DeriveBytes derive bytes multiple times.

Finally, it could be somewhat beneficial to cache objects holding an AES key locally. First of all, you don't want any additional key objects around if you don't have to, and second, AES first calculates the subkeys from the given key which takes a small amount of time (although nowhere near the time it takes to execute Rfc2898DeriveBytes).

Then again, don't do this if it unnecessarily complicates your design. The advantages are not big enough for that.