C# AES + RSA Encryption Implementation

Question

I've put together an AES encryption implementation to solve a set of requirements. I believe that I've met the requirements, but I'd like to see if anyone is willing to review the implementation to ensure that my crypto approach is sound.

Requirements

• Static symmetric encryption API composed of Encrypt(string) and Decrypt(string) methods.
• Multi-server use. Cannot use server scoped encryption. Previous Triple DES implementation did this.
• Passphrase used to derive encryption key is stored as plaintext on disc (see "Notes" section below).

Encryption libraries used:

• AES - System.Security.Cryptography.RijndaelManaged
• RSA - System.Security.Cryptography.RSACryptoServiceProvider

Usage prerequisites

• Server using encryption library must have certificate installed in certificate store.
• Server using encryption library must have plain text passphrase present on disc.

Encryption Flow

• Application requests encryption from AES library.
• AES retrieves passphrase from disc.
• 256 bit AES encryption key is derived from passphrase using certificate.
• AES encryption is performed using key that is derived from RSA encryption.

Notes

I've omitted certain business requirements that are driving the requirements here for brevity. I do recognize that the additional server certificate security layer seems unnecessary without those details. The basic idea is that even if you know the base passphrase, you still need to have the certificate installed in order to encrypt/decrypt.

Code (semi-redacted)

AES

public static class AES
{
    private static readonly byte[] _Salt = new byte[] { 1, 2, 23, 234, 37, 48, 134, 63, 248, 4 };
    private const int KEY_SIZE = 256;
    private const int BLOCK_SIZE = 128;
    private const PaddingMode PADDING_MODE = PaddingMode.PKCS7;
    private static readonly byte[] _AESKey = GetEncryptionKey(KEY_SIZE);

    public static string Encrypt(string dataToEncrypt)
    {
        if (dataToEncrypt == null || dataToEncrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToEncrypt");
        }

        var data = Encoding.UTF8.GetBytes(dataToEncrypt);
        var encrypted = Encrypt(data);

        return Convert.ToBase64String(encrypted);
    }

    public static string Decrypt(string dataToDecrypt)
    {
        if (dataToDecrypt == null || dataToDecrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToDecrypt");
        }

        var data = Convert.FromBase64String(dataToDecrypt);
        var decrypted = Decrypt(data);

        return Encoding.UTF8.GetString(decrypted);
    }

    public static byte[] Encrypt(byte[] dataToEncrypt)
    {
        if (dataToEncrypt == null || dataToEncrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToEncrypt");
        }

        return Encrypt(
            dataToEncrypt,
            _AESKey
            );
    }

    public static byte[] Decrypt(byte[] dataToDecrypt)
    {
        if (dataToDecrypt == null || dataToDecrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToDecrypt");
        }

        return Decrypt(
            dataToDecrypt,
            _AESKey
            );
    }

    private static byte[] Encrypt(byte[] dataToEncrypt, byte[] key)
    {
        byte[] encryptedData;

        using (var rij = new RijndaelManaged())
        {
            rij.KeySize = KEY_SIZE;
            rij.BlockSize = BLOCK_SIZE;
            rij.Padding = PADDING_MODE;
            rij.Key = key;
            rij.GenerateIV();

            ICryptoTransform encryptor = rij.CreateEncryptor(rij.Key, rij.IV);

            using (var memoryStream = new MemoryStream())
            {
                using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
                {
                    using (var binaryWriter = new BinaryWriter(cryptoStream))
                    {
                        binaryWriter.Write(rij.IV);
                        binaryWriter.Write(dataToEncrypt);
                        cryptoStream.Flush();
                        cryptoStream.FlushFinalBlock();

                        memoryStream.Position = 0;
                        encryptedData = memoryStream.ToArray();
                    }



                    memoryStream.Close();
                    cryptoStream.Close();
                }
            }
        }

        return encryptedData;
    }

    private static byte[] Decrypt(byte[] dataToDecrypt, byte[] key)
    {
        byte[] decrypted;

        using (var rij = new RijndaelManaged())
        {
            rij.KeySize = KEY_SIZE;
            rij.BlockSize = BLOCK_SIZE;
            rij.Padding = PADDING_MODE;
            rij.Key = key;
            var iv = new byte[16];
            Array.Copy(dataToDecrypt, 0, iv, 0, iv.Length);
            rij.IV = iv;

            ICryptoTransform decryptor = rij.CreateDecryptor(rij.Key, rij.IV);

            using (var memoryStream = new MemoryStream())
            {
                using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Write))
                {
                    using (var binaryWriter = new BinaryWriter(cryptoStream))
                    {
                        binaryWriter.Write(dataToDecrypt, iv.Length, dataToDecrypt.Length - iv.Length);
                        cryptoStream.Flush();
                        cryptoStream.FlushFinalBlock();
                        memoryStream.Position = 0;
                    }

                    decrypted = memoryStream.ToArray();
                    memoryStream.Close();
                    cryptoStream.Close();
                }
            }
        }

        return decrypted;
    }

    private static byte[] GenerateIV()
    {
        byte[] iv;

        using (var aes = new AesCryptoServiceProvider())
        {
            aes.GenerateIV();
            iv = aes.IV;
        }

        return iv;
    }

    private static byte[] GetEncryptionKey(int keySize)
    {
        byte[] key;
        var passphrase = @"some passphrase retrieved from server";

        var encryptedPassphrase = RSA.Encrypt(passphrase);
        var encryptedPassphraseBytes = new byte[encryptedPassphrase.Length * sizeof(char)];
        System.Buffer.BlockCopy(encryptedPassphrase.ToCharArray(), 0, encryptedPassphraseBytes, 0, encryptedPassphraseBytes.Length);

        return encryptedPassphraseBytes.Take(keySize / 8).ToArray();
    }
}

RSA

static class RSA
{
    private static X509Certificate2 GetEncryptionCertificate()
    {
        X509Certificate2 encryptionCertificate = null;
        X509Store certificateStore = new X509Store(StoreLocation.LocalMachine);
        certificateStore.Open(OpenFlags.ReadOnly);

        foreach (var certificate in certificateStore.Certificates)
        {
            if (certificate.FriendlyName == "NameOfCertificate")
            {
                encryptionCertificate = certificate;
                break;
            }
        }

        certificateStore.Close();

        if (encryptionCertificate == null)
        {
            throw new Exceptions.EncryptionCertificateNotFoundException();
        }

        return encryptionCertificate;
    }

    public static string Encrypt(string dataToEncrypt)
    {
        if (dataToEncrypt == null || dataToEncrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToEncrypt");
        }

        var data = Encoding.UTF8.GetBytes(dataToEncrypt);
        var encrypted = Encrypt(data);

        return Convert.ToBase64String(encrypted);
    }

    public static string Decrypt(string dataToDecrypt)
    {
        if (dataToDecrypt == null || dataToDecrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToDecrypt");
        }

        var data = Convert.FromBase64String(dataToDecrypt);
        var decrypted = Decrypt(data);

        return Encoding.UTF8.GetString(decrypted);
    }

    public static byte[] Encrypt(byte[] dataToEncrypt)
    {
        if (dataToEncrypt == null || dataToEncrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToEncrypt");
        }

        byte[] encrypted;

        using (RSACryptoServiceProvider rsa = (RSACryptoServiceProvider)GetEncryptionCertificate().PublicKey.Key)
        {
            encrypted = rsa.Encrypt(dataToEncrypt, false);
        }

        return encrypted;
    }

    public static byte[] Decrypt(byte[] dataToDecrypt)
    {
        if (dataToDecrypt == null || dataToDecrypt.Length < 1)
        {
            throw new ArgumentNullException("dataToDecrypt");
        }

        byte[] decrypted;

        using (RSACryptoServiceProvider rsa = (RSACryptoServiceProvider)GetEncryptionCertificate().PrivateKey)
        {
            decrypted = rsa.Decrypt(dataToDecrypt, false);
        }

        return decrypted;
    }
}

Answer 1

As far as I can tell, this

byte[] encryptedData;

// ...

using (var memoryStream = new MemoryStream())
{
    using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
    {
        using (var binaryWriter = new BinaryWriter(cryptoStream))
        {
            binaryWriter.Write(rij.IV);
            binaryWriter.Write(dataToEncrypt);
            cryptoStream.Flush();
            cryptoStream.FlushFinalBlock();

            memoryStream.Position = 0;
            encryptedData = memoryStream.ToArray();
        }



        memoryStream.Close();
        cryptoStream.Close();
    }
}

return encryptedData;

Can be replaced with this

using (var memoryStream = new MemoryStream())
{
    using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
    {
        cryptoStream.Write(rij.IV, 0, rij.IV.Length);
        cryptoStream.Write(dataToEncrypt, 0, dataToEncrypt.Length);
    }

    return memoryStream.ToArray();
}

• CryptoStream.Flush is a no-op
• Disposing of the CryptoStream will call FlushFinalBlock.
• ToArray can still be called on the MemoryStream after it has been closed by disposing of the CryptoStream.
• ToArray "writes the stream contents to a byte array, regardless of the Position property."

Similarly, this

byte[] decrypted;

// ...

using (var memoryStream = new MemoryStream())
{
    using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Write))
    {
        using (var binaryWriter = new BinaryWriter(cryptoStream))
        {
            binaryWriter.Write(dataToDecrypt, iv.Length, dataToDecrypt.Length - iv.Length);
            cryptoStream.Flush();
            cryptoStream.FlushFinalBlock();
            memoryStream.Position = 0;
        }

        decrypted = memoryStream.ToArray();
        memoryStream.Close();
        cryptoStream.Close();
    }
}

return decrypted;

Can be written

using (var memoryStream = new MemoryStream())
{
    using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Write))
    {
        cryptoStream.Write(dataToDecrypt, iv.Length, dataToDecrypt.Length - iv.Length);
    }

    return memoryStream.ToArray();
}

---

dataToEncrypt == null || dataToEncrypt.Length < 1

All code like this can be replaced with

string.IsNullOrEmpty(dataToEncrypt)

---

byte[] iv;

using (var aes = new AesCryptoServiceProvider())
{
    aes.GenerateIV();
    iv = aes.IV;
}

return iv;

Can be written

using (var aes = new AesCryptoServiceProvider())
{
    aes.GenerateIV();
    return aes.IV;
}

Similar rewrites are possible in RSA.Encrypt(byte[]) and RSA.Decrypt(byte[]).

---

There's an unused variable key in AES.GetEncryptionKey.

Answer 2

You should, in general, avoid using concrete implementation types for cryptographic algorithms in .NET (as of 4.6.1 that should always work).

• Don't use RijndaelManaged, use Aes.
• You are both assigning the Key property then reading it to call CreateEncryptor(byte[], byte[]). You only need to set it to call CreateEncryptor(). Pick a paradigm and stick with it.
• You have a private GenerateIV method, but you don't call it here. Using the object's own GenerateIV seems the simplest.
• Don't use AesCryptoServiceProvider, use Aes.
• Why are you using both RijndaelManaged AND AesCryptoServiceProvider?
• Don't use RSACryptoServiceProvider, use RSA.
  • Don't use X509Certificate2.PrivateKey use X509Certificate2.GetRSAPrivateKey().
  • (This last point is so that you can handle CNG-backed certificates, and get null instead of a crash on non-RSA certs).
• ICryptoTransform is IDisposable, so you should have it in a using statement.
• You seem to be writing the IV in through the CryptoStream, but reading as if it were plain data. Are you sure your first block isn't coming back corrupt?

Algorithmically speaking, you are using the public key as a secret in a custom Key Derivation Function. Much more normal would be to distribute the RSA encrypted symmetric key and use the private key to decrypt it. Quite a lot of things won't consider the .cer portion of a certificate to be secret. And the usual rule in cryptography is "if you think you're being clever, you're getting less secure"... you should use established practices or KDF algorithms, not roll your own.

RSA Decrypt:

public static byte[] Decrypt(byte[] dataToDecrypt)
{
    if (dataToDecrypt == null || dataToDecrypt.Length < 1)
    {
        throw new ArgumentNullException(nameof(dataToDecrypt));
    }

    using (RSA rsa = GetEncryptionCertificate().GetRSAPrivateKey())
    {
        return rsa.Decrypt(dataToDecrypt, RSAEncryptionPadding.Pkcs1);
    }
}

AES Encrypt:

private static byte[] Encrypt(byte[] dataToEncrypt, byte[] key)
{
    using (var aes = Aes.Create())
    {
        // Setting KeySize is only important for calling GenerateKey, don't need to do it.
        //rij.KeySize = KEY_SIZE;

        // AES only has one valid block size (128 bit), no need to set this.
        //rij.BlockSize = BLOCK_SIZE;

        // You are explicitly setting the padding mode to the default,
        // but aren't setting the cipher mode.
        // It seems like you'd either do both for being explicit,
        // or neither for consistency.
        aes.Padding = PADDING_MODE;
        aes.Mode = CipherMode.CBC;

        // Don't set the key property if you're going to call CreateEncryptor(byte[], byte[])
        // since you're just causing more memory copying.
        //rij.Key = key;

        // Regenerating a new IV every call is good.
        // But this is actually redundant, because when the IV is read
        // the first time after creating the object it will effectively
        // call GenerateIV for you. So this doesn't really help, but it's
        // a personal call for if it makes you feel better to have it here
        // or to reduce the number of lines of code.
        //rij.GenerateIV();
        byte[] iv = aes.IV;

        using (ICryptoTransform encryptor = aes.CreateEncryptor(key, iv))
        using (var memoryStream = new MemoryStream())
        {
            memoryStream.Write(iv, 0, iv.Length);

            using (var cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
            {
                cryptoStream.Write(dataToEncrypt, 0, dataToEncrypt.Length);
            }

            return memoryStream.ToArray();
        }
    }
}