1
$\begingroup$

I need an encryption algorithm that would be secure enough to store credit card data. So it should be reasonably secure.

Here's what I've come up with. I'd appreciate any constructive criticism. Is it secure enough? Could it be made better?

public class Encryption
{
    // Algorithm meta sizes (in bits)
    private const int SaltSize = 256;
    private const int BlockSize = 256;
    private const int KeySize = 256;

    // Number of iterations for password bytes generation
    private const int DerivationIterations = 1000;

    /// <summary>
    /// Encrypts a string using the given password.
    /// </summary>
    /// <param name="s">String to encrypt.</param>
    /// <param name="password">Encryption password.</param>
    /// <returns>Encrypted string.</returns>
    public string Encrypt(string s, string password)
    {
        if (s == null)
            throw new ArgumentNullException(nameof(s));
        if (password == null)
            throw new ArgumentNullException(nameof(password));
        if (password.Length == 0)
            throw new ArgumentException("Password cannot be empty", nameof(password));

        var salt = GetRandomBytes(SaltSize);
        var iv = GetRandomBytes(BlockSize);
        var data = Encoding.UTF8.GetBytes(s);

        using (Rfc2898DeriveBytes derivedKey = new Rfc2898DeriveBytes(password, salt, DerivationIterations))
        {
            var keyBytes = derivedKey.GetBytes(KeySize / 8);
            using (var encrypt = new RijndaelManaged())
            {
                encrypt.BlockSize = BlockSize;
                encrypt.Mode = CipherMode.CBC;
                encrypt.Padding = PaddingMode.PKCS7;
                using (var encryptor = encrypt.CreateEncryptor(keyBytes, iv))
                {
                    using (var stream = new MemoryStream())
                    {
                        stream.Write(salt, 0, salt.Length);
                        stream.Write(iv, 0, iv.Length);
                        using (var cryptoStream = new CryptoStream(stream, encryptor, CryptoStreamMode.Write))
                        {
                            cryptoStream.Write(data, 0, data.Length);
                            cryptoStream.FlushFinalBlock();
                        }
                        return Convert.ToBase64String(stream.ToArray());
                    }
                }
            }
        }
    }

    /// <summary>
    /// Decrypts a string previously encrypted by <see cref="Encrypt"></see>.
    /// </summary>
    /// <param name="s">The text to decrypt.</param>
    /// <param name="password">The decryption password.</param>
    /// <returns>The decrypted string.</returns>
    public string Decrypt(string s, string password)
    {
        if (s == null)
            throw new ArgumentNullException(nameof(s));
        if (password == null)
            throw new ArgumentNullException(nameof(password));
        if (password.Length == 0)
            throw new ArgumentException("Password cannot be empty", nameof(password));

        try
        {
            // Get data plus salt and IV
            byte[] allData = Convert.FromBase64String(s);
            byte[] salt = new byte[SaltSize / 8];
            byte[] iv = new byte[BlockSize / 8];
            byte[] data = new byte[allData.Length - (salt.Length + iv.Length)];

            Buffer.BlockCopy(allData, 0, salt, 0, salt.Length);
            Buffer.BlockCopy(allData, salt.Length, iv, 0, iv.Length);
            Buffer.BlockCopy(allData, salt.Length + iv.Length, data, 0, data.Length);

            using (Rfc2898DeriveBytes derivedBytes = new Rfc2898DeriveBytes(password, salt, DerivationIterations))
            {
                var keyBytes = derivedBytes.GetBytes(KeySize / 8);
                using (var encrypt = new RijndaelManaged())
                {
                    encrypt.BlockSize = BlockSize;
                    encrypt.Mode = CipherMode.CBC;
                    encrypt.Padding = PaddingMode.PKCS7;
                    using (var decryptor = encrypt.CreateDecryptor(keyBytes, iv))
                    {
                        using (var memoryStream = new MemoryStream(data))
                        using (var cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
                        {
                            var plainTextBytes = new byte[data.Length];
                            var decryptedByteCount = cryptoStream.Read(plainTextBytes, 0, plainTextBytes.Length);
                            return Encoding.UTF8.GetString(plainTextBytes, 0, decryptedByteCount);
                        }
                    }
                }
            }
        }
        catch (Exception)
        {
            return string.Empty;
        }
    }

    private byte[] GetRandomBytes(int bits)
    {
        byte[] bytes = new byte[bits / 8];
        using (var provider = new RNGCryptoServiceProvider())
        {
            provider.GetBytes(bytes);
        }
        return bytes;
    }
}

Note: According to the documentation for the Rijndael Class, "The Rijndael class is the predecessor of the Aes algorithm. You should use the Aes algorithm instead of Rijndael." I found that the code works if I change RijndaelManaged to AesCryptoServiceProvider and change the value of BlockSize to 128. But I'm not clear on if that makes things better.

$\endgroup$
8
  • $\begingroup$ Welcome to Cryptography. If you simplify the code into a smaller it will be better for us. We are not code reviewier. Even if you show only the important parts. Where do you store the card information? Do you need to execute a query if it is in the database? Devil is in the details and this is the same in Cryptography! $\endgroup$ – kelalaka Oct 21 '19 at 19:46
  • $\begingroup$ @kelalaka The scheme seems to be PBKDF2 with a freshly randomized salt and a randomized IV for key derivation and AES-CBC for data encryption. $\endgroup$ – SEJPM Oct 21 '19 at 19:50
  • $\begingroup$ @kelalaka: Well, the core issue I guess is how secure is Rijndael and if AES is better. I don't want to get too far into where I'm storing it as I'm not looking for help there. If you are concerned about a specific issue related to database queries, I'd love to hear it. But it's more about the encryption that I'm unsure of. $\endgroup$ – Jonathan Wood Oct 21 '19 at 19:52
  • $\begingroup$ The process is important. Rijndael is not AES and not standard. Actually I don't care about your storage or process you should care for it!. My suggestion is then, use Argon2id for key derivation and use AES-GCM or ChaCha20-Poly1305. $\endgroup$ – kelalaka Oct 21 '19 at 19:56
  • $\begingroup$ OK, clarifying questions: What are your sources of key material for the encryption? A password manually entered by the admin? A user password? A HSM? A cloud key management service? A local key management service? And if a password is actually the right choice here, what kind of machine and request-level parallelism are we talking about here? Are you expecting 1-2 users authenticating per second? Per minute? Is this for a native application on the client end or on a server? $\endgroup$ – SEJPM Oct 21 '19 at 19:59
2
$\begingroup$

As your network profile suggests, I will suppose that you know how to write code and will focus purely on the cryptography at play here.


So what the code currently does is roughly the following:

  1. Generate a random salt.
  2. Use that salt and a secure PBKDF (PBKDF2) to derive a key.
  3. Additionally generate a IV (technically not neccessary, but doesn't hurt either).
  4. Use the derived key and the IV to encrypt the data with AES-CBC.

This methodology is provably secure, for an appropriate and sensible definition of "secure". There are stronger (and safer) ones which can easily be achieved by switching out building blocks.


In particular the PBKDF can be switched out as well as the main encryption algorithm.

PBKDF2 is secure in itself but doesn't achieve a desirable security goal for a PBKDF: namely that the most efficient strategy for a brute-forcing adversary is to use the same hardware as you are using, in parcticular dedicated hardware and GPUs are much more energy-efficient in computing PBKDF2. The fix to this is to use so-called memory-hard password hashing, like Argon2id, which are optimized to not be computable cheaper on dedicated hardware than on a standard CPU.

AES-CBC "only" provides chosen-plaintext security which allow an adversary to modify your ciphertext without you noticing and allowing them get you specifically crafted plaintext back when decrypting. This in the past has led to exploitable breaches like eFail. The fix here is quite simple as well (conceptually): Using chosen-ciphertext secure (CCA-secure) encryption schemes which add authentication data to prevent these crafted decryptions to go by unnoticed. Examples for this include AES-GCM and ChaCha20-Poly1305 as used commonly in TLS.


Additional clarifications: Typically when people talk about "Rijndael" they talk about it instead of AES because they want properties that the former offers and the latter doesn't. But of course nobody has really seriously looked at these variants in the years since AES' standardization and therefore it's a bit risky to use them and usually you want other tools anyways when the need for e.g. a larger block size arises (also the variants have no HW support).

$\endgroup$

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.