AES implementation

Question

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[] bytes = Convert.FromBase64String(CipherText);
        using (var ms = new MemoryStream(bytes))
        {
            rm.Key = key.GetBytes(rm.KeySize / 8);
            //Get the initialization vector from the encrypted stream
            rm.IV = ReadByteArray(ms);
            ICryptoTransform decryptor = rm.CreateDecryptor(rm.Key, rm.IV);
            using (var cs = new CryptoStream(ms, decryptor, CryptoStreamMode.Read))
            {
                using (var sr = new StreamReader(cs))
                {
                    //Read bytes from stream to the end
                    plainText = sr.ReadToEnd();
                }
            }
        }

        return plainText;
    }

    private static byte[] ReadByteArray(Stream s)
    {
        byte[] rawLength = new byte[sizeof(int)];
        if(s.Read(rawLength, 0, rawLength.Length) != rawLength.Length)
        {
            throw new SystemException("Stream did not contain properly formatted byte array");
        }

        byte[] buffer = new byte[BitConverter.ToInt32(rawLength, 0)];
        if(s.Read(buffer, 0, buffer.Length) != buffer.Length)
        {
            throw new SystemException("Did not read byte array properly");
        }

        return buffer;
    }

Answer 1

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.

Answer 2

• Instead of having private string _sharedSecret; you should consider to use SecureString instead. Each time you need this password you should convert it to a string. Read how-to-convert-securestring-to-system-string and how-to-properly-convert-securestring-to-string

• Don't create the RijndaelManaged rm; in the constructor. Create it if you need it. Right now you are storing after you first it the key and IV in this instance.

• The RijndaelManaged has an overloaded CreateEncryptor() method without parameter which just uses the current key and IV. So there is no need to pass them to the method.

• Rfc2898DeriveBytes and RijndaelManaged both implement IDisposable so you should enclose them in a using statement.

So for instance by using the following method

private ICryptoTransform GetEncryptor()
{
        using (RijndaelManaged rm = new RijndaelManaged() { Mode = CipherMode.CBC })
        using (var key = new Rfc2898DeriveBytes(_sharedSecret, _salt, 10000))
        {
            rm.Key = key.GetBytes(rm.KeySize / 8);
            rm.GenerateIV();
            return rm.CreateEncryptor();
        }
}  

you can change the former Encrypt() method to

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

    string output = null;

    using (ICryptoTransform encryptor = GetEncryptor())
    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;
}

You may have noticed that I have change the casing of the parameter PlainText from using PascalCase to camelCase because thats how it is defined in the .NET naming guidelines.

Speaking about naming, you are mixing your naming styles. You have classmember variables starting with and without an underscore. You should stick to a choosen style.

Speaking about style, you are mixing your style regarding using braces {} for single line if statements. I would like to encourage you to always use them to make your code less error prone.

Answer 3

Your code overall is quite easy to read and understand. It's so easy that even I could read it! You know what that means? A+ on readability!

But here's a very scary thing:

private static byte[] _salt = Encoding.ASCII.GetBytes("SomeConstantSalt");

You shouldn't have the same salt for everything. Salts are meant to be unique per encryption. They don't need to be secret, just unique.

---

You are storing your shared secret like this:

private string _sharedSecret;

This is so easy to extract from your memory, with something like CheatEngine. I would try to do something like this:

private byte[] encodeSecret (byte[] secret, byte key)
{
    int length = secret.Length;
    for (var i = 0; i < length; i++)
    {
        key = secret[i] ^ key;
    }

    return secret;
}

private byte[] decodeSecret (byte[] secret, byte key)
{
    return encodeSecret(secret, key);
}

This is nowhere perfect, but it is better than storing it in plain text. This is just a tad better than nothing. Still, it isn't safe.

---

There seems to be a magic number:

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

Besides this line being repeated, it also has the number of rounds to run.

You should create a constant for that, at the top.

---

Since the point above mentions a repeated line, I suggest the following:

private const ROUNDS = 10000;
private byte[] getDerivatedKey()
{
    var key = new Rfc2898DeriveBytes(_sharedSecret, _salt, ROUNDS);
    return key.GetBytes(rm.KeySize / 8);
}

---

I'm sure there's more to improve, but my knowledge is fairly limited.