AES implementation

We are using an outdated 3DES algorithm for encryption and I have been tasked with writing a new implementation using AES with a 128bit shared secret.
I would like to know if there are any security holes in my implementation and if there is any way to optimize the code. Please note that I have removed the code for implementation of IDisposable since there is nothing special going on there.

```
public class AESCrypto : IDisposable
{
private static byte[] _salt = Encoding.ASCII.GetBytes("SomeConstantSalt");
private string _sharedSecret;
private RijndaelManaged rm;

public AESCrypto(string SharedSecret)
{
rm = new RijndaelManaged();
rm.Mode = CipherMode.CBC;
_sharedSecret = SharedSecret;
}

public string Encrypt(string PlainText)
{
if (string.IsNullOrEmpty(PlainText))
throw new ArgumentNullException("PlainText");

string output = null;

var key = new Rfc2898DeriveBytes(_sharedSecret, _salt, 10000);
rm.Key = key.GetBytes(rm.KeySize / 8);
rm.GenerateIV();

using (ICryptoTransform encryptor = rm.CreateEncryptor(rm.Key, rm.IV))
using (var ms = new MemoryStream())
{
//Prepend the initialization vector
ms.Write(BitConverter.GetBytes(rm.IV.Length), 0, sizeof(int));
ms.Write(rm.IV, 0, rm.IV.Length);
using (var cs = new CryptoStream(ms, encryptor, CryptoStreamMode.Write))
{
using (var sw = new StreamWriter(cs))
{
sw.Write(PlainText);
}
}
output = Convert.ToBase64String(ms.ToArray());
}

return output;
}

public string Decrypt(string CipherText)
{
if (string.IsNullOrEmpty(CipherText))
throw new ArgumentNullException("CipherText");

string plainText = null;

var key = new Rfc2898DeriveBytes(_sharedSecret, _salt, 10000);

byte[]

Since I'm more familiar with cryptography than with C#, I'm going to mainly focus on the high-level cryptographic aspects of your code (mostly, key management) rather than on coding style.

-
You should not be using Rfc2898DeriveBytes on every call to Decrypt or Encrypt. The PBKDF2 algorithm used by Rfc2898DeriveBytes is (deliberately) very slow, so you should minimize the number of times you need to run it.

At the very least, use Rfc2898DeriveBytes only in the constructor, and either store the resulting key in something like a SecureString (as suggested by Heslacher) or pass it directly to a RijndaelManaged instance and let it store it for you.

(Disclaimer: I'm not familiar enough with RijndaelManaged to say how efficient and secure storing instances of that class for long periods might be. Certainly, though, if you're going to be keeping one around, you might as well let it store your key. And honestly, it probably won't be the weakest link in your security anyway.)

-
As noted in other answers, hardcoding the iteration count for Rfc2898DeriveBytes is bad style. Ideally, you should be adjusting this value to be as large as possible, while still delivering acceptable performance. At least, you should make it a named constant that you can adjust later. Better yet, you could make it adjustable at runtime, and perhaps even adjust it automatically based on benchmarks. (Note that this may require storing the iteration count alongside the encrypted messages. Also, just to be safe, you should set some reasonable minimum value for the iteration count.)

-
You might also consider decoupling the key derivation from your encryption code entirely, such as having a separate AESKey class that encapsulates one key (e.g. in a SecureString), maybe with a factory method like AESKey.FromPassword() that uses Rfc2898DeriveBytes.

You could even use a two-stage PBKDF2 + HKDF-Expand scheme, as described e.g. in this crypto.SE answer (or this one), where you'd have a single CryptoKey class that encapsulates an "intermediate master key" derived (e.g.) using Rfc2898DeriveBytes, and which exposed a getDerivedKey(int bytes, string purpose) method that used HKDF-Expand from RFC 5869 to derive and return a subkey for the indicated purpose. (HKDF-Expand is a very simple algorithm to implement; it basically just amounts to calling HMAC repeatedly until you have enough bytes.) This would allow you to initialize the CryptoKey class once, and then pass it to any code that needs an encryption key, relying on said code to call getDerivedKey() with a different purpose identifier to get quasi-independent derived keys.

-
Finally, since you're changing your encryption scheme anyway, you almost certainly should be switching to authenticated encryption while you're at it. This will not only protect your messages against tampering, but as a side effect, it'll also protect you from a wide class of other cryptographic attacks (such as padding oracle attacks on CBC mode).

In its most basic form, you could implement AE by calculating an HMACSHA256 authentication token on your ciphertext after encryption (ideally, using a separate key derived from your shared secret, e.g. using the PBKDF2 + HKDF scheme described above), and appending it to the encrypted message. When decrypting, you'd first strip off this authetication token, recalculate it for the remaining ciphertexts, and compare the received and recalculated tokens to make sure they match. Only if they do should you then proceed to actually decrypt the message.

StackExchange Code Review Q#101272, answer score: 12