AES-GCM and AES-CBC-HMAC encryption methods

In general

First let's address the Elephant in the room. Neither function clearly explains what it does. Both are doing too much, deriving both the key and performing the encryption, clearing the password etc..

If these functions implement a specific protocol then that protocol should be named and referenced. Furthermore, you'd expect things like versioning for a protocol. Imagine being another dev and trying to find out what these functions do, why and if they are secure.

I'm usually the poor sod that has to do that kind of analysis, and without a protocol description it's often a complete non-starter for me. Is it secure? I don't know, what is it supposed to do? What should it protect against?

Security

The use of Argon2

• Iterations seems to be some kind of property. That's not bad in any way, but as the returned ciphertext doesn't store the number of Iterations nor the salt it remains to be seen if this is handled well. If the number of iterations is not stored then it may be impossible to up the number of iterations later on (they could also be linked to a version number of the protocol).
• Currently there is no way to tell if the returned ciphertext or password is incorrect. That cannot be entirely avoided, but you could e.g. use a new part of the output of Argon2 to create a password validation code and put it in front of the ciphertext.

The use of GCM

• The nonce of GCM is not randomly generated but it needs to be set. Now GCM works best on a nonce of 12 bytes, but as the user doesn't even know that GCM is used they'll just have to guess that.
• GCM has some nasty properties:
  • the nonce is relatively small which may lead to collisions if many plaintext are processed;
  • similarly, it can only handle data up to a certain size (64GiB isn't that much anymore);
  • it is not key confirming;
• when both the salt and IV are set to the same value for different messages it breaks catastrophically, leaking both the MAC subkey and a large amount of information about the plaintext. If you're going to include a random IV in the ciphertext I'd generate it in the encryption method to avoid this issue - same for the HMAC code.

The use of HMAC

• SHA3 is not that more secure than SHA2 and in general SHA-256 is considered secure enough for HMAC. Most processors are able to accelerate SHA-256 and of course that may also speed up HMAC calculation.
• When using SHA3 it is more efficient and future proof to use KMAC, I'm not entirely sure why Microsoft went for HMAC here. They should know better.
• (Deprecated) Decryption / HMAC verification isn't shown, which is a shame because the HMAC comparison should be time constant otherwise information may be leaked.
• You forgot to clear the HMAC key; it is at least as important as the encryption key in most scenarios.
• Congratulations, you didn't forget about including the IV in the HMAC calculation.

Efficiency

• As it is it is impossible to reuse the calculated key, for instance with a separate nonce. That means that the Argon2 calculations need to be performed for each separate encryption, even with the same password.
• Everything is single shot, and memory copies are made even if they are not required. For instance, it is perfectly possible to calculate a HMAC without putting everything in a complete array using streams or using the TransformBlock + TransformFinalBlock methods. Also, it is possible to encrypt / decrypt to streams, or to encrypt to a specific part in an existing byte array.

Coding

• Clearing the password without telling the user is entirely against the principle of least surprise. It's fine to tell the user to clear the password if it is not required anymore, but as it currently is there is a terrible risk of the user using an zeroed out password in the next call. That said, as the key is only used internally, it's perfectly fine to clear it and it is thoughtful that you did.
• Never ever return empty or null, especially not during decryption. Leave the exception handling to the user and think about when or why the exceptions are thrown in the first place. An empty message may well be a valid message after all. You can return a more specific exception such as DecryptionException if you really want to easy error handling.
• The guard statements testing for null are a nice touch, but catching those is really against all common sense. These problems occur because of programmer error and can be caught during testing, never to appear in a release version.
• The code is a bit of a hodge-podge within a too generic try / catch. The creation and use of Argon2, the cipher and the HMAC should be performed in this simple order, instead of being interleaved.
• The code is nice enough to read, well spaced and the naming of the identifiers used within the functions is fine as well.

Here’s what I’ve come up with for AES-CBC-HMAC. For now this is what I’ve chosen.. still open to feedback.

//the const values for argon2id and hmac private const int Iterations = 64; private const double MemorySize = 1024d * 1024d * 10d; public const int SaltSize = 512 / 8; public static readonly int ByteSize = 24; public static readonly int KeySize = 32; public static readonly int IvBit = 128; private const int TagLen = 16; private const int HmacTagLength = 64; private const int NonceSize = 12; private static readonly RandomNumberGenerator RndNum = RandomNumberGenerator.Create(); private const int BlockBitSize = 128; private const int KeyBitSize = 256; // creates a random byte array that // is the size of the parameter “size” public static byte[] RndByteSized(int size) { var buffer = new byte[size]; RndNum.GetBytes(buffer); return buffer; } public static async Task<byte[]> EncryptFile(string userName, char[] passWord, string file) { // null checks if (userName == null || passWord == null || file == null) throw new ArgumentNullException(); // generate random iv var iv = RndByteSized(IvBit / 8); // the salt is stored as a .txt file // inside the user folder. This salt is // used for password hash comparison to // login, as well as other things var saltString = await File.ReadAllTextAsync(Authentication.GetUserSalt(userName)); var salt = DataConversionHelpers.Base64StringToByteArray(saltString); // reads the string to encrypt and converts // to a byte array var fileStr = await File.ReadAllTextAsync(file); var fileBytes = DataConversionHelpers.StringToByteArray(fileStr); // gets bytes of the password var passwordBytes = Encoding.UTF8.GetBytes(passWord); if (fileBytes == null || salt == null) return Array.Empty<byte>(); // Call encrypt method var encryptedFile = await EncryptAsync(fileBytes, passwordBytes, iv, salt); return encryptedFile; } public static async Task<byte[]> DecryptFile(string userName, char[] passWord, string file) { if (userName == null || passWord == null || file == null) throw new ArgumentNullException(); var saltString = await File.ReadAllTextAsync(Authentication.GetUserSalt(userName)); var salt = DataConversionHelpers.Base64StringToByteArray(saltString); var fileStr = await File.ReadAllTextAsync(file); var fileBytes = DataConversionHelpers.Base64StringToByteArray(fileStr); var passwordBytes = Encoding.UTF8.GetBytes(passWord); if (fileBytes == null || salt == null) return Array.Empty<byte>(); var encryptedFile = await DecryptAsync(fileBytes, passwordBytes, salt); return encryptedFile; } private static (byte[] cipherResult, byte[] iv) InitBuffer(byte[] cipherText) { var iv = new byte[IvBit / 8]; var cipherResult = new byte[cipherText.Length - iv.Length]; Buffer.BlockCopy(cipherText, 0, iv, 0, iv.Length); Buffer.BlockCopy(cipherText, iv.Length, cipherResult, 0, cipherResult.Length); return (cipherResult, iv); } public static async Task<byte[]> EncryptAsync(byte[]? plainText, byte[]? password, byte[]? iv, byte[]? salt) { try { // null and empty checks if (plainText == Array.Empty<byte>()) throw new ArgumentException(@"Value was empty or null.", nameof(plainText)); if (password == null) throw new ArgumentException(@"Value was empty or null.", nameof(password)); if (salt == null) throw new ArgumentException(@"Value was empty or null.", nameof(salt)); if (iv == null) throw new ArgumentException(@"Value was empty or null.", nameof(iv)); // create aes object using var aes = Aes.Create(); aes.BlockSize = BlockBitSize; aes.KeySize = KeyBitSize; aes.Mode = CipherMode.CBC; aes.Padding = PaddingMode.PKCS7; // create argon2id object using var argon2 = new Argon2id(password); argon2.Salt = salt; argon2.DegreeOfParallelism = Environment.ProcessorCount * 2; argon2.Iterations = Iterations; argon2.MemorySize = (int)MemorySize; // derive our encryption and decryption // key from the user password var key = await argon2.GetBytesAsync(KeySize); // derive 64 bytes hmac key from user password var hmacKey = await argon2.GetBytesAsync(HmacTagLength); // do encryption using copyto method byte[] cipherText; using (var encryptor = aes.CreateEncryptor(key, iv)) using (var memStream = new MemoryStream()) { await using (var cryptoStream = new CryptoStream(memStream, encryptor, CryptoStreamMode.Write)) { using (var cipherStream = new MemoryStream(plainText)) { cipherStream.CopyTo(cryptoStream, (int)cipherStream.Length); cipherStream.Flush(); cryptoStream.FlushFinalBlockAsync(); } } cipherText = memStream.ToArray(); } // clear key from memory after encryption // is finished Array.Clear(key, 0, key.Length); // create hmac object using var hmac = new HMACSHA512(hmacKey); // create byte array that is the length // of iv and ciphertext var prependItems = new byte[cipherText.Length + iv.Length]; // copy iv (prepend) to byte array Buffer.BlockCopy(iv, 0, prependItems, 0, iv.Length); // copy ciphertext to byte array Buffer.BlockCopy(cipherText, 0, prependItems, iv.Length, cipherText.Length); // compute hash of iv and ciphertext var tag = hmac.ComputeHash(prependItems); // create new byte arrays var authenticatedBuffer = prependItems.Length + tag.Length; var authenticatedBytes = new byte[authenticatedBuffer]; // copy our created ciphertext and // append tag to the end Buffer.BlockCopy(prependItems, 0, authenticatedBytes, 0, prependItems.Length); Buffer.BlockCopy(tag, 0, authenticatedBytes, prependItems.Length, tag.Length); // clear hmac key Array.Clear(hmacKey, 0, hmacKey.Length); return authenticatedBytes; } catch (CryptographicException ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } catch (ArgumentNullException ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } catch (ObjectDisposedException ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } catch (Exception ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } } public static async Task<byte[]> DecryptAsync(byte[]? cipherText, byte[]? password, byte[]? salt) { try { if (cipherText == null) throw new ArgumentException(@"Value was empty or null.", nameof(cipherText)); if (password == null) throw new ArgumentException(@"Value was empty or null.", nameof(password)); if (salt == null) throw new ArgumentException(@"Value was empty or null.", nameof(salt)); using var aes = Aes.Create(); aes.BlockSize = BlockBitSize; aes.KeySize = KeyBitSize; aes.Mode = CipherMode.CBC; aes.Padding = PaddingMode.PKCS7; using var argon2 = new Argon2id(password); argon2.Salt = salt; argon2.DegreeOfParallelism = Environment.ProcessorCount * 2; argon2.Iterations = Iterations; argon2.MemorySize = (int)MemorySize; var key = await argon2.GetBytesAsync(KeySize); var hmacKey = await argon2.GetBytesAsync(HmacTagLength); using var hmac = new HMACSHA512(hmacKey); var receivedHash = new byte[HmacTagLength]; Buffer.BlockCopy(cipherText, cipherText.Length - HmacTagLength, receivedHash, 0, HmacTagLength); var cipherWithIv = new byte[cipherText.Length - HmacTagLength]; Buffer.BlockCopy(cipherText, 0, cipherWithIv, 0, cipherText.Length - HmacTagLength); var hashedInput = hmac.ComputeHash(cipherWithIv); var isMatch = CryptographicOperations.FixedTimeEquals(receivedHash, hashedInput); if (!isMatch) throw new CryptographicException("Invalid tag."); var (cipherResult, iv) = InitBuffer(cipherWithIv); using var decryptor = aes.CreateDecryptor(key, iv); using var memStream = new MemoryStream(); await using (var decryptStream = new CryptoStream(memStream, decryptor, CryptoStreamMode.Write)) { using (var plainStream = new MemoryStream(cipherResult)) { plainStream.CopyTo(decryptStream, (int)plainStream.Length); plainStream.Flush(); await decryptStream.FlushFinalBlockAsync(); } } Array.Clear(key, 0, key.Length); return memStream.ToArray(); } catch (CryptographicException ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } catch (ArgumentNullException ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } catch (ObjectDisposedException ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } catch (Exception ex) { Array.Clear(password, 0, password.Length); ErrorLogging.ErrorLog(ex); return Array.Empty<byte>(); } #pragma warning restore }