Search code examples
cryptographytask-parallel-librarystatic-methodsrijndaelmanaged

Issue with static encryption/decryption in a multi-threaded/TPL application

I have a static encryption/decryption class with static methods in one of my libraries. The code is a bit large so I posted it here: http://pastebin.com/zRZtNmjU

I haven't had any issues with this code until it started being used within the execution stack of some code executing in a Parallel.ForEach() call. I tried adding some use of lock on some of the MemoryStream and CryptoStream variables but that hasn't seemed to help. I did this because I am fairly certain this issue is being caused by accessing this static class & methods from multiple threads.

The exception I keep getting is: 'Padding is invalid and cannot be removed.'

This exception occurs when execution reaches decryptedTextStream.FlushFinalBlock(); within the last method in the code called Decrypt (line 250).

My question is - I can't figure out what's wrong with the code and am wondering if the only issue is that the class and methods are static? I've been using this code without a single issue for months and have only recently run into issues when starting to use it within some other TPL code. Should I just refactor this class into being instance-based or will that not solve the exception I'm getting?

Solution

  • your code simply isn't threadsafe ... what happens? currently you have an iCryptoTransform that is created once (static constructor) so every encryption/decryption call will use that object ...

    there are 2 problems with that:

    1. what if 2 or more threads try to encrypt/decrypt in parallel? your system has an internal state that is shared across all calls which is bad because in operation modes with an IV every encryption block depends on the block before ... this is good for security ... but very bad if the block that was encrypted before doesn't belong to the current stream of blocks because another tread pipes blocks through the same instance at the same time ...

    2. you don't care if your iCryptoTransform is reusable ... in your case, it is, but normally, you need to check the CanReuseTransform member or create a new one for every operation

    suggestion:

    let your ICryptoTransform live inside the Encrypt()/Decrypt() method ... don't use static variables ...