An AES Cryptography Wrapper

Question

I built this wrapper over the System.Security.Cryptography.Aes encryption/decryption so that one need only instance the class and call the appropriate methods. This makes it much simpler to work with AES encryption so that developers don't need to reuse the same AES code everywhere they need encryption.

Any and all comments are much appreciated.

/// <summary>
/// Provides Cryptography methods based on AES cryptography implementation.
/// </summary>
public class AesCrypto
{
 /// <summary>
 /// Gets or sets the salt to use with AES encryption.
 /// </summary>
 public byte[] Salt { get; set; }
 /// <summary>
 /// Gets or sets the passphrase for use with AES encryption.
 /// </summary>
 public string Passphrase { get; set; }
 /// <summary>
 /// Constructs a new instance of <see cref="AesCrypto"/> from the specified values.
 /// </summary>
 /// <param name="salt">The <see cref="Salt"/> used in encryption.</param>
 /// <param name="passphrase">The <see cref="Passphrase"/> used in encryption.</param>
 public AesCrypto(byte[] salt, string passphrase)
 {
 if (string.IsNullOrWhiteSpace(passphrase))
 {
 throw new ArgumentException($"The parameter {nameof(passphrase)} is required.");
 }
 if (salt == null || salt.Length == 0)
 {
 throw new ArgumentException($"The parameter {nameof(salt)} is required.");
 }
 Salt = salt;
 Passphrase = passphrase;
 }
 /// <summary>
 /// Uses AES encryption to encrypt a string of data.
 /// </summary>
 /// <param name="clearText">The data to encrypt. Data is expected to be Unicode.</param>
 /// <returns>An encrypted Base64 string.</returns>
 public string AesEncrypt(string clearText)
 {
 byte[] clearBytes = Encoding.Unicode.GetBytes(clearText);
 using (Aes encryptor = Aes.Create())
 {
 Rfc2898DeriveBytes pdb = new Rfc2898DeriveBytes(Passphrase, Salt);
 encryptor.Key = pdb.GetBytes(32);
 encryptor.IV = pdb.GetBytes(16);
 using (MemoryStream ms = new MemoryStream())
 {
 using (CryptoStream cs = new CryptoStream(ms, encryptor.CreateEncryptor(), CryptoStreamMode.Write))
 {
 cs.Write(clearBytes, 0, clearBytes.Length);
 cs.Close();
 }
 clearText = StringExtensions.ToBase64String(ms.ToArray());
 }
 }
 return clearText;
 }
 /// <summary>
 /// Uses AES encryption to decrypt a string of data.
 /// </summary>
 /// <param name="cipherText">The encrypted Base64 string to decrypt.</param>
 /// <param name="Passphrase">The secret pre-shared passphrase.</param>
 /// <param name="throwExceptions">If true, will throw exceptions on decryption failure. Else, returns null string on decryption failure.</param>
 /// <returns>The plaintext data. Data is unicode.</returns>
 public string AesDecrypt(string cipherText, bool throwExceptions)
 {
 if (throwExceptions)
 {
 cipherText = AesDecryptInternal(cipherText);
 }
 else
 {
 try
 {
 cipherText = AesDecryptInternal(cipherText);
 }
 catch
 {
 cipherText = null;
 }
 }
 return cipherText;
 }
 private string AesDecryptInternal(string cipherText)
 {
 byte[] cipherBytes = StringExtensions.FromBase64String(cipherText);
 using (Aes encryptor = Aes.Create())
 {
 Rfc2898DeriveBytes saltDerived = new Rfc2898DeriveBytes(Passphrase, Salt);
 encryptor.Key = saltDerived.GetBytes(32);
 encryptor.IV = saltDerived.GetBytes(16);
 using (MemoryStream ms = new MemoryStream())
 {
 using (CryptoStream cs = new CryptoStream(ms, encryptor.CreateDecryptor(), CryptoStreamMode.Write))
 {
 cs.Write(cipherBytes, 0, cipherBytes.Length);
 cs.Close();
 }
 cipherText = Encoding.Unicode.GetString(ms.ToArray());
 }
 }
 return cipherText;
 }
}

• CodesInChaos: How does the decrypter learn which salt the encrypter used? I don't see you storing it alongside the ciphertext.
• Der Kommissar: @CodesInChaos The developer is responsible for storing that. That's why I made a Salt property, the developer just needs to save the Salt somewhere. I didn't find it necessary to embed the salt with the result.
• CodesInChaos: Implementing proper salt/IV management is at least half of the work of implementing symmetric crypto, so your code doesn't help the developer much. For example an uneducated developer might choose to use the same salt to encrypt more than one message.
• Der Kommissar: @CodesInChaos I suppose it wouldn't be hard to embed the IV or a random salt within the encrypted string.
• bartonjs: Usually the IV is transmitted with the message, and your heuristic here uses the IV as the first 16 bytes of the key, since both of them use the same salt with PBKDF2, reducing your AES256 to AES128. Though really the IV should just be randomly generated, not computed. You encrypt, transmit [random IV][ciphertext], then on decrypt read the first block as IV, then the rest as ciphertext.

Answer 1

public string AesEncrypt(string clearText)
{
 byte[] clearBytes = Encoding.Unicode.GetBytes(clearText);
 using (Aes encryptor = Aes.Create())
 {
 Rfc2898DeriveBytes pdb = new Rfc2898DeriveBytes(Passphrase, Salt);
 encryptor.Key = pdb.GetBytes(32);
 encryptor.IV = pdb.GetBytes(16);
 using (MemoryStream ms = new MemoryStream())
 {
 using (CryptoStream cs = new CryptoStream(ms, encryptor.CreateEncryptor(), CryptoStreamMode.Write))
 {
 cs.Write(clearBytes, 0, clearBytes.Length);
 cs.Close();
 }
 clearText = StringExtensions.ToBase64String(ms.ToArray());
 }
 }
 return clearText;
} 

• the call of the Close() method isn't needed, because the disposing of the CryptoStream will close the stream (See the source).

• Instead of calling ms.ToArray() you should use the MemoryStream.GetBuffer() method to avoid having a copy of the byte array.

The same is true for the AesDecryptInternal() method.

• because Rfc2898DeriveBytes is inheriting DerivedBytes which is implementing IDisposable you should enclose it in an using block too.

The conditions when you throw an ArgumentExceptionin the constructor aren't sufficient. The call to the Rfc2898DeriveBytes constructor will throw if the Salt isn't at least 8 bytes long. So checking against salt.Length < 8 should be done.

In addition to whats being said about the length of the Salt, you should consider to have a backing field for the Salt property so you can change the property to have the same validation there. This will be true for the Passphrase property too.

Answer 2

This feels a little clumsy:

public string AesDecrypt(string cipherText, bool throwExceptions)
{
 if (throwExceptions)
 {
 cipherText = AesDecryptInternal(cipherText);
 }
 else
 {
 try
 {
 cipherText = AesDecryptInternal(cipherText);
 }
 catch
 {
 cipherText = null;
 }
 }
 return cipherText;
}

I suggest an exception filter instead:

public string AesDecrypt(string cipherText, bool throwExceptions)
{
 try
 {
 return AesDecryptInternal(cipherText);
 }
 catch when (!throwExceptions)
 {
 return null;
 }
}

You can also combine the two methods now.

• Der Kommissar: I never knew you could use catch like that, is that new to C#6.0?
• Jeroen Vannevel: Yes, it's a new and often overlooked feature of C# 6