AES Encryption C# .NET

Question

I have written a tool for encrypting string using the AesCryptoServiceProvider in .NET. The folllowing parameters are used:

• Block Cipher Mode: CBC
• Initialization Vector: 16 bytes (randomized per encryption operation), stored in last 16 bytes of final message
• Key/Block size: 128 bit Padding: PKCS #7

Overview of code I have added two functions below. The first prepares a message for encryption, takes the result and appends the Initialization Vector to the cipher message. The second function encrypts the provided message using the specified key and initialization vector.

Obviously, i'm interested in any security holes that exist with this code. Also any optimizations are very much appreciated.

Side questions. In development I always store bytes as strings in their hex representation, seen below. Is this bad? The only other alternative I see is base64 encoding? Which is better?

public void cryptoExample {

    AesCryptoServiceProvider aes = new AesCryptoServiceProvider();
    aes.KeySize = 128;
    aes.BlockSize = 128;

    aes.Key = Helper.StringToByteArray("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"); // Sample key
    aes.GenerateIV();

    String plainMessage = "Hello AES Crypto!!";

    String encryptedMessage = Helper.ByteArrayToString(CryptoHelper.EncryptStringToBytes_Aes(plainMessage, aes.Key, aes.IV));

    // Must always use a random IV, meaning it needs to be stored with the cipher message.
    // Append it to the end. IV is a constant 16 bytes so we just extract and remove it before decryption.
    String strIv = Helper.ByteArrayToString(aes.IV);

    String messageToTransmit = encryptedMessage += strIv;
}
public static byte[] EncryptStringToBytes_Aes(string plainText, byte[] Key, byte[] IV)
{
    // Check arguments.
    if (plainText == null || plainText.Length <= 0)
        throw new ArgumentNullException("plainText");
    if (Key == null || Key.Length <= 0)
        throw new ArgumentNullException("Key");
    if (IV == null || IV.Length <= 0)
        throw new ArgumentNullException("IV");
    byte[] encrypted;
    // Create an AesCryptoServiceProvider object
    // with the specified key and IV.
    using (AesCryptoServiceProvider aesAlg = new AesCryptoServiceProvider())
    {
        aesAlg.Key = Key;
        aesAlg.IV = IV;
        aesAlg.Mode = CipherMode.CBC;
        aesAlg.Padding = PaddingMode.PKCS7;
        // Create a decrytor to perform the stream transform.
        ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV);

        // Create the streams used for encryption.
        using (MemoryStream msEncrypt = new MemoryStream())
        {
            using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
            {
                using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
                {

                    //Write all data to the stream.
                    swEncrypt.Write(plainText);
                }
                encrypted = msEncrypt.ToArray();
            }
        }
    }
}

Answer 1

1. You can simplify it by replacing all those streams with a call to encrypter.TransformFinalBlock.
2. You don't have a MAC, so an attacker who can send a modifier message to the decrypter can probably exploit a padding oracle to decrypt the message
3. Why ask the caller for an IV? You could generate it internally. If you want to keep a deterministic implementation around, at least create an overload without IV.
4. Helper.ByteArrayToString and StringToByteArray should have a name that says which encoding (hex, Base64, etc.) they use. I'd use BytesToHexString and HexStringToBytes.

Answer 2

Few tiny notes:

1. In the method cryptoExample, the usage of AesCryptoServiceProvider should be wrapped in a using block much like it is in the EncryptStringToBytes_Aes method.

2. Similarly, ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV) in the EncryptStringToBytes_Aes method is an IDisposable resource and should also be wrapped in a using block.

3. The comment above that line says "create a decryptor", yet, it's creating an encryptor. Will confuse future readers.