Reputation: 168
The usage of the PasswordDeriveBytes class in a using block (which disposes it, because it implements IDisposable) creates a problem if the class is used a second time. This is the code:
public class AES
{
protected static CryptoData localCryptoData;
static AES()
{
localCryptoData = new CryptoData();
}
public static string Encrypt(CryptoData cryptoData)
{
using (PasswordDeriveBytes pass = new PasswordDeriveBytes(cryptoData.Password, cryptoData.Salt, "SHA1", 2))
using (RijndaelManaged symmetricKey = new RijndaelManaged())
{
byte[] keyBytes = pass.GetBytes(cryptoData.KeySize / 8);
symmetricKey.Padding = PaddingMode.PKCS7;
symmetricKey.Mode = CipherMode.CBC;
using (ICryptoTransform encryptor = symmetricKey.CreateEncryptor(keyBytes, cryptoData.InitVector))
using (MemoryStream memoryStream = new MemoryStream())
using (CryptoStream cryptoStream = new CryptoStream(memoryStream, encryptor, CryptoStreamMode.Write))
{
cryptoStream.Write(cryptoData.ByteText, 0, cryptoData.ByteText.Length);
cryptoStream.FlushFinalBlock();
return Convert.ToBase64String(memoryStream.ToArray());
}
}
}
public static string Decrypt(CryptoData cryptoData)
{
using (PasswordDeriveBytes pass = new PasswordDeriveBytes(cryptoData.Password, cryptoData.Salt, "SHA1", 2))
using (RijndaelManaged symmetricKey = new RijndaelManaged())
{
byte[] cipherTextBytes = Convert.FromBase64String(cryptoData.Text);
byte[] keyBytes = pass.GetBytes(cryptoData.KeySize / 8);
symmetricKey.Padding = PaddingMode.PKCS7;
symmetricKey.Mode = CipherMode.CBC;
using (ICryptoTransform decryptor = symmetricKey.CreateDecryptor(keyBytes, cryptoData.InitVector))
using (MemoryStream memoryStream = new MemoryStream(cipherTextBytes))
using (CryptoStream cryptoStream = new CryptoStream(memoryStream, decryptor, CryptoStreamMode.Read))
{
byte[] textBytes = new byte[cipherTextBytes.Length];
int count = cryptoStream.Read(textBytes, 0, textBytes.Length); //throws CryptographicException - Padding is invalid and cannot be removed.
return Encoding.UTF8.GetString(textBytes, 0, count);
}
}
}
If this class is used in this manner:
AES.Encrypt(cryptoData); AES.Decrypt(cryptoData);
Ths first use gives you a correct AES encrypted string, but if fails with an exception when trying to decrypt the same string. The problem lies in the assigning of the first parameter (The password to derive the key from) from the PasswordDeriveBytes class, when this password is given through a Byte array. If it is given as a string (because of the overload) it works ok.
The helper CryptoData class:
public class CryptoData
{
private string text;
public string Text
{
get { return text; }
set
{
text = value;
if (value != null)
{
ByteText = Encoding.ASCII.GetBytes(value);
}
else
{
ByteText = null;
}
}
}
public byte[] ByteText { get; private set; }
public byte[] Password { get; set; }
public int KeySize { get; set; }
public byte[] InitVector { get; set; }
public byte[] Salt { get; set; }
}
If you just change this row in the methods:
using (PasswordDeriveBytes pass = new PasswordDeriveBytes(cryptoData.Password,
cryptoData.Salt, "SHA1", 2))
into
using (PasswordDeriveBytes pass = new PasswordDeriveBytes("somePassword",
cryptoData.Salt, "SHA1", 2))
everything works fine. The problem is, the instance of PasswordDeriveBytes does not get the byte array for password the second time used, because of the using statement. If a string has been passed, instead of a byte array, it works.
Edit: After reviewing it closer, it seems that there is a problem in the default property setter for the password parameter. It gets the pointer of the array, and that is why it is disposing it. It should make a value.clone() of the array, as is the case with the salt array. It's a definite bug.
Am I right, or am I doing something wrong?
Edit:
*Change the first line in AES.Encrypt() and AES.Decrypt methods with this and it works: *
using (PasswordDeriveBytes pass = new PasswordDeriveBytes(
(byte[])cryptoData.Password.Clone(),
cryptoData.Salt, "SHA1", 2))
Upvotes: 4
Views: 2340
Reputation: 3714
This is certainly counter-intuitive and undocumented behavior, although whether or not it is a bug could possibly be debated. Basically, when you pass the password byte array to the constructor, the PasswordDeriveBytes instance is taking ownership of that array. This is similar to the way StreamReader takes ownership of the Stream which is passed to it, and will Dispose it when it is Disposed (this behavior was also criticized on similar grounds, which led to the addition of a boolean parameter to the StreamReader constructor in .NET 4.0 which can prevent the underlying stream from being Disposed).
Cloning the byte array before you pass it in is probably your best option.
Upvotes: 1