1
\$\begingroup\$

I need to store sensitive strings in DB, so decided to encrypt it in a DB and decrypt it at the application layer. And noticed that it is not so easy to find ready to use an example. Please check my code below. It is based on this damienbod's topic, but I included IV to the string itself and also want to know if it is production-ready. And a somewhat unrelated question: probably I should use an old Rijndael solution like this?

public class SymmetricEncryptDecrypt
{
    private const int ivBytes = 128;

    public (string Key, string IVBase64) InitSymmetricEncryptionKeyIV()
    {
        var key = GetEncodedRandomString(32); // 256
        using (Aes cipher = CreateCipher(key))
        {
            cipher.GenerateIV();
            var IVBase64 = Convert.ToBase64String(cipher.IV);
            return (key, IVBase64);
        }
    }

    private byte[] GetNewIv()
    {
        using (Aes cipher = CreateCipher(GetEncodedRandomString(32)))
        {
            cipher.GenerateIV();
            return cipher.IV;
        }
    }

    /// <summary>
    /// Encrypt using AES
    /// </summary>
    /// <param name="text">any text</param>
    /// <param name="IV">Base64 IV string/param>
    /// <param name="key">Base64 key</param>
    /// <returns>Returns an encrypted string</returns>
    public string Encrypt(string text, string key)
    {
        var iv = this.GetNewIv();

        using (Aes cipher = CreateCipher(key))
        {
            cipher.IV = iv;

            ICryptoTransform cryptTransform = cipher.CreateEncryptor();
            byte[] plaintext = Encoding.UTF8.GetBytes(text);
            byte[] cipherText = cryptTransform.TransformFinalBlock(plaintext, 0, plaintext.Length);

            return Convert.ToBase64String(iv.Concat(cipherText).ToArray());
        }
    }

    /// <summary>
    /// Decrypt using AES
    /// </summary>
    /// <param name="text">Base64 string for an AES encryption</param>
    /// <param name="key">Base64 key</param>
    /// <returns>Returns a string</returns>
    public string Decrypt(string encryptedText, string key)
    {
        var cipherTextBytesWithSaltAndIv = Convert.FromBase64String(encryptedText);

        var ivStringBytes = cipherTextBytesWithSaltAndIv.Take(ivBytes / 8).ToArray();
        // Get the actual cipher text bytes by removing the first 64 bytes from the cipherText string.
        var cipherTextBytes = cipherTextBytesWithSaltAndIv.Skip(ivBytes / 8).Take(cipherTextBytesWithSaltAndIv.Length - (ivBytes / 8)).ToArray();

        using (Aes cipher = CreateCipher(key))
        {
            cipher.IV = ivStringBytes;

            ICryptoTransform cryptTransform = cipher.CreateDecryptor();
            byte[] plainBytes = cryptTransform.TransformFinalBlock(cipherTextBytes, 0, cipherTextBytes.Length);

            return Encoding.UTF8.GetString(plainBytes);
        }
    }

    private string GetEncodedRandomString(int length)
    {
        var base64 = Convert.ToBase64String(GenerateRandomBytes(length));
        return base64;
    }

    /// <summary>
    /// Create an AES Cipher using a base64 key
    /// </summary>
    /// <param name="key"></param>
    /// <returns>AES</returns>
    private Aes CreateCipher(string keyBase64)
    {
        // Default values: Keysize 256, Padding PKC27
        Aes cipher = Aes.Create();
        cipher.Mode = CipherMode.CBC; // Ensure the integrity of the ciphertext if using CBC
        cipher.Padding = PaddingMode.ISO10126;
        cipher.Key = Convert.FromBase64String(keyBase64);
        return cipher;
    }

    private byte[] GenerateRandomBytes(int length)
    {
        var byteArray = new byte[length];
        RandomNumberGenerator.Fill(byteArray);
        return byteArray;
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ What .NET version? 3.1? \$\endgroup\$ – aepot Mar 30 at 21:42
  • 1
    \$\begingroup\$ @aepot, yes .NET 3+ \$\endgroup\$ – Ssss Mar 30 at 21:51
  • \$\begingroup\$ You're documenting PKCS#7 compatible padding, and then use a slightly different ISO 10126 padding mode, which is by-and-large deprecated. \$\endgroup\$ – Maarten Bodewes Apr 10 at 13:53
2
\$\begingroup\$

This code looks complicated for me. Not because I don't know how to use AES encrypter but it contains a lot of redundancy. From setting CipherMode.CBC which is default to GetNewIv() which is random by default.

Is a lot of code is targeting the a security improvement? If it is, you have a breach storing key in a string. I can easily get the key from App's memory even if it currently not in use because string is immutable and can be stored in memory for undefined period of time. Storing key in byte[] array allows to cleanup the array at any moment.

If security on DB side is enough then you may simplify the code as following two methods.

I use streams because it more friendly for me. As a bonus the code can be easily ported to streams use.

private static string Encrypt(string text, byte[] key)
{
    using AesManaged aes = new AesManaged() { Key = key };
    using MemoryStream ms = new MemoryStream();
    ms.Write(aes.IV);
    using (CryptoStream cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write, true))
    {
        cs.Write(Encoding.UTF8.GetBytes(text));
    }
    return Convert.ToBase64String(ms.ToArray());
}

private static string Decrypt(string base64, byte[] key)
{
    using MemoryStream ms = new MemoryStream(Convert.FromBase64String(base64));
    byte[] iv = new byte[16];
    ms.Read(iv);
    using AesManaged aes = new AesManaged() { Key = key, IV = iv };
    using CryptoStream cs = new CryptoStream(ms, aes.CreateDecryptor(), CryptoStreamMode.Read, true);
    using MemoryStream output = new MemoryStream();
    cs.CopyTo(output);
    return Encoding.UTF8.GetString(output.ToArray());
}

Demo

string text = "Hello world";
byte[] key = Enumerable.Range(0, 32).Select(x => (byte)x).ToArray(); // just for example :)
string base64 = Encrypt(text, key);
Console.WriteLine(base64);
Console.WriteLine(Decrypt(base64, key));

Output

Qh+XfnIuIdgllOiKFgzCpTURW+bUuj91S91zA1przRQ=
Hello world

I think that's enough to keep it secure on DB side. Btw, you may keep the Key generator methods if you need it.

P.S. old Rijndael solution - Rijndael was superseded by Aes in .NET, i don't remember the exact reason but something like Rijndael has a padding issue and Aes fixed it.


One more tip. If you're on Windows and the encrypted data is allowed to be lost (in case of emergency), you can use DPAPI to protect the key (MS NuGet package exists, Current User/Local Machine protecting modes available) and store protected key in DB in the same sequence as IV. Then you can use totally random key for each Encrypt() call and store it with data. The data may be lost on Windows reinstall or moving the ASP.NET server to other machine, because DPAPI uses key associated with Current User credentials or Local Machine ID. But protected data can't be restored if DB was stolen or in any other way on other PC. Anyway learn how DPAPI works, it may be useful if the server runs under Windows.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Personally I don't like default values at all. They are largely hidden in the API documentation; I'd much rather use explicit values. The key generator doesn't seem to be secure, so I would not keep it (or do you mean the key generator in the original code?). You may want to look at the padding remark I made below the question, it's not PKCS#7. \$\endgroup\$ – Maarten Bodewes Apr 10 at 13:58
  • \$\begingroup\$ @MaartenBodewes yes, I mean key generator in the original code. Shoul I remove Padding = PaddingMode.ISO10126? I'm not sure about it. \$\endgroup\$ – aepot Apr 10 at 14:59
  • 1
    \$\begingroup\$ Well, you copied it from the question and a remark has been made now, so I guess it is clear as it is. However, I would consider PKCS#7 better supported so I would use it just to avoid interesting errors during decryption on another platform. \$\endgroup\$ – Maarten Bodewes Apr 10 at 15:10
2
\$\begingroup\$

There is already a security review and a hint on how to better code the class, but I would like to point out the issues with the coding quality and documentation.

Initialization

private const int ivBytes = 128;

Constants should be in all caps, and an IV for AES-CBC is 16 bytes / 128 bits, not 128 bytes.

public (string Key, string IVBase64) InitSymmetricEncryptionKeyIV()

The parameters should not be capitalized. As it is already in a class called SymmetricEncryptDecrypt you can loose the SymmetricEncryption part.

var key = GetEncodedRandomString(32); // 256

Encoded as what? What does 256 even mean here? And a key is not a string, it's a byte array.

cipher.GenerateIV();

This call is repeated twice, which is generally considered code smell if it is only needed once (but kudos for using a method for it).

Encryption

/// Encrypt using AES

AES in which mode? Shouldn't you specify the padding mode either? And the fact that you prefix the IV? And the fact that you use UTF-8?

/// <returns>Returns an encrypted string</returns>

But here the encoding is not specified (again).

return Convert.ToBase64String(iv.Concat(cipherText).ToArray());

Decryption

One-liners like this just make reading and debugging harder. Split into 3 lines.

var cipherTextBytesWithSaltAndIv = Convert.FromBase64String(encryptedText);

Oh, right, where is the salt in the encryption phase?

    var ivStringBytes = cipherTextBytesWithSaltAndIv.Take(ivBytes / 8).ToArray();

Where is the string? Do you mean octet string?

The rest

var base64 = Convert.ToBase64String(GenerateRandomBytes(length));
return base64;

I'd think that var randomBytes = GenerateRandomBytes(length) followed by return Convert.ToBase64String(randomBytes) is acceptable: don't perform two functions in one line unless it helps readability.

// Default values: Keysize 256, Padding PKC27

The padding mode is called PKCS#7 compatible padding, and you're not using it.

 cipher.Mode = CipherMode.CBC; // Ensure the integrity of the ciphertext if using CBC

That's a good hint, but #1: you're not doing it and #2 the comment should be in the documentation of the class if you're not doing it yourself, not in an end-of-line comment.

\$\endgroup\$
3
  • \$\begingroup\$ thanks for the answer. Could you please elaborate on this: "where is the salt in the encryption phase?"? \$\endgroup\$ – Ssss Apr 14 at 8:10
  • \$\begingroup\$ Are you speaking about variable names only? Yes, it is a "copy-paste" issue. Or do you mean that we need salt? \$\endgroup\$ – Ssss Apr 14 at 8:17
  • 1
    \$\begingroup\$ cipherTextBytesWithSaltAndIv is in your decryption method, but no mention is there in the encryption method of a salt. So I don't know - you tell me. Maybe that part of the code is missing, but in that case your program isn't very symmetric w.r.t. method & input / output parameter definitions. You'd only really need a salt if you use password based encryption (rather than using a fully randomized key). That kind of copy / paste error should not be acceptable anywhere, don't wrong foot your colleagues (including a future you). \$\endgroup\$ – Maarten Bodewes Apr 14 at 9:05

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.