Manual authenticated encryption on .NET Core

Question

I've been tasked with creating some way to encrypt arbitrary text for a .NET Core app, with the following requirements:

• Messages will be between 10 bytes and 1 kB.
• Boss doesn't want me to use AES-GCM (which I know would be the sensible option as it's already included in .NET Core and I've already read that implementing your own encryption is a bad idea) and wants me to come up with a custom implementation.
• This implementation will not be used too much, usually only at the app's startup.

After some reading, I've come up with the following:

Encryption:

1. Convert message to UTF-8 bytes (plaintext)
2. Generate a random salt.
3. Using salt, derive enough material with PBKDF2 for an aes_key and an hmac_key.
4. Generate a random iv.
5. AES-CBC encrypt plaintext with aes_key and iv (ciphertext).
6. Compute an HMAC-SHA-256 hash of ciphertext||iv.
7. Return salt||hash||iv||ciphertext (payload).

Decryption:

1. Extract salt, hash, iv, ciphertext from payload.
2. Using salt, derive enough material with PBKDF2 for an aes_key and an hmac_key.
3. Compute an HMAC-SHA-256 computed_hash of ciphertext||iv.
4. If computed_hash and hash are different, reject payload.
5. AES-CBC decrypt ciphertext with aes_key and iv (plaintext).
6. Convert plaintext to UTF-8 string (message).
7. Return message.

The implementation is as follows:

public class AuthenticatedEncryptionHelper
{
 private const int _SALT_LENGTH = 16;
 private const int _KEY_DERIVATION_ITERATIONS = 100000;
 private const int _CRYPTO_KEY_LENGTH = 16;
 private const int _HMAC_KEY_LENGTH = 16;
 private const int _IV_LENGTH = 16;
 public byte[] Encrypt(string message, string password)
 {
 var salt = GenerateRandomBytes(_SALT_LENGTH);
 var (enc_key, hmac_key) = DeriveKeys(password, salt);
 var iv = GenerateRandomBytes(_IV_LENGTH);
 var plaintext = Encoding.UTF8.GetBytes(message);
 var ciphertext = AesEncrypt(plaintext, enc_key, iv);
 var hmac = ComputeHmac(ciphertext, iv, hmac_key);
 var payload = GeneratePayload(salt, hmac, iv, ciphertext);
 return payload;
 }
 public string Decrypt(byte[] payload, string password)
 {
 var (salt, hmac, iv, ciphertext) = ExplodePayload(payload);
 var (dec_key, hmac_key) = DeriveKeys(password, salt);
 var computed_hmac = ComputeHmac(ciphertext, iv, hmac_key);
 if (!AreHmacsEqual(hmac, computed_hmac))
 {
 throw new Exception();
 }
 var plaintext = AesDecrypt(ciphertext, dec_key, iv);
 var message = Encoding.UTF8.GetString(plaintext);
 return message;
 }
 private byte[] GenerateRandomBytes(int length)
 {
 var result = new byte[length];
 using (var r = RandomNumberGenerator.Create())
 {
 r.GetBytes(result);
 }
 return result;
 }
 private (byte[], byte[]) DeriveKeys(string password, byte[] salt)
 {
 var length = _CRYPTO_KEY_LENGTH + _HMAC_KEY_LENGTH;
 var result = new byte[length];
 using (var pbkdf2 = new Rfc2898DeriveBytes(password, salt, _KEY_DERIVATION_ITERATIONS))
 {
 result = pbkdf2.GetBytes(length);
 }
 var r1 = new byte[_CRYPTO_KEY_LENGTH];
 var r2 = new byte[_HMAC_KEY_LENGTH];
 Buffer.BlockCopy(result, 0, r1, 0, _CRYPTO_KEY_LENGTH);
 Buffer.BlockCopy(result, _CRYPTO_KEY_LENGTH, r2, 0, _HMAC_KEY_LENGTH);
 return (r1, r2);
 }
 private byte[] AesEncrypt(byte[] plaintext, byte[] key, byte[] iv)
 {
 byte[] ciphertext;
 using (var aes = Aes.Create())
 {
 aes.Mode = CipherMode.CBC;
 aes.Padding = PaddingMode.PKCS7;
 aes.Key = key;
 aes.IV = iv;
 using (var encryptor = aes.CreateEncryptor())
 {
 ciphertext = encryptor.TransformFinalBlock(plaintext, 0, plaintext.Length);
 }
 }
 return ciphertext;
 }
 private byte[] ComputeHmac(byte[] ciphertext, byte[] iv, byte[] key)
 {
 var input = new byte[iv.Length + ciphertext.Length];
 Buffer.BlockCopy(iv, 0, input, 0, iv.Length);
 Buffer.BlockCopy(ciphertext, 0, input, iv.Length, ciphertext.Length);
 byte[] result;
 using (var hash = new HMACSHA256(key))
 {
 result = hash.ComputeHash(input);
 }
 return result;
 }
 private byte[] GeneratePayload(byte[] salt, byte[] hmac, byte[] iv, byte[] ciphertext)
 {
 var result = new byte[salt.Length + hmac.Length + iv.Length + ciphertext.Length];
 var curr_index = 0;
 Buffer.BlockCopy(salt, 0, result, curr_index, salt.Length);
 curr_index += salt.Length;
 Buffer.BlockCopy(hmac, 0, result, curr_index, hmac.Length);
 curr_index += hmac.Length;
 Buffer.BlockCopy(iv, 0, result, curr_index, iv.Length);
 curr_index += iv.Length;
 Buffer.BlockCopy(ciphertext, 0, result, curr_index, ciphertext.Length);
 return result;
 }
 private (byte[], byte[], byte[], byte[]) ExplodePayload(byte[] payload)
 {
 var salt = new byte[_SALT_LENGTH];
 var hmac = new byte[32];
 var iv = new byte[_IV_LENGTH];
 var ciphertext = new byte[payload.Length - (_SALT_LENGTH + 32 + _IV_LENGTH)];
 var curr_index = 0;
 Buffer.BlockCopy(payload, curr_index, salt, 0, _SALT_LENGTH);
 curr_index += _SALT_LENGTH;
 Buffer.BlockCopy(payload, curr_index, hmac, 0, 32);
 curr_index += 32;
 Buffer.BlockCopy(payload, curr_index, iv, 0, _IV_LENGTH);
 curr_index += _IV_LENGTH;
 Buffer.BlockCopy(payload, curr_index, ciphertext, 0, ciphertext.Length);
 return (salt, hmac, iv, ciphertext);
 }
 private bool AreHmacsEqual(byte[] hmac, byte[] computedHmac)
 {
 var result = true;
 if (hmac.Length != computedHmac.Length)
 {
 result = false;
 }
 for (int i = 0; i < hmac.Length; i++)
 {
 if (hmac[i] != computedHmac[i])
 {
 result = false;
 }
 }
 return result;
 }
 private byte[] AesDecrypt(byte[] ciphertext, byte[] key, byte[] iv)
 {
 byte[] plaintext;
 using (var aes = Aes.Create())
 {
 aes.Mode = CipherMode.CBC;
 aes.Padding = PaddingMode.PKCS7;
 aes.Key = key;
 aes.IV = iv;
 using (var decryptor = aes.CreateDecryptor())
 {
 plaintext = decryptor.TransformFinalBlock(ciphertext, 0, ciphertext.Length);
 }
 }
 return plaintext;
 }
}

Any criticism of this code (or the algorithm) will be greatly appreciated.

Answer 1

Cryptography

Yes, CBC followed by HMAC is fine (also known as encrypt-then-MAC), and you include the IV into the calculations, which is great. You indeed do not have to include the salt and iteration count into the HMAC calculation as they already influence the key being used. You may need to review this choice if you switch to GCM though - including more in the validated parameters never hurts.

---

You are using a cryptographically secure random number generator, so kudos there.

---

Beware that password based encryption still very much depends on the password strength. A password of 40 bit strength won't get anywhere near 128 bit strength, even with a key stretching algorithm such as PBKDF2.

---

You are using the terribly named Rfc2898DeriveBytes class, which implements PBKDF2. This is not top of class algorithm (see e.g. Argon2) but it may suffice. This is presuming that you require a password in the first place of course; if you can perform encryption without requiring passwords then that's always better.

---

If you don't call the method more than once, then 100_000 iterations may be on the low side. Especially for weak passwords: the higher the value the better. You may want to make this value dynamic so that you can choose a higher value when time progresses.

---

16 bytes of secure random salt is precisely on the mark.

---

One ugly problem with PBKDF2 / Rfc2898DeriveBytes is that it performs all the iterations again for each hash size required, standardizing on SHA-1 / 20 bytes. You are in this case better off specifying SHA-256 or SHA-512, otherwise you are having to perform more iterations than a possible attacker (who has enough with 1 key).

---

HMAC-SHA-256 kinda defaults to a 256 bit key. Using 16 bytes / 128 bits is not bad in any sense, but there's that (this is also why specifying SHA-512 for PBKDF2 may be beneficial).

---

You are using a (presumably) time-constant compare for the HMAC tag, nice, that would be one of the major issues when people just look at the methods, as Microsoft never indicates that you should use a time constant compare.

Code

See the question / answer at StackOverflow sister site about naming conventions, especially regarding constant values: those should not start with an underscore.

---

The block copy after PBKDF2 / function is not needed:

The Rfc2898DeriveBytes class implements PBKDF2 functionality by using a pseudorandom number generator based on HMACSHA1. The Rfc2898DeriveBytes class takes a password, a salt, and an iteration count, and then generates keys through calls to the GetBytes method. Repeated calls to this method will not generate the same key; instead, appending two calls of the GetBytes method with a cb parameter value of 20 is the equivalent of calling the GetBytes method once with a cb parameter value of 40.

---

Later versions of C# allow for underscores in literals. 100_000 is a more readable version of 100000.

---

aes.Padding = PaddingMode.PKCS7;

Although this is the default, I applaude you for setting it anyway; not all readers may know it is the default - crypto experts are not always also language/runtime specialists.

Design

You could use a version indicator before your ciphertext. In that case you can upgrade your algorithms and or iteration count when required (your boss may be corrected by a subject expert).

---

Having small messages kind of obsoletes the need for streaming, but personally I'd still use streaming, especially since the .NET API seems to have been build around those. In this case all the byte arrays and whatnot won't matter much though.

---

 curr_index += 32;

Use a constant here, e.g. HMACSHA256_OUTPUT_SIZE = 32 or you'll be in trouble if somebody decides on using a different hash.