Thread Safety of .NET Encryption Classes?

Question

I have a high-level goal of creating a static utility class that encapsulates the encryption for my .NET application. Inside I'd like to minimize the object creations that aren't necessary.

My question is: what is the thread-safety of the classes which implement symmetric encryption within the .NET Framework? Specifically System.Security.Cryptography.RijndaelManaged and the ICryptoTransform types it generates.

For instance, in my class constructor can I simply do something along the following lines?

static MyUtility()
{
    using (RijndaelManaged rm = new RijndaelManaged())
    {
        MyUtility.EncryptorTransform = rm.CreateEncryptor(MyUtility.MyKey, MyUtility.MyIV);
        MyUtility.DecryptorTransform = rm.CreateDecryptor(MyUtility.MyKey, MyUtility.MyIV);
    }
}

Side-stepping the issue of is it secure to have Key and IV exist within this class, this example block brings up a number of other questions:

1. Can I continually reuse the EncryptorTransform and DecryptorTransform over and over? The *.CanReuseTransform and *.CanTransformMultipleBlocks properties imply "yes", but are there any caveats I should be aware of?

2. Since RijndaelManaged implements IDisposable my inclination is to put it within a using block especially since it probably ties into external OS-level libs. Are there any caveats with this since I'm keeping the ICryptoTransform objects around?

3. Potentially the most important question, in a highly multithreaded environment, will I run into issues with sharing the ICryptoTransform objects between threads?

4. If the answer to #3 is that it isn't thread-safe, will I experience serious performance degradation from locking while I use the ICryptoTransform objects? (Depends on load I suppose.)

5. Would it be more performant to simply instantiate new RijndaelManaged each time? Or store one RijndaelManaged and generate new RijndaelManaged().CreateEncryptor(...) each time?

I am hoping that someone out there knows how these work under the hood or are experienced with issues from similar implementations. I've found that a lot of these kinds of performance and thread-related issues typically do not manifest themselves until there is a sizable amount of load.

Thanks!

Branch

Answer 1

1) Yes.

2) One you dispose of it, you cannot use it. Up until then, you can share/use it (but see below)

3-4) From MSDN:

"Any public static (Shared in Visual Basic) members of this type are thread safe. Any instance members are not guaranteed to be thread safe. "

If you want to keep this around, and share it between threads, you'll need to implement locking and treat it as a locked resource. Otherwise, I'd recommend just making separate versions as needed, and disposing of them when you're done.

5) I would recommend creating these as needed, and then trying to optimize it if later you find you have a performance problem. Don't worry about the performance implications of creating a new version until you see that's its a problem after profiling.

Flogging

Good sound advice. I guess I made the assumption that creating and destroying some of these objects came at a cost due to the underlying dependencies upon the navtive Platform SDK classes. If no one thinks this is a warranted concern, then it sounds like the synchronization mechanisms might in the end be the bigger bottleneck. – Branch

True that you should not spend time optimizing prematurely, but just want to drop here that in our case we found that performing ~500 small encryptions (all with the same password) took almost 10 seconds which was a disaster. Nearly all the time was spent creating encryptors. Re-using ICryptoTransforms was the obvious solution. – Greg

My 2cents, I found that not reusing the ICryptoTransform caused very bad performance of our application. – Chiliasm

Answer 2

One could solve the concurrency problem simply with a cache based on a concurrent stack:

static ConcurrentStack<ICryptoTransform> decryptors = new ConcurrentStack<ICryptoTransform>();

void Encrypt()
{
   // Pop decryptor from cache...
   ICryptoTransform decryptor;
   if (!decryptors.TryPop(out decryptor))
   {
       // ... or create a new one since cache is depleted
       AesManaged aes = new AesManaged();
       aes.Key = key;
       aes.IV = iv;
       decryptor = aes.CreateDecryptor(aes.Key, aes.IV);
    }

    try
    {
       //// use decryptor
    }
    finally
    {
       decryptors.Push(decryptor);
    }
 }

Multiphase

hello, I know this is a very old post but this did not fix the problem for me. I still have random failed results from the decrypting, but if I CreateDecryptor() for each call it works without fail. Any tips? In my case i'm using the decryptor in a CryptoStream(). thanks! – Sunshine

found the reason but not the real fix. let me know if you can help. I posted my findings as answer below. – Sunshine

Answer 3

1. Absolutely NOT, I had troubles. I had been using it for two years when all of a sudden some of my most important code started throwing errors when decrypting. Result of Microsoft patches? McAfee? dot net framework updates? (framework 4.7.2)

When I finally figured it out I moved the CreateEncryptor and CreateDecryptor into a using inside the actual call to encrypt and decrypt. Problem fixed.

Rattle

I know this is old but I was having the same problem, I even try to implement the solution above from @Multiphase with the ConcurrentStack but it did not fix it, for some reason I kept having random wrong results from decryption. The only solution that work for me was to CreateDecryptor() on each call. No idea why. I try framework 4.7.2 and 4.8 :s – Sunshine

found the reason but not the real fix. let me know if you can help. I posted my findings as answer below. – Sunshine

Answer 4

I have been fighting with this problem that I tough was thread safety related but I just dicovered it was different. This "solution" is not really related with the OP questions but this post was the closest I found when searching for solutions for my problem, thus I will post here and maybe it will help someone else.

When using CryptoStream like this:

 using (MemoryStream memoryStream = new MemoryStream())
 using (CryptoStream cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Write))
 {
    cryptoStream.Write(cipherData, 0, cipherData.Length);
    cryptoStream.FlushFinalBlock();
    result = memoryStream.ToArray();
 }

If the cipherData is incorrect (the content is plain text and not encrypted for example) it may cause an expection at the FlushFinalBlock(), for example:

"The input data is not a complete block." (System.Security.Cryptography.CryptographicException)

Because of this something will stay unfinished and will cause the next usage of this function (using the same decryptor) to return a result with leftovers of the previous usage, thus causing unexpected errors handling the result later.

As @Doug Hill sugested the creation of a new CreateEncryptor()/CreateDecryptor() at every call will fix it since that is the link between each call of the function but at a cost of performance.

With the help of the base code from @Andreas I created this solution that will dispose those ICryptoTransform objects only when the exception is thrown because I have no idea how to clear/reset it to reuse again. Its not perfect but should improve performance.

private Aes _aes;
private static ConcurrentStack<ICryptoTransform> decryptors = new ConcurrentStack<ICryptoTransform>();

public void Initialize()
{
    _aes = Aes.Create();
    // ...
}

public byte[] Decode(byte[] cipherData)
{
    var decryptor = DecryptorPop();
    
    try
    {
        byte[] result = null;
        using (MemoryStream memoryStream = new MemoryStream())
        using (CryptoStream cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Write))
        {
            cryptoStream.Write(cipherData, 0, cipherData.Length);
            cryptoStream.FlushFinalBlock();
            result = memoryStream.ToArray();
        }
        return result;
    }
    catch (Exception ex)
    {
        // since I do not know how to fix the decryptor I will just dispose it
        decryptor.Dispose();
        decryptor = null;

        // throw the same exception again if you want to catch it outside
        System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(ex).Throw();
    }
    finally
    {
        DecryptorPush(decryptor);
    }

    return null;
}

private ICryptoTransform DecryptorPop()
{
    ICryptoTransform decryptor;
    if (!decryptors.TryPop(out decryptor))
    {
        decryptor = _aes.CreateDecryptor();
    }

    return decryptor;
}

private void DecryptorPush(ICryptoTransform decryptor)
{
    if(decryptor == null) return;

    decryptors.Push(decryptor);
}

Sunshine

Answer 5

Reusing the ICryptoTransform instance returned from a call to AesManaged.CreateDecryptor implies that you are using the same key and initialization vector to decrypt multiple messages, that were previously encrypted with the same key and initialization vector.

This is a very, very, bad idea.

IV's should never be re-used, and keys should only be re-used indirectly, by having a master key protecting the actual session or message key, which should be unique for every message. So the only re-used key is the master key, which is only used to encrypt other keys, never data. Nor should there be any relationship between the keys or the IV, i.e. don't derive the IV from the key or the session key from the master key etc.

Thus, to answer the original and subsequent question which boils to "How to safely re-use 'decryptor' objects", the answer is simply: don't because then you're re-using both the key and the IV, and neither should be re-used.

Rolling your own cryptographic schemes is always fraught with danger, and in general I'd recommend not to do it at all. Just look at the number of times major companies and organizations (Microsoft's Office encryption, IEEE WiFi encryption just to mention two) got it wrong, multiple times even.

It's not a question of thread safety or not etc., it's a question of why you're encrypting in the first place. If you're not doing it correctly, it's better not to do it all, since then at least it's clear that the messages are not protected. Doing it incorrectly amounts to no more than obfuscation.

Galvez