2
\$\begingroup\$

Looking for code reviews of these two encryption methods. And if GCM is better or worse than CBC with HMAC.

AES-GCM

public static async Task<byte[]> EncryptAsyncV2(byte[] plainText, byte[] password, byte[] nonce, byte[] salt)
    {
        try
        {
            if (plainText == null)
                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 (nonce == null)
                throw new ArgumentException(@"Value was empty or null.", nameof(nonce));

            var cipherText = new byte[plainText.Length];
            var tag = new byte[TagLen];

            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);

                using var aesGcm = new AesGcm(key, TagLen);
                aesGcm.Encrypt(nonce, plainText, cipherText, tag);
                Array.Clear(key, 0, key.Length);
            }

            cipherText = tag.Concat(nonce.Concat(cipherText)).ToArray();
            return cipherText;
        }
        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 (Exception ex)
        {
            Array.Clear(password!, 0, password!.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
    }

    public static async Task<byte[]> DecryptAsyncV2(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 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);

            using var aesGcm = new AesGcm(key, TagLen);
            var tag = new byte[TagLen];
            var nonce = new byte[NonceSize];
            var cipherResult = new byte[cipherText.Length - nonce.Length - tag.Length];

            Buffer.BlockCopy(cipherText, 0, tag, 0, tag.Length);
            Buffer.BlockCopy(cipherText, tag.Length, nonce, 0, nonce.Length);
            Buffer.BlockCopy(cipherText, tag.Length + nonce.Length, cipherResult, 0, cipherResult.Length);

            var plainText = new byte[cipherResult.Length];

            aesGcm.Decrypt(nonce, cipherResult, tag, plainText);

            Array.Clear(key, 0, key.Length);

            return plainText;
        }
        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 (Exception ex)
        {
            Array.Clear(password!, 0, password!.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
#pragma warning restore
    }

The nonce is generated with the RandomNumberGenerator class using this method

public static byte[] RndByteSized(int size)
    {
        var buffer = new byte[size];
        RndNum.GetBytes(buffer);
        return buffer;
    }

AES-CBC with HMAC

public static async Task<byte[]?> EncryptAsync(byte[]? plainText, byte[]? password, byte[]? iv, byte[]? salt)
 {
     try
     {
         if (plainText == null)
             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));

         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(64);

         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();
         }

         Array.Clear(key, 0, key.Length);

         var tag = new byte[64];
         using (var hmac = new HMACSHA3_512(hmacKey))
         {
             var prependItems = new byte[cipherText.Length + iv.Length];
             Buffer.BlockCopy(iv, 0, prependItems, 0, iv.Length);
             Buffer.BlockCopy(cipherText, 0, prependItems, iv.Length, cipherText.Length);
             tag = hmac.ComputeHash(prependItems);
             var authenticatedBuffer = prependItems.Length + tag.Length;
             var authenticatedBytes = new byte[authenticatedBuffer];
             Buffer.BlockCopy(prependItems, 0, authenticatedBytes, 0, prependItems.Length);
             Buffer.BlockCopy(tag, 0, authenticatedBytes, prependItems.Length, tag.Length);
             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 (Exception ex)
     {
         Array.Clear(password, 0, password.Length);
         ErrorLogging.ErrorLog(ex);
         return Array.Empty<byte>();
     }
 }
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Tell us about your use case. How do you exercise this code, what business goals does it address, what is your threat model, what are pain points or things you're especially concerned about. In this context what does it mean for one encryption scheme to be "better" than another? Some hosts have dozens of cores and air conditioning. Others are battery operated. Life is full of tradeoffs. You haven't told us how you make them. \$\endgroup\$
    – J_H
    Commented Oct 30, 2023 at 21:19
  • \$\begingroup\$ GCM worries me slightly due to the 96 bit nonce which has a higher risk of collision than the 128 bit IV with CBC. I’m creating a password vault as a personal project, and am just looking for the most solid encryption scheme. I’ve also considered xchacha20-poly1305 \$\endgroup\$ Commented Oct 30, 2023 at 21:22
  • \$\begingroup\$ Just looking for the one with the least amount of trade offs. \$\endgroup\$ Commented Oct 30, 2023 at 21:24
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers. Doing so goes against the Question + Answer style of Code Review, as it unfortunately invalidates the existing review(s). This is not a forum where you should keep the most updated version in your question, so I rolled your changes back to the previous version. Please see see what you may and may not do after receiving answers for ways to announce your new code. \$\endgroup\$
    – Martin R
    Commented Oct 31, 2023 at 7:45
  • \$\begingroup\$ Understandable. Thanks \$\endgroup\$ Commented Oct 31, 2023 at 9:16

2 Answers 2

3
\$\begingroup\$

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.
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for the feedback. I updated the cbc-hmac code with all the variables and values, and added the decrypt function. This works fine for me, when calling encrypt / decrypt multiple times. Could you look back over it? Thanks! \$\endgroup\$ Commented Oct 31, 2023 at 3:52
  • \$\begingroup\$ It does work and do a time constant compare, so I guess it works fine that way. Otherwise the same remarks as for the rest of the code, and yeah, definitely don't shuffle the exceptions under the table for security relevant code. \$\endgroup\$ Commented Oct 31, 2023 at 3:55
  • \$\begingroup\$ What’s the best way of handling exceptions for this kind of code? \$\endgroup\$ Commented Oct 31, 2023 at 3:56
  • \$\begingroup\$ Split into three: programming errors (missing implementations of ciphers for instance), input errors such as password errors and errors validating the ciphertext. The first two can be "runtime" errors, but the user needs to be clear that they should handle the latter themselves- possibly at a higher level. That's the nice thing about exceptions: you can just leave them alone until you are ready to handle them. \$\endgroup\$ Commented Oct 31, 2023 at 3:59
1
\$\begingroup\$

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
    }
\$\endgroup\$
1
  • \$\begingroup\$ Rather than update the question, or post an answer for your own question, you can post a follow up question with a link back to your original question. \$\endgroup\$
    – pacmaninbw
    Commented Oct 31, 2023 at 11:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.