C# AES encryption behaving strangely

Question

I am trying to encrypt a string in C# using this piece of code:

public static string AesEncrypt(string plainText, byte[] Key, byte[] IV)
{
     // Create an Aes object with the specified key and IV
     using Aes aesAlg = Aes.Create();
     aesAlg.Key = Key;
     aesAlg.IV = IV;

     // Create an encryptor 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);

     return Encoding.UTF8.GetString(msEncrypt.ToArray());
}

This is largely based on the sample found here: example.

However, the results are somewhat strange and unpredictable - one time I ran the code and got some string as a result, the other time the resulting string (and the msEncrypt stream) were empty, and one time the application even froze. I don't understand what the problem is.

Edit: I just ran the code again, on this line:

 aesAlg.Key = Key;

something strange happened, I got this error:

Here is how I actually call this method:

public static class Authentication
{
        ...

        private static readonly AesEncryption aes = new AesEncryption();

        public static string GenerateToken()
        {
            AuthenticationData data = new AuthenticationData()
            {
                // some data
            };

            string serialized = JsonConvert.SerializeObject(data);
            return AesEncryption.AesEncrypt(serialized, aes.Key, aes.IV);
        }
}

Answer 1

There are two problems with this code.

1. The SteamWriter isn't closed before the stream is used. There's no call to StreamWriter.Flush either. StreamWriter uses a buffer internally and writes it out to the stream when it's full.

   The StreamWriter doesn't know it's writing to a MemoryStream, all streams look the same to it. Given how small the plaintext is in most examples, no flushing will occur before trying to read the data from the memory stream.

2. UTF8 supports only a limited set of byte sequences. Good encryption algorithms though generate essentially random sequences. Some of them won't correspond to any valid UTF8 sequence. One of the most common way to encode binary data as text is to use Base64 encoding. That's what most encryption or hashing samples show.

The code should change to something like :

public static string AesEncrypt(string plainText, byte[] Key, byte[] IV)
{
     // Create an Aes object with the specified key and IV
     using Aes aesAlg = Aes.Create();
     aesAlg.Key = Key;
     aesAlg.IV = IV;

     ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV);

     using MemoryStream msEncrypt = new MemoryStream();
     using CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write);
     using StreamWriter swEncrypt = new StreamWriter(csEncrypt);
     swEncrypt.Write(plainText);

     //CHANGES HERE
     swEncrypt.Flush();
     return Convert.ToBase64String(msEncrypt.ToArray());
}

The sample doesn't suffer from this problem because it uses explicit using blocks, so the writer is closed before the memory stream is used :

using (MemoryStream msEncrypt = new MemoryStream())
{
    using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
    {
        using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
        {
            swEncrypt.Write(plainText);
        }
        encrypted = msEncrypt.ToArray();
    }
}

Neither Visual Studio nor Resharper would suggest using a using statement in this case, as the writer is clearly closed in the innermost block.

Don't mix using styles

You can mix using blocks and statements, eg :

using MemoryStream msEncrypt = new MemoryStream();
using CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write);
using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
{
    swEncrypt.Write(plainText);
}
encrypted = msEncrypt.ToArray();

Given the confusion caused already though, you probably shouldn't. The code isn't that much cleaner and it already caused a problem. Someone else looking at the same code in the future may get confused as well and use a bad refactoring