C# AES Encryption/Decryption or byte[] array with a custom Key/IV

Question

I wanted to create an AES encryption service which will use a custom key/iv.

I want 2 methods: dycrypt/encrypt byte[]

The code works as expected, any remarks/issues?

public class AesGenericEncryptionService
{
    private readonly byte[] m_Key;
    private readonly byte[] m_IV;

    public AesGenericEncryptionService(byte[] key, byte[] iv)
    {
        if (key == null || key.Length <= 0)
            throw new ArgumentNullException(nameof(key));
        if (iv == null || iv.Length <= 0)
            throw new ArgumentNullException(nameof(iv));

        m_Key = key;
        m_IV = iv;
    }

    public byte[] Encrypt(byte[] data)
    {
        byte[] encrypted;

        using (RijndaelManaged rijAlg = new RijndaelManaged())
        {
            rijAlg.Mode = CipherMode.CBC;
            rijAlg.KeySize = m_Key.Length * 8;
            rijAlg.Key = m_Key;
            rijAlg.BlockSize = m_IV.Length * 8;
            rijAlg.IV = m_IV;

            using (ICryptoTransform encryptor = rijAlg.CreateEncryptor(rijAlg.Key, rijAlg.IV))
            {
                using (MemoryStream msEncrypt = new MemoryStream())
                {
                    using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
                    {
                        csEncrypt.Write(data, 0, data.Length);
                        csEncrypt.FlushFinalBlock();

                        encrypted = msEncrypt.ToArray();
                    }
                }
            }
        }

        return encrypted;
    }
    public byte[] Decrypt(byte[] cipher)
    {
        // Check arguments.
        if (cipher == null || cipher.Length <= 0)
            throw new ArgumentNullException(nameof(cipher));

        byte[] dycrypted = null;
        using (RijndaelManaged rijAlg = new RijndaelManaged())
        {
            rijAlg.Mode = CipherMode.CBC;
            rijAlg.KeySize = m_Key.Length * 8;
            rijAlg.Key = m_Key;
            rijAlg.BlockSize = m_IV.Length * 8;
            rijAlg.IV = m_IV;

            using (ICryptoTransform decryptor = rijAlg.CreateDecryptor(rijAlg.Key, rijAlg.IV))
            {
                using (MemoryStream ms = new MemoryStream(cipher))
                {
                    using (var cryptoStream = new CryptoStream(ms, decryptor, CryptoStreamMode.Read))
                    {
                        dycrypted = new byte[cipher.Length];
                        var bytesRead = cryptoStream.Read(dycrypted, 0, cipher.Length);

                        dycrypted = dycrypted.Take(bytesRead).ToArray();
                    }
                }
            }
        }

        return dycrypted;
    }
}

Answer 1

ctor

Neither key.Length nor iv.Length can be smaller than 0 you only need to check if it is equal to 0. But for me as a user of that class it would look strange to receive an ArgumentNullException if I pass e.g a key with Length == 0. I would expect an ArgumentOutOfRangeException.

Omitting braces althougth they might be optional can lead to hidden and therfor hard to find bugs.

Encrypt()

• The default Mode of RijndaelManaged is already CipherMode.CBC so there is no need to set it again.

• By returning out of the most inner using you can remove byte[] encrypted.

• By stacking the using's you save some levels of indentation.

• You can use the parameterless CreateEncryptor() method because you already set the Key and IV.

• As the method is public you should validate its parameter.

Decrypt()

• The default Mode of RijndaelManaged is already CipherMode.CBC so there is no need to set it again.

• By returning out of the most inner using you can remove byte[] decrypted.

• By stacking the using's you save some levels of indentation.

• You can use the parameterless CreateDecryptor() method because you already set the Key and IV.

• You shouldn't mix styles. In the using's you sometimes use the concrete type and sometimes you use var.

General

Naming things in a good way is hard. Mostly it is sufficient and just more readable to just use a classname in camelCase casing. If you stumble in a few months over the variable rijAlg you will need to ask yourself what it stands for. By naming it rijndaelManaged you will see at first glance what it is about.
Another example of "bad" naming is in Encrypt() you name the MemoryStream msEncrypt but in Decrypt() you name it ms the same is true for csEncrypt vs cryptoStream.

Implementing the mentioned points leads to

public class AesGenericEncryptionService
{
    private readonly byte[] m_Key;
    private readonly byte[] m_IV;

    public AesGenericEncryptionService(byte[] key, byte[] iv)
    {
        if (key == null)
        {
            throw new ArgumentNullException(nameof(key));
        }
        else if (key.Length == 0)
        {
            throw new ArgumentOutOfRangeException(nameof(key));
        }

        if (iv == null || iv.Length <= 0)
        {
            throw new ArgumentNullException(nameof(iv));
        }
        else if (iv.Length == 0)
        {
            throw new ArgumentOutOfRangeException(nameof(iv));
        }

        m_Key = key;
        m_IV = iv;
    }

    public byte[] Encrypt(byte[] data)
    {
        if (data == null)
        {
            throw new ArgumentNullException(nameof(data));
        }

        using (var rijndaelManaged = new RijndaelManaged())
        {
            rijndaelManaged.KeySize = m_Key.Length * 8;
            rijndaelManaged.Key = m_Key;
            rijndaelManaged.BlockSize = m_IV.Length * 8;
            rijndaelManaged.IV = m_IV;

            using (var encryptor = rijndaelManaged.CreateEncryptor())
            using (var ms = new MemoryStream())
            using (var cryptoStream = new CryptoStream(ms, encryptor, CryptoStreamMode.Write))
            {
                cryptoStream.Write(data, 0, data.Length);
                cryptoStream.FlushFinalBlock();

                return ms.ToArray();
            }
        }
    }
    public byte[] Decrypt(byte[] cipher)
    {
        // Check arguments.
        if (cipher == null)
        {
            throw new ArgumentNullException(nameof(cipher));
        }
        else if (cipher.Length == 0)
        {
            throw new ArgumentOutOfRangeException(nameof(cipher));
        }

        using (var rijndaelManaged = new RijndaelManaged())
        {
            rijndaelManaged.KeySize = m_Key.Length * 8;
            rijndaelManaged.Key = m_Key;
            rijndaelManaged.BlockSize = m_IV.Length * 8;
            rijndaelManaged.IV = m_IV;

            using (var decryptor = rijndaelManaged.CreateDecryptor())
            using (var ms = new MemoryStream(cipher))
            using (var cryptoStream = new CryptoStream(ms, decryptor, CryptoStreamMode.Read))
            {

                var dycrypted = new byte[cipher.Length];
                var bytesRead = cryptoStream.Read(dycrypted, 0, cipher.Length);

                return dycrypted.Take(bytesRead).ToArray();
            }
        }
    }
}

Answer 2

public class AesGenericEncryptionService

That's not a very good name. The AES method itself already performs "generic" encryption. I'm also not sure if you would currently name anything "generic" if it doesn't perform authenticated encryption (encryption + MAC) such as AES/GCM. See the bottom of my answer for such an implementation. If you'd use this "generic encryption" for transport mode security then you are vulnerable to plaintext / padding oracle attacks as well as undetected change of the plaintext during transit.

private readonly byte[] m_IV;

Storing a static IV makes this object single use. That's not something that is wrong in particular, but it is something to keep in mind. You would wonder if a single method call would not achieve the same. Key + IV reuse makes a cipher vulnerable after all.

rijAlg.Mode = CipherMode.CBC;

I'm glad that you are setting this explicitly, contrary to the other answer. Using defaults for cryptographic methods leads to unreadable code, where the reader has to guess which mode has been used.

rijAlg.KeySize = m_Key.Length * 8;

This, on the other hand, is directly the same as just assigning the key, the key size will be set automatically.

rijAlg.BlockSize = m_IV.Length * 8;

This is dangerous, as Rijndael accepts different block sizes, while AES doesn't. So if you allow this then you've named your class incorrectly. And again, it's not really needed. You should instead make sure that your IV is always 128 bits if you want AES.

---

In general, this class is just hiding detail from RijndaelManaged. It does this in such a way that using the actual class is possibly a better idea (as I've found out when I wrote my own "wrapper" classes).

Finally, RijndaelManaged is, as the name suggests, the managed version of AES, i.e. executed byte code instead of using a native implementation. In general, I'd prefer just Aes.Create() so you can use the hardware acceleration that the native implementation provides (on a system with AES-NI or something supported & similar anyway). It can fall back on RijndaelManaged where required.

---

Here is a new AES / GCM implementation by Microsoft. Note that it provides "one shot" encryption like your class. However, it only uses the key as field, not the nonce (which replaces the IV).