Implementation of aes-cbc-hmac combined with xchacha20-poly1305

Question

I’m using two ciphers in this code that are in this order- encrypt plain text with xchacha20-poly1305 then encrypt the resulting ciphertext with aes-cbc-blake2b hmac.

I was told elsewhere rather than derive 2 keys from one key that’s derived from a password, it would be better to derive all the bytes from the password and split them into 3 keys.

I was also wondering if having two ciphers with different keys adds a layer of security or not.

Is this a safe and secure implementation of aes-cbc-hmac combined with xchacha20-poly1305?

Code:

private const int Iterations = 32; // 32 iterations in release build
    private const double MemorySize = 1024d * 1024d * 5d; // 5gib in release build
    public const int SaltSize = 512 / 8;
    public static readonly int KeySize = 32;
    public static readonly int IvBit = 128;
    private const int HmacLength = 64;
    private const int ChaChaNonceSize = 24;

    private static readonly RandomNumberGenerator RndNum = RandomNumberGenerator.Create();

public static async Task<(byte[] Salt, byte[] Salt2, byte[] Salt3, byte[] Salt4)> GetUserSaltAsync(string userName)
    { 
        try
        {
            var salt = await File.ReadAllTextAsync(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), 
                "Password Vault", "Users", userName, $"{userName}.salt"));
            var salt2 = await File.ReadAllTextAsync(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
                "Password Vault", "Users", userName, $"{userName}-Salt2.salt"));
            var salt3 = await File.ReadAllTextAsync(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
                "Password Vault", "Users", userName, $"{userName}-Salt3.salt"));
            var salt4 = await File.ReadAllTextAsync(Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
                "Password Vault", "Users", userName, $"{userName}-Salt4.salt"));

            var saltResult = DataConversionHelpers.Base64StringToByteArray(salt);
            var saltResult2 = DataConversionHelpers.Base64StringToByteArray(salt2);
            var saltResult3 = DataConversionHelpers.Base64StringToByteArray(salt3);
            var saltResult4 = DataConversionHelpers.Base64StringToByteArray(salt4);

            return (saltResult, saltResult2, saltResult3, saltResult4);
        }
        catch (IOException ex)
        {
            MessageBox.Show(ex.Message, @"Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
            ErrorLogging.ErrorLog(ex);
            return (Array.Empty<byte>(), Array.Empty<byte>(), Array.Empty<byte>(), Array.Empty<byte>());
        }
    }

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)
    {
        try
        {
            if (userName == string.Empty || passWord == Array.Empty<char>() || file == string.Empty)
                throw new ArgumentException(@"Value was empty.", userName == string.Empty ? nameof(userName) :
                    passWord == Array.Empty<char>() ? nameof(passWord) : nameof(file));

            var saltResult = await Authentication.GetUserSaltAsync(userName);

            var salt = saltResult.Salt;
            var salt2 = saltResult.Salt2;
            var salt3 = saltResult.Salt3;

            var fileStr = await File.ReadAllTextAsync(file);
            var fileBytes = DataConversionHelpers.StringToByteArray(fileStr);

            var passwordBytes = Encoding.UTF8.GetBytes(passWord);

            if (fileBytes == Array.Empty<byte>() || salt == Array.Empty<byte>())
                throw new ArgumentException(@"Value was empty.",
                    fileBytes == Array.Empty<byte>() ? nameof(fileBytes) : nameof(salt));

            var encryptedFile = await EncryptAsyncV3(fileBytes, salt, salt2, salt3, passwordBytes);

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

            return encryptedFile;
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message, @"Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
    }

    public static async Task<byte[]> DecryptFile(string userName, char[] passWord, string file)
    {
        try
        {
            if (userName == string.Empty || passWord == Array.Empty<char>() || file == string.Empty)
                throw new ArgumentException(@"Value was empty.", userName == string.Empty ? nameof(userName) :
                    passWord == Array.Empty<char>() ? nameof(passWord) : nameof(file));

            var saltResult = await Authentication.GetUserSaltAsync(userName);

            var salt = saltResult.Salt;
            var salt2 = saltResult.Salt2;
            var salt3 = saltResult.Salt3;

            var fileStr = await File.ReadAllTextAsync(file);
            var fileBytes = DataConversionHelpers.Base64StringToByteArray(fileStr);

            var passwordBytes = Encoding.UTF8.GetBytes(passWord);

            if (fileBytes == Array.Empty<byte>() || salt == Array.Empty<byte>())
                throw new ArgumentException(@"Value was empty.",
                    fileBytes == Array.Empty<byte>() ? nameof(fileBytes) : nameof(salt));

            var decryptedFile = await DecryptAsyncV3(fileBytes, salt, salt2, salt3, passwordBytes);

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

            return decryptedFile;
        }
        catch (Exception ex)
        {
            MessageBox.Show(ex.Message, @"Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
    }

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


    private const int BlockBitSize = 128;
    private const int KeyBitSize = 256;
    public static async Task<byte[]> EncryptAsync(byte[] plainText, byte[] key, byte[] iv, byte[] salt)
    {
        if (plainText == Array.Empty<byte>())
            throw new ArgumentException(@"Value was empty or null.", nameof(plainText));
        if (key == Array.Empty<byte>())
            throw new ArgumentException(@"Value was empty or null.", nameof(key));
        if (salt == Array.Empty<byte>())
            throw new ArgumentException(@"Value was empty or null.", nameof(salt));

        using var argon2 = new Argon2id(key);
        argon2.Salt = salt;
        argon2.DegreeOfParallelism = Environment.ProcessorCount * 2;
        argon2.Iterations = Iterations;
        argon2.MemorySize = (int)MemorySize;

        var hmacKey = await argon2.GetBytesAsync(HmacLength);

        try
        {
            using var aes = Aes.Create();
            aes.BlockSize = BlockBitSize;
            aes.KeySize = KeyBitSize;
            aes.Mode = CipherMode.CBC;
            aes.Padding = PaddingMode.PKCS7;

            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))
                    {
                       await cipherStream.FlushAsync();
                       await cipherStream.CopyToAsync(cryptoStream, (int)cipherStream.Length);
                    }

                    await cryptoStream.FlushFinalBlockAsync();
                }

                cipherText = memStream.ToArray();
            }

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

            using var hmac = new HMACBlake2B(hmacKey, HmacLength * 8);
            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);

            var 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(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (ArgumentNullException ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (ObjectDisposedException ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (Exception ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
    }

    public static async Task<byte[]> DecryptAsync(byte[] cipherText, byte[] key, byte[] salt)
    {
        if (cipherText == Array.Empty<byte>())
            throw new ArgumentException(@"Value was empty or null.", nameof(cipherText));
        if (key == Array.Empty<byte>())
            throw new ArgumentException(@"Value was empty or null.", nameof(key));
        if (salt == Array.Empty<byte>())
            throw new ArgumentException(@"Value was empty or null.", nameof(salt));

        using var argon2 = new Argon2id(key);
        argon2.Salt = salt;
        argon2.DegreeOfParallelism = Environment.ProcessorCount * 2;
        argon2.Iterations = Iterations;
        argon2.MemorySize = (int)MemorySize;

        var hmacKey = await argon2.GetBytesAsync(HmacLength);

        try
        {
            using var aes = Aes.Create();
            aes.BlockSize = BlockBitSize;
            aes.KeySize = KeyBitSize;
            aes.Mode = CipherMode.CBC;
            aes.Padding = PaddingMode.PKCS7;

            using var hmac = new HMACBlake2B(hmacKey, HmacLength * 8);
            var receivedHash = new byte[HmacLength];

            Buffer.BlockCopy(cipherText, cipherText.Length - HmacLength, receivedHash, 0, HmacLength);

            var cipherWithIv = new byte[cipherText.Length - HmacLength];

            Buffer.BlockCopy(cipherText, 0, cipherWithIv, 0, cipherText.Length - HmacLength);

            var hashedInput = hmac.ComputeHash(cipherWithIv);

            var isMatch = CryptographicOperations.FixedTimeEquals(receivedHash, hashedInput);

            if (!isMatch)
                throw new CryptographicException();

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

            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))
                {
                    await plainStream.CopyToAsync(decryptStream, (int)plainStream.Length);
                    await plainStream.FlushAsync();
                }

                await decryptStream.FlushFinalBlockAsync();
            }

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

            return memStream.ToArray();
        }
        catch (CryptographicException ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (ArgumentNullException ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (ObjectDisposedException ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (Exception ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(hmacKey, 0, hmacKey.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
#pragma warning restore
    }

    public static async Task<byte[]> EncryptAsyncV3(byte[] plaintext, byte[] salt, byte[] salt2, byte[] salt3,
          byte[] password)
    {
        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 argon2L2 = new Argon2id(key);
        argon2L2.Salt = salt2;
        argon2L2.DegreeOfParallelism = Environment.ProcessorCount * 2;
        argon2L2.Iterations = Iterations;
        argon2L2.MemorySize = (int)MemorySize;

        var key2 = await argon2L2.GetBytesAsync(KeySize);

        try
        {
            if (plaintext == Array.Empty<byte>() || salt == Array.Empty<byte>() || salt2 == Array.Empty<byte>()
                || password == Array.Empty<byte>())
                throw new ArgumentException(@"Value was empty.",
                    plaintext == Array.Empty<byte>() ? nameof(plaintext) :
                    salt == Array.Empty<byte>() ? nameof(salt) :
                    salt2 == Array.Empty<byte>() ? nameof(salt2) :
                    nameof(password));

            var nonce = RndByteSized(ChaChaNonceSize);
            var nonce2 = RndByteSized(IvBit / 8);

            var cipherText = SecretAeadXChaCha20Poly1305.Encrypt(plaintext, nonce, key);
            var cipherTextL2 = await EncryptAsync(cipherText, key2, nonce2, salt3);

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

            return nonce.Concat(nonce2).Concat(cipherTextL2).ToArray();
        }
        catch (CryptographicException ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(key2, 0, key2.Length);
            Array.Clear(password, 0, password.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (Exception ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(key2, 0, key2.Length);
            Array.Clear(password, 0, password.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
    }

    public static async Task<byte[]> DecryptAsyncV3(byte[] cipherText, byte[] salt, byte[] salt2, byte[] salt3,
        byte[] password)
    {
        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 argon2L2 = new Argon2id(key);
        argon2L2.Salt = salt2;
        argon2L2.DegreeOfParallelism = Environment.ProcessorCount * 2;
        argon2L2.Iterations = Iterations;
        argon2L2.MemorySize = (int)MemorySize;

        var key2 = await argon2L2.GetBytesAsync(KeySize);

        try
        {
            if (cipherText == Array.Empty<byte>() || salt == Array.Empty<byte>() || salt2 == Array.Empty<byte>()
                || password == Array.Empty<byte>())
                throw new ArgumentException(@"Value was empty.",
                    cipherText == Array.Empty<byte>() ? nameof(cipherText) :
                    salt == Array.Empty<byte>() ? nameof(salt) :
                    salt2 == Array.Empty<byte>() ? nameof(salt2) :
                    nameof(password));

            var nonce = new byte[ChaChaNonceSize];
            Buffer.BlockCopy(cipherText, 0, nonce, 0, nonce.Length);

            var nonce2 = new byte[IvBit / 8];
            Buffer.BlockCopy(cipherText, nonce.Length, nonce2, 0, nonce2.Length);

            var cipherResult =
                new byte[cipherText.Length - nonce2.Length - nonce.Length];

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

            var resultL2 = await DecryptAsync(cipherResult, key2, salt3);
            var resultL0 = SecretAeadXChaCha20Poly1305.Decrypt(resultL2, nonce, key);


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

            return resultL0;
        }
        catch (CryptographicException ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(key2, 0, key2.Length);
            Array.Clear(password, 0, password.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
        catch (Exception ex)
        {
            Array.Clear(key, 0, key.Length);
            Array.Clear(key2, 0, key2.Length);
            Array.Clear(password, 0, password.Length);
            ErrorLogging.ErrorLog(ex);
            return Array.Empty<byte>();
        }
    }

The code has been tested extensively and works fine.

I’m wondering, what changes can I make to make it more readable, did i implement the use of two ciphers correctly, as well as would splitting the 3 keys from one argon2id be more efficient / secure. And if there’s anything else I can do to improve security. I’m basically trying to make this impossible to break.

Answer 1

threat environment

[I appended some material in this vein at end-of-answer.]

Apparently you're addressing a use case along these lines:

• Alice has sensitive material to store or transmit
• Mallory wants to read it from disk or network
• We're concerned that AES and/or ChaCha may have
• • theory bugs (e.g. like MD5 collisions or arc4 weaknesses)
• • implementation bugs (e.g. like Debian's infamous "predictable random" CVE-2008-0166)

The source code should explicitly describe the environment, perceived threats, and the mitigations implemented.

Given that you're key-stretching a low entropy human-remembered passphrase, it's not clear the combined key length of two different ciphers is a good match for the deployed environment.

We also need an overview of what an encrypted file looks like. There is some layering, and some added items like nonces and salts, and binary values represented in base64 form. Document the details.

---

version

There's a bunch of details. History tells us that the details of any given complex scheme are likely to change as time goes by.

Invent a new magic number. Prepend it, as cleartext, when you write out a file. Its final byte should be a version number. That way you can rev this software, and have it read old formats in a backward compatible way.

---

key management

Write down some advice to users on how they should manage key material. You owe it to them, since you have burdened them with securely storing symmetric cipher keys. For example, does key material belong on a separate filesystem? On a thumb drive? On a separate host? In a cloud credentials vault?

I am sad that this code does not appear to leverage GPG's public key crypto support, for example by having GPG encrypt a small binary file of keying material. If an individual or organization has already thought through the key management details for GPG, it would be natural for them to want to apply that when using the present code.

---

fractional bytes

    private const double MemorySize = 1024d * 1024d * 5d; // 5gib in release build

That's not a natural way to represent amount of RAM consumed. Recommend you change this to some kind of unsigned integer. In C we'd be looking for a size_t.

This awkward cast might have tipped you off:

        argon2.MemorySize = (int)MemorySize;

Thank you for the helpful comment. But it would have been better to define a manifest constant of GIGABYTE = 2 ** 20 or 1024 squared or whatever.

---

magic number

    public const int SaltSize = 512 / 8;

Well, it's good that we're naming this quantity, thank you. But it merits at least a comment about where those numbers came from. Which of the two crypto schemes wants that amount of salt?

---

plural identifier

    public static readonly int IvBit = 128;

It appears this should have been IvBits, since the IV is clearly more than a single bit.

I'm glad you're defining a bunch of numeric parameters all together. I wish it was more clear that they all have units of bytes rather than bits.

A sentence or two of "here's the high level setup" would be a helpful introduction, to motivate the parameters, similar to OP's context which explained the Matryoshka pair of nested ciphers. Ideally you would include documentation resembling what TripleSec offers: They put the HMAC in a weird place, up front. A trailing HMAC would be more convenient when streaming network data.

---

local variable

In GetUserSaltAsync, rather than pasting in that LocalApplicationData expression a bunch of times, assign it to a local variable that has a mercifully short name. Or extract a one-line helper function.

It is odd that Salt and saltResult don't have the digit 1 as part of the identifier.

I am sad that we don't see a comment here explaining why it was infeasible to smoosh the four variables into a single unified array.

I can't imagine why we catch the IOException, instead of just letting fatal error bubble up the call stack. Writing @"Error" instead of "Error" seems odd, given that there's no \ backslashes nor other characters to escape in there. Similarly for @"Value was empty." a bit further down.

Returning four empty arrays seems Bad. Wouldn't it be better to not return at all, to let fatal error tank the process?

Similar remarks for error handling further down, e.g. in EncryptFile.

---

random seed

I'm not happy with how seeding is being handled in this code. There are two requirements:

• completely predictable pseudo-random number sequence, for deterministic unit tests, and
• reliably random PRNG sequence, for production use

When we create a RandomNumberGenerator I believe it is seeded from current time? Which is bad for both of those requirements.

I didn't see us reading from /dev/random or from an API that makes similar guarantees.

I am sad that RandomNumberGenerator.GetBytes offers such an inconvenient Public API that we need the trivial RndByteSized helper. But I'm sure the code is better with the helper than without.

---

ternary ternary

When EncryptFile throws an arg exception, the compound ternary ternary expression is just crazy. Prefer three separate {check, throw} lines. Extracting a trivial helper might make that more convenient. The logic is probably correct, but the task of an Author is to show that it is obviously correct.

The pre-condition checks in EncryptAsync are much easier to read, though they would still benefit from Extract Helper.

---

missing param

Fortran coders have a saying: If a function accepts more than half a dozen parameters, you probably left one out.

I don't understand this code at all:

            var saltResult = await Authentication.GetUserSaltAsync(userName);

            var salt = saltResult.Salt;
            var salt2 = saltResult.Salt2;
            var salt3 = saltResult.Salt3;

Why are we ignoring saltResult.Salt4? Or put another way, why did we define it?

Now I really wish a unified array was managing such details for us.

Instead of digits, consider using names like saltAes or saltChaCha.

---

zeroing optimized away

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

I understand the motivation. Kudos for trying to clear it.

But I'm not at all convinced that that had any effect on RAM (and thus on paged out memory pages appearing on disk). The Clear documentation makes no mention of security properties. I imagine the underlying implementation is simply memset.

There is a whole literature on how compilers "prove" that no code reads passWord after the call, therefore it's OK to optimize away those dead stores of zeros. Net result is you get no security at all from writing that line.

OpenSSL, for example, grappled with this issue and now offers a cleanse() function which will actually zero out credentials even when using an optimizing compiler.

MSDN suggests that you might want to use SecureZeroMemory(). Not sure if the "legacy" URL should make me nervous. Quoting from their documentation:

If ZeroMemory were called ... instead of SecureZeroMemory, the compiler could optimize the call because the szPassword buffer is not read from before it goes out of scope. The password would remain on the application stack where it could be captured in a crash dump or probed by a malicious application.

Amusingly, SSE registers are not cleared after use. So 64 bytes of a credential buffer previously copied can still appear in the xmm{0,1,2,3} registers. It's turtles all the way down!

I note in passing that credentials aren't cleared if an exception happens.

There is some layering to your software, and I see credential clearing occurring in more than one place. It would be helpful to document whether caller or callee is responsible for credential clearing.

Please use conventional spelling of password, rather than passWord. It's not like we have a motDePasse phrase or anything.

---

compression

You might possibly wish to use xz or another compressor on plaintext bytes prior to encrypting a file. For example, UTF-16 text files tend to have a lot of zeros which compress nicely.

Once the file has been crypted, all avenues for compression have been closed off.

---

binary streams

You say the code works, and I believe you. In DecryptFile I found this surprising:

            var fileStr = await File.ReadAllTextAsync(file);
            var fileBytes = DataConversionHelpers.Base64StringToByteArray(fileStr);

I was expecting the first call to access an interface for binary data rather than for text. And the second call suggests that an N byte cleartext file turned into significantly more than N bytes of ciphertext?!? Yikes! Or maybe that's just a b64 salt? It's not yet clear to me.

---

check after use

I don't understand the error checking in EncryptAsyncV3. We use salt, ask Argon2id to use it, and then we check whether caller handed us an empty array?!?

Similarly for salt2.

---

DRY

In repetitive error handlers such as what we see in EncryptAsync + EncryptAsyncV3, prefer to Extract Helper rather than keep pasting in the same fragment of source code.

---

informative identifiers

Names like {Encrypt,Decrypt}File are brilliant, they're very helpful to the Gentle Reader.

OTOH the distinction between EncryptAsync and EncryptAsyncV3 is obscure. Those are not helpful names. Figure out how their high level responsibilities differ, and make that clear in their names or at least in their /// documentation comments.

---

method length

I'm reluctant to make this comment because I feel it's not constructive. I don't see a straightforward way to address it.

But many of these methods are starting to get "too long", such that one must scroll vertically in order to take in the logic.

---

unit tests

OP did not include automated unit or integration tests. This codebase really needs them. I would be terrified to change even a single line of code, for fear of invalidating someone's thousand crypted files which become no longer readable due to an ostensible "bug fix".

---

This codebase achieves a subset of its design goals. It does not yet appear to be mature enough for use by multiple individuals who store sensitive material for longer than a few weeks.

I would not be willing to delegate or accept maintenance tasks on it.

---

EDIT

strength analysis

The motivation behind this project is "two ciphers are better than one!", in case one turns out to be weak. Well, let's test that, in the spirit of a "reduced rounds" attack. Let's deliberately make a layer weak and see how the other belt-and-suspender layers cope with that.

Write some plugin layers:

• WeakPlain
• WeakChaCha
• WeakAes

Alice has an 8-bit clean channel. She could be sending /dev/random output. In which case Eve has a hard time knowing whether she won the game. Let's make it easy for her.

WeakPlain puts known MIME headers at front and back of message. Now Eve knows the initial plaintext will always be Content-Transfer-Encoding: 8bit, plus some other predictable headers. We might even have enough constant plaintext to fill the first one or two blocks of a block cipher.

Notice that adding an xz compression layer, which might introduce a constant "fd 37 7a 58 5a 00" header, can play a role similar to WeakPlain.

The weak crypto layers work same as the OP code except that we clear all but the last two bits of the key to zero. That is, by Kerckhoff, we have told Eve what most of the key bits are: they are zero. And any IV or salt parameters are reduced to a single bit, the LSB, with remaining bits cleared to zero. Now Eve can brute force the keyspace in the blink of an eye, it takes almost no effort at all.

There are at least two games Eve can try to win. Reading plaintext is one. The other relates to length, the amount of padding. Eve already knows how many blocks long the plaintext is. We told AES to use PKCS7 padding instead of ciphertext stealing. Suppose the plaintext was chosen to have lengths that make "number of padding bytes" a uniformly distributed random number. If Eve can do better than random guess when she predicts the amount of padding, she wins.

So the question here, that I don't have an answer to, is "which layer would Eve prefer to be weak?" Would ChaCha + WeakAes be stronger than WeakChaCha + Aes ?

A related question: Is it better to do AES first, and then ChaCha ?