5
\$\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.

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 symmetricKey = new RijndaelManaged())
                {
                    symmetricKey.BlockSize = BlockSize;
                    symmetricKey.Mode = CipherMode.CBC;
                    symmetricKey.Padding = PaddingMode.PKCS7;
                    using (var decryptor = symmetricKey.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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ If you're actually going to use this to store credit card details, would I be correct in assuming that the entire system is going to have to pass a PCI-DSS audit? \$\endgroup\$ – Peter Taylor Oct 20 at 20:03
  • \$\begingroup\$ I don't know. It's been a while since I processed credit cards. Why? Do you see an issue there? \$\endgroup\$ – Jonathan Wood Oct 20 at 20:07
  • 1
    \$\begingroup\$ It seems a bit odd to ask for a review of this code on two counts: firstly, it's arguably the least important part - certainly less important than the key management; and secondly, there seems to be a not inconsiderable risk that specialist auditors are going to come in and tell you not to roll your own crypto code, making the review pointless. \$\endgroup\$ – Peter Taylor Oct 20 at 21:11
  • \$\begingroup\$ @PeterTaylor: Well, that seems like an odd response to a review request. I've got two upvotes on my post and I think a lot of people are curious about stuff like this. If you think the topic is unimportant, then why would you even comment? If you don't see any value, you could just move on. \$\endgroup\$ – Jonathan Wood Oct 20 at 21:15
  • \$\begingroup\$ You might want to ask a cryptography expert then. Just to quote from the documentation though: "The Rijndael class is the predecessor of the Aes algorithm. You should use the Aes algorithm instead of Rijndael." So right from the start the answer would be "no". \$\endgroup\$ – ferada Oct 21 at 19:22
2
\$\begingroup\$

I'll do a full answer then: The algorithm choice seems bad suboptimal, or at least strange at first view: "The Rijndael class is the predecessor of the Aes algorithm. You should use the Aes algorithm instead of Rijndael."

Even then the general advice would be not to make up your own crypto scheme do these choices on your own from scratch. I usually look at this article every once in a while. Also of course https://crypto.stackexchange.com/ for detailed questions and answers. This answer might be a start.

Long story short would probably be that there are better options than the choice of algorithms that are present in the code and if you're not sure what to use, some good library that has already done those choices for you would be preferable, like NaCl (for .NET of course). I've seen some bindings for that, but I would hesitate to suggest which one is to be recommended at all.

Apart from algorithm choice a library might also be taking care of properly clearing any memory that was used (so some plaintext doesn't remain readable in memory) and prevents other problems (non-cryptographic random number generator, IV reuse, ... there's lots of things that can go wrong).

Also make sure to use of some form of authentication encryption so that you can be sure before decryption that the data hasn't been tampered with, c.f. Padding Oracle.

Finally, credit card data is probably also more of a regulatory problem, PCI DSS was mentioned.


Okay, so keeping this specific code in mind:

  • Depending on how it's stored (in a database at rest?) it might be good to also store the other parameters, like number of iterations (then again, the lengths could just also be stored, it's not a lot of data after all), as part of the data package, that way the parameters can be later increased without having to reprocess all stored data.
  • The exception should probably at least be logged so someone can check what's up in production.
  • new RNGCryptoServiceProvider() gets run too often, should be enough to keep one instance around?
  • The password check is just to make sure there was no coding error, right? Otherwise the minimum length should be a bit more, plus some dictionary checks etc. would be required. But then again, where's this password coming from, is it user-chosen or the global password for the whole database?
  • 1000 iterations seems low, but I also haven't (can't) checked what the actual run time of it is. To prevent attacks based on that it should probably be high enough to be noticeable when doing the password derivation. In any case there's a few better functions for this as mentioned in the linked documents.
\$\endgroup\$
  • \$\begingroup\$ Thanks, but AES is not my own crypto scheme. Regarding Rijndael vs AES, it looks like Rijndael is an implementation of AES. And there are tips for settings to make it like standard AES. The main difference appears to be restrictions AES puts on the block size. I've also posted on crypto.stackexchange.com as suggested. \$\endgroup\$ – Jonathan Wood Oct 21 at 19:42
  • \$\begingroup\$ Okay, rephrased it. But why would you have readers look that up if there's standard AES? Yes, also not made up, I've changed it, point being that it's easier to make unsafe choices if it's done from scratch without being vetted. Like, I think it actually looks safe (on its own! I've no idea how this is stored, if the password is randomised per entry etc.), but I wouldn't bet my money (well, credit card) on it. \$\endgroup\$ – ferada Oct 21 at 19:46
  • \$\begingroup\$ Not sure I follow. Have readers look up what? I just see a lot of encryption examples using Rijndael, and after reading your comments and I looked around, it seemed that Rijndael implements AES. I'm not clear on what you think I missed there. \$\endgroup\$ – Jonathan Wood Oct 21 at 20:12
  • \$\begingroup\$ I mean that readers will now have look up what AES vs. Rijndael means. While that's valuable knowledge, it distracts from checking whether the code makes sense, or potentially has problems. And since everyone calls it AES it makes sense to stick to that naming. \$\endgroup\$ – ferada Oct 21 at 20:53

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.