Encrypt and decrypt a message using AES-256 with GCM mode using Bouncy Castle C# library

Question

Goal:

Given a plaintext message and 256 bit key, encrypt (and subsequently decrypt) the message using a 12 byte IV (in this case null bytes for simplicity, should not do this, I know) with MAC of 128-bit length using GCM mode of AES symmetric algorithm with/without Authenticated Encryption with Associated Data (AEAD).

Since .NET framework (any version so far!) does not support AES in GCM mode, I used Bouncy Castle C# library (version: 1.8.1) (on .NET framework 4.7) to encrypt and decrypt data using following code. The test passes, however, I am not sure if this is the correct way to write this code.

Can somebody please validate that this code is written correctly and if so, is this the preferred way of writing this code? (Please ignore the utility functions in your comments)

using Microsoft.VisualStudio.TestTools.UnitTesting;
using Org.BouncyCastle.Crypto.Engines;
using Org.BouncyCastle.Crypto.Modes;
using Org.BouncyCastle.Crypto.Parameters;
using System;
using System.Collections;
using System.Linq;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;

[TestMethod]
public void TestCanEncryptWithAesAnd256BitKey()
{
    var hexString = "305B624727B235489A72B42F01564ED0CDF46230316EE74B2BB88170D08382C2";
    var key = StringToByteArray(hexString);

    var plainText = "The whole problem with the world is that fools and fanatics are always so certain of themselves, and wiser people so full of doubts - Bertrand Russell (1872-1970)";
    var plainTextBytes = Encoding.ASCII.GetBytes(plainText);
    Console.WriteLine($"Plain Text =>{plainText} (Number of bytes: {plainTextBytes.Length})");
    var associatedText = "My Associated Text";  //Random text that can be used in encryption
    var associatedTextBytes = Encoding.ASCII.GetBytes(associatedText);
    var useAssociatedText = true;

    var encryptCipher = new GcmBlockCipher(new AesFastEngine());
    var encryptKeyParameter = new KeyParameter(key);
    if (useAssociatedText)
    {
        encryptCipher.Init(true, new AeadParameters(encryptKeyParameter, 128, new byte[12], associatedTextBytes));
    }
    else
    {
        encryptCipher.Init(true, new AeadParameters(encryptKeyParameter, 128, new byte[12]));
    }

    var expectedNumberOfOutputBytes = (plainTextBytes.Length / 16) * 16;
    byte[] firstEncryptedBytes = new byte[expectedNumberOfOutputBytes];
    var numberOfOutputBytes = encryptCipher.ProcessBytes(plainTextBytes, 0, plainTextBytes.Length, firstEncryptedBytes, 0);
    Assert.AreEqual(expectedNumberOfOutputBytes, numberOfOutputBytes);

    expectedNumberOfOutputBytes = (plainTextBytes.Length % 16) + 16;
    byte[] lastEncryptedBytes = new byte[expectedNumberOfOutputBytes];
    numberOfOutputBytes = encryptCipher.DoFinal(lastEncryptedBytes, 0); //Compute the MAC tag
    Assert.AreEqual(expectedNumberOfOutputBytes, numberOfOutputBytes);

    // Concatentate byte array to get all encrypted bytes with tag
    byte[] totalEncryptedBytes = new byte[plainTextBytes.Length + 16];
    Assert.AreEqual(totalEncryptedBytes.Length, firstEncryptedBytes.Length + lastEncryptedBytes.Length);

    Buffer.BlockCopy(firstEncryptedBytes, 0, totalEncryptedBytes, 0, firstEncryptedBytes.Length);
    Buffer.BlockCopy(lastEncryptedBytes, 0, totalEncryptedBytes, firstEncryptedBytes.Length, lastEncryptedBytes.Length);

    DumpBytes(plainTextBytes, nameof(plainTextBytes));
    DumpBytes(firstEncryptedBytes, nameof(firstEncryptedBytes));
    DumpBytes(lastEncryptedBytes, nameof(lastEncryptedBytes));
    DumpBytes(totalEncryptedBytes, nameof(totalEncryptedBytes));

    // ********* Copy the encrypted bytes to buffer (strictly speaking:not needed) ***********
    var bytesToDecrypt = new byte[totalEncryptedBytes.Length];
    Buffer.BlockCopy(totalEncryptedBytes, 0, bytesToDecrypt, 0, bytesToDecrypt.Length);

    // Tampering code that messes up the tag itself
    //bytesToDecrypt[bytesToDecrypt.Length - 2] = 117;
    // Tampering code that changes the first byte in the message
    //bytesToDecrypt[0] = 117;
    // Tampering with associated text
    //associatedTextBytes = Encoding.ASCII.GetBytes(associatedText + "Hacked!");

    // ********* Decryption ***********
    var decryptCipher = new GcmBlockCipher(new AesFastEngine());
    var decryptKeyParameter = new KeyParameter(key);
    if (useAssociatedText)
    {
        decryptCipher.Init(false, new AeadParameters(encryptKeyParameter, 128, new byte[12], associatedTextBytes));
    }
    else
    {
        decryptCipher.Init(false, new AeadParameters(encryptKeyParameter, 128, new byte[12]));
    }

    var lengthWithoutTag = bytesToDecrypt.Length - 16;
    expectedNumberOfOutputBytes = (lengthWithoutTag / 16) * 16;
    byte[] firstDecryptedBytes = new byte[expectedNumberOfOutputBytes];
    numberOfOutputBytes = decryptCipher.ProcessBytes(bytesToDecrypt, 0, bytesToDecrypt.Length, firstDecryptedBytes, 0);
    Assert.AreEqual(expectedNumberOfOutputBytes, numberOfOutputBytes);
    //Console.WriteLine($"Decrypted Text =>{Encoding.ASCII.GetString(outputBytes)} (Number of bytes: {outputBytes.Length})");

    expectedNumberOfOutputBytes = (bytesToDecrypt.Length % 16);
    var lastDecryptedBytes = new byte[expectedNumberOfOutputBytes];
    numberOfOutputBytes = decryptCipher.DoFinal(lastDecryptedBytes, 0);  //Validate the MAC tag
    Assert.AreEqual(expectedNumberOfOutputBytes, numberOfOutputBytes);

    // Concatentate byte array to get all decrypted bytes
    byte[] totalDecryptedBytes = new byte[lengthWithoutTag];
    Assert.AreEqual(totalDecryptedBytes.Length, firstDecryptedBytes.Length + lastDecryptedBytes.Length);

    Buffer.BlockCopy(firstDecryptedBytes, 0, totalDecryptedBytes, 0, firstDecryptedBytes.Length);
    Buffer.BlockCopy(lastDecryptedBytes, 0, totalDecryptedBytes, firstDecryptedBytes.Length, lastDecryptedBytes.Length);

    //DumpBytes(bytesToDecrypt, nameof(bytesToDecrypt));
    //DumpBytes(decryptedBytes, nameof(decryptedBytes));
    //DumpBytes(finalDecryptedBlockOutputBytes, nameof(finalDecryptedBlockOutputBytes));
    //DumpBytes(totalDecryptedOutputBytes, nameof(totalDecryptedOutputBytes));

    Console.WriteLine($"{plainText} (Number of bytes: {plainTextBytes.Length}) (Before Encryption)");
    Console.WriteLine($"{Encoding.ASCII.GetString(totalEncryptedBytes)} (Number of bytes: {totalEncryptedBytes.Length})  (After Encryption)");
    Console.WriteLine($"{Encoding.ASCII.GetString(totalDecryptedBytes)} (Number of bytes: {totalDecryptedBytes.Length})  (After Decryption)");

    Assert.AreEqual(plainTextBytes.Length, totalDecryptedBytes.Length);
    Assert.AreEqual(plainText, Encoding.ASCII.GetString(totalDecryptedBytes));
}

private byte[] StringToByteArray(String hex)
{
    int NumberChars = hex.Length;
    byte[] bytes = new byte[NumberChars / 2];
    for (int i = 0; i < NumberChars; i += 2)
        bytes[i / 2] = Convert.ToByte(hex.Substring(i, 2), 16);

    DumpBytes(bytes);
    return bytes;
}

private void DumpBytes(byte[] bytes, string name = null)
{
    if (name != null)
    {
        Console.WriteLine($"{name}");
    }

    for (int i = 0; i < bytes.Length; i++)
    {
        Console.WriteLine($"[{i}]: {bytes[i]}");
    }

    var hexString = BitConverter.ToString(bytes).Replace("-", string.Empty);
    Console.WriteLine($"Hex format: {hexString}");
    Console.WriteLine($"byte array length: {bytes.Length} ({name})");
}

Answer 1

OK, so let's get through this.

1. storing the key in an immutable string is not a good idea, storing it in a byte array and then destroying the key data is a better way of handling the key;
2. calling the method multiple times will reuse the zero IV, which will all but destroy the confidentiality - use a random IV / nonce instead (12 bytes / 96 bits is the right size though);
3. using ASCII encoding may lead to loss of data if characters outside the ASCII range are used as input;
4. using GetOutputSize can be used to determine the size of the output, there is no need to do all the copying from one array to the other;
5. using ProcessAadBytes could be used instead of the if switch between the two parameter constructors (there are two ways of processing the AAD bytes) - it is still preferred to handle the AAD before the other plaintext though;
6. StringToByteArray is horribly named, it should be named DecodeHex or something similar.