C# AES Encryption

Question

I've been researching AES encryption a bit over the past several days. The official (MSDN) examples I've seen are encrypting and decrypting using the same AES instance. They don't go in to what to do when generating and saving an encrypted value with AES and needing to decrypt it later with another AES instance.

I came up with the following and am wondering if there is anything wrong with it, aside from defaulting the password to a static value (I will be developing something else to manage encryption passwords)? It generates a random salt on encryption and stores it with the encrypted cipher prior to Base64 encoding. This assures that running the encryption twice on the same input does not result in the same cipher text.

 public static string Encrypt(string plainText, string password = "BadgersAreAwesome")
 {
 if (plainText == null)
 throw new ArgumentNullException("plainText");
 if (password == null)
 throw new ArgumentNullException("password");
 // Will return the cipher text
 string cipherText = "";
 // Utilizes helper function to generate random 16 byte salt using RNG
 byte[] salt = GenerateSaltBytes(SaltSize);
 // Convert plain text to bytes
 byte[] plainBytes = Encoding.Unicode.GetBytes(plainText);
 // create new password derived bytes using password/salt
 using (Rfc2898DeriveBytes pdb = new Rfc2898DeriveBytes(password, salt))
 {
 using (Aes aes = AesManaged.Create())
 {
 // Generate key and iv from password/salt and pass to aes
 aes.Key = pdb.GetBytes(aes.KeySize / 8);
 aes.IV = pdb.GetBytes(aes.BlockSize / 8);
 // Open a new memory stream to write the encrypted data to
 using (MemoryStream ms = new MemoryStream())
 {
 // Create a crypto stream to perform encryption
 using (CryptoStream cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write))
 {
 // write encrypted bytes to memory
 cs.Write(plainBytes, 0, plainBytes.Length);
 }
 // get the cipher bytes from memory
 byte[] cipherBytes = ms.ToArray();
 // create a new byte array to hold salt + cipher
 byte[] saltedCipherBytes = new byte[salt.Length + cipherBytes.Length];
 // copy salt + cipher to new array
 Array.Copy(salt, 0, saltedCipherBytes, 0, salt.Length);
 Array.Copy(cipherBytes, 0, saltedCipherBytes, salt.Length, cipherBytes.Length);
 // convert cipher array to base 64 string
 cipherText = Convert.ToBase64String(saltedCipherBytes);
 }
 aes.Clear();
 }
 }
 return cipherText;
 }
 public static string Decrypt(string cipherText, string password = "BadgersAreAwesome")
 {
 if (cipherText == null)
 throw new ArgumentNullException("cipherText");
 if (password == null)
 throw new ArgumentNullException("password");
 // will return plain text
 string plainText = "";
 // get salted cipher array
 byte[] saltedCipherBytes = Convert.FromBase64String(cipherText);
 // create array to hold salt
 byte[] salt = new byte[SaltSize];
 // create array to hold cipher
 byte[] cipherBytes = new byte[saltedCipherBytes.Length - salt.Length];
 // copy salt/cipher to arrays
 Array.Copy(saltedCipherBytes, 0, salt, 0, salt.Length);
 Array.Copy(saltedCipherBytes, salt.Length, cipherBytes, 0, saltedCipherBytes.Length-salt.Length);
 // create new password derived bytes using password/salt
 using (Rfc2898DeriveBytes pdb = new Rfc2898DeriveBytes(password, salt))
 {
 using (Aes aes = AesManaged.Create())
 {
 // Generate key and iv from password/salt and pass to aes
 aes.Key = pdb.GetBytes(aes.KeySize / 8);
 aes.IV = pdb.GetBytes(aes.BlockSize / 8);
 // Open a new memory stream to write the encrypted data to
 using (MemoryStream ms = new MemoryStream())
 {
 // Create a crypto stream to perform decryption
 using (CryptoStream cs = new CryptoStream(ms, aes.CreateDecryptor(), CryptoStreamMode.Write))
 {
 // write decrypted data to memory
 cs.Write(cipherBytes, 0, cipherBytes.Length); 
 }
 // convert decrypted array to plain text string
 plainText = Encoding.Unicode.GetString(ms.ToArray());
 }
 aes.Clear();
 }
 }
 return plainText;
 }

• CodesInChaos: Take a look at jbtule's answer to Encrypt and decrypt a string
• DxTx: Check this Microsoft Article: docs.microsoft.com/en-us/dotnet/api/…

Answer 1

• You're obtaining more than 20 bytes from PBKDF2-HMAC-SHA-1 and the attacker doesn't need the data from the second block (12 IV bytes), so your code slows down defenders by a factor 2 without affecting attackers.

• generate random 16 bit salt using RNG

  16 bits is very short. You should use 16 bytes or 128 bits. I suspect this is a typo in the comment, since Rfc2898DeriveBytes rejects salts shorter than 8 bytes.

• 1000 iterations of PBKDF2-HMAC-SHA-1 is pretty low. I'd use at least 10000.

• You don't have a MAC, leaving you open to active attacks, such as padding oracles
• if you use aes.CreateEncryptor().TransformFinalBlock you can throw out all those streams
• I'd consider using UTF-8 over UTF-16. It's shorter for most non-Asian texts.

• You're using password based encryption. That can be the right choice, but depending on your application you should consider:

  • Key based encryption, with the key stored in a password encrypted file key file (similar to SSH keys).

    This means you only need to run the expensive key derivation operation when opening the password file, instead of per message. So you can probably afford more iterations, improving security.

  • Key based encryption with a randomly generated key
  • An encrypted network protocol like TLS

• Consider using a better password hash, like bcrypt or scrypt and/or a faster (native) implementation with more iterations. That way you strengthen the password more, making password guessing attacks more expensive.

• Rfc2898DeriveBytes generates a random salt if you pass in the length of the salt, so you don't need your own method.

• Sam: I'm relatively new to this so I'm not sure what you mean in your first point. You are correct, 16 bit was a typo--it is 16 bytes. I would use the Rfc2898DeriveBytes(password, saltSize, iterations) constructor to generate a random salt and specify more iterations, correct? Is salt size for this constructor in bits or bytes (Intellisense/MSDN doesn't say)? Do you have examples for the key based encryption stuff you mentioned? I'm trying to keep this somewhat simple so consumers can just call Encrypt(plainText)/Decrypt(cipherText). I have to work out the details on managing the password.
• Sam: CodesInChaos, I just researched the TransformFinalBlock. It does look like I can get rid of the streams. But I also noticed that ICryptoTransform returned by CreateEncryptor() and CreateDecryptor() is an IDisposable. Should that not be wrapped in a using block and properly disposed of?

Answer 2

This looks quite ok (without digging more into AES).

Good

• you name almost all of your variables meaningful
• you use the proper casing style for naming methods and variables
• you are throwing the exception which are expected

Improvable

• you should use braces {} also for single if statements to make your code less errorprone.
• you could return String.Empty for the case that plainText == String.Empty

• you could stack your using statements where possible to reduce horizontal spacing

  using (Rfc2898DeriveBytes pdb = new Rfc2898DeriveBytes(password, salt))
  using (Aes aes = AesManaged.Create())
  {
   aes.Key = pdb.GetBytes(aes.KeySize / 8);
   aes.IV = pdb.GetBytes(aes.BlockSize / 8);
   using (MemoryStream ms = new MemoryStream())
   using (CryptoStream cs = new CryptoStream(ms, aes.CreateEncryptor(), CryptoStreamMode.Write))
   {
   cs.Write(plainBytes, 0, plainBytes.Length);
   byte[] cipherBytes = ms.ToArray();
   // create a new byte array to hold salt + cipher
   byte[] saltedCipherBytes = new byte[salt.Length + cipherBytes.Length];
   Array.Copy(salt, 0, saltedCipherBytes, 0, salt.Length);
   Array.Copy(cipherBytes, 0, saltedCipherBytes, salt.Length, cipherBytes.Length);
   cipherText = Convert.ToBase64String(saltedCipherBytes);
   }
   aes.Clear();
  }
  

• it would be better to return the byte[] instead of the encoded string. If you later on need the string you could easily convert it. But if you need the raw bytes, you need to convert the string back.

  So assuming you have changed the method to return byte[] instead of string we will add a overloaded method

  public static string Encrypt(string plainText, string password = "BadgersAreAwesome") 
  {
   return Convert.ToBase64String(Encrypt(plainText,password));
  } 
  

  Additional thought, you should also change the method to take a byte[] instead of a string as inputparameter, which will make the method more flexible. You usually will e.g encrypt files which aren't necessaryly textfiles.

Naming

• you should consider renaming Rfc2898DeriveBytes pdb to a more descriptive name. Shortening names removes readability.

General

• comments should be used to describe why something is done. Let the code itself describe what is done by using meaningful names for methods, classes and variables.

• if the returned type is obvious from the right hand side of an assignment you could use var instead of the type. E.g

  using (var ms = new MemoryStream()) 
  

• instead of initialising a string variable to "" you should consider to use String.Empty which is clearer.

• Sam: Thanks for the feedback. I tend to use if blocks without brackets if the condition only uses one line and there is no else condition. I had been using String.Empty for several years, but someone convinced me that "" is shorter and easier for JavaScript/Java people to read/understand. I'm a little embarrassed that I've been programming in C# for almost 10 years and didn't know about stacking usings. However, in this case the CryptoStream has to be disposed of before you can read from the memory stream.
• Sam: I'm also curious why you suggested me using the more verbose String.Empty instead of "", then recommended I use the less verbose var instead of the type name?
• Heslacher: For String.Empty I said should, because I think it is more readable on the first glance. To distinguish between "" and " " with not so good eyes is hard. For var I said could to show this possibility.