13
\$\begingroup\$

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;
    }
\$\endgroup\$
  • \$\begingroup\$ Rfc2898DeriveBytes implements IDisposable, so it's lifetime should be wrapped in a using construct. Further, your elided Dispose() code should be calling rm.Dispose(); at the very least since the class holds an IDisposable resource in the RijndaelManaged member. \$\endgroup\$ – Jesse C. Slicer Aug 18 '15 at 11:59
  • 1
    \$\begingroup\$ @JesseC.Slicer - Thank you. The Dispose method disposes of rm. I will add a using statment for Rfc2898DeriveBytes as you suggested. \$\endgroup\$ – Elad Lachmi Aug 18 '15 at 12:02
  • 1
    \$\begingroup\$ You can ask those new questions with new code in a follow-up question. The current question has already received answers. \$\endgroup\$ – Jamal Aug 18 '15 at 16:10
12
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Great info! Just one question. Can you look at the edit I made and maybe try and clear up where that fits in to the schema you described? Since I am adding the generated IV key to the cipher, I thought that does the MAC job. Am I missing something? Thank you! \$\endgroup\$ – Elad Lachmi Aug 18 '15 at 15:12
  • \$\begingroup\$ The IV is not a MAC; that should be obvious from the fact that the IV can be computed without knowing the message. (Tangent: It is possible to use a MAC as an IV, if one is careful; this is notably done in SIV mode by Rogaway and Shrimpton.) The purpose of a MAC is to detect modification of the message, and so prevent attacks that exploit the malleability of the encryption mode. \$\endgroup\$ – Ilmari Karonen Aug 18 '15 at 15:22
  • \$\begingroup\$ Since the .net implementation of aes does not support aes-gcm, I am adding an HMAC hash at the end of the message. Can I use the shared secret as the authentication token or do I need to share another bit of information between the systems? \$\endgroup\$ – Elad Lachmi Aug 18 '15 at 16:04
  • \$\begingroup\$ You can use a key derived from the shared secret as the HMAC key. (Presumably, since you're using Rfc2898DeriveBytes, that means that your original shared secret may have low entropy, and should not be used directly for anything.) You could just reuse the AES key as your HMAC key, and it should be safe, but cryptographers tend to be paranoid about key reuse, and would much rather prefer that you used e.g. HKDF to derive two separate quasi-independent subkeys from your master key (itself derived from the shared secret using Rfc2898DeriveBytes) and use them for AES and HMAC respectively. \$\endgroup\$ – Ilmari Karonen Aug 18 '15 at 16:10
  • \$\begingroup\$ That is just what I was looking for. Thank you very much. \$\endgroup\$ – Elad Lachmi Aug 18 '15 at 16:21
8
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ I was going to look up into SecureString, but you are way ahead of me! A great answer indeed! \$\endgroup\$ – Ismael Miguel Aug 18 '15 at 12:48
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ All great points. Thank you! About the salt, if I generate a random salt, how do I go about attaching it to the cipher text so that it can be used for decryption? \$\endgroup\$ – Elad Lachmi Aug 18 '15 at 11:53
  • 1
    \$\begingroup\$ @EladLachmi The only way would be to return it as part of the encryption value. Then you can add a parameter to receive the salt \$\endgroup\$ – Ismael Miguel Aug 18 '15 at 11:55
  • \$\begingroup\$ Also, I'm very happy that you find it readable! :) \$\endgroup\$ – Elad Lachmi Aug 18 '15 at 11:58
  • 1
    \$\begingroup\$ Your encodeSecret() function seems frankly useless. If the code is not running on a hostile system (where an attacker might e.g. examine it with a debugger), it's not needed; if it is, it will only slow the attacker by maybe a few minutes at most. Basically, it just provides an illusion of security without any actual substance. If you were really concerned about memory inspection attacks, you'd be much better off using SecureString, as suggested by Heslacher (but even that may not help in all cases; if an attacker can attach a debugger to your code, security is basically hopeless). \$\endgroup\$ – Ilmari Karonen Aug 18 '15 at 15:09
  • 1
    \$\begingroup\$ @EladLachmi: No, you don't need a random salt for key generation if you're already using a properly chosen (random, for CBC mode) IV for encryption. It's still a good idea to use an application-specific (and maybe e.g. recipient-specific) salt, just in case user might use the same "shared secret" for multiple purposes. (Also, you do want a random salt if you're using Rfc2898DeriveBytes for creating a password hash that will be stored in a user database, but that doesn't apply as long as you're not permanently storing your keys anywhere.) \$\endgroup\$ – Ilmari Karonen Aug 18 '15 at 15:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.