We are using an outdated 3DES algorithm for encryption and I have been tasked with writing a new implementation using AES with a 128bit shared secret.
I would like to know if there are any security holes in my implementation and if there is any way to optimize the code. Please note that I have removed the code for implementation of IDisposable
since there is nothing special going on there.
public class AESCrypto : IDisposable
{
private static byte[] _salt = Encoding.ASCII.GetBytes("SomeConstantSalt");
private string _sharedSecret;
private RijndaelManaged rm;
public AESCrypto(string SharedSecret)
{
rm = new RijndaelManaged();
rm.Mode = CipherMode.CBC;
_sharedSecret = SharedSecret;
}
public string Encrypt(string PlainText)
{
if (string.IsNullOrEmpty(PlainText))
throw new ArgumentNullException("PlainText");
string output = null;
var key = new Rfc2898DeriveBytes(_sharedSecret, _salt, 10000);
rm.Key = key.GetBytes(rm.KeySize / 8);
rm.GenerateIV();
using (ICryptoTransform encryptor = rm.CreateEncryptor(rm.Key, rm.IV))
using (var ms = new MemoryStream())
{
//Prepend the initialization vector
ms.Write(BitConverter.GetBytes(rm.IV.Length), 0, sizeof(int));
ms.Write(rm.IV, 0, rm.IV.Length);
using (var cs = new CryptoStream(ms, encryptor, CryptoStreamMode.Write))
{
using (var sw = new StreamWriter(cs))
{
sw.Write(PlainText);
}
}
output = Convert.ToBase64String(ms.ToArray());
}
return output;
}
public string Decrypt(string CipherText)
{
if (string.IsNullOrEmpty(CipherText))
throw new ArgumentNullException("CipherText");
string plainText = null;
var key = new Rfc2898DeriveBytes(_sharedSecret, _salt, 10000);
byte[] bytes = Convert.FromBase64String(CipherText);
using (var ms = new MemoryStream(bytes))
{
rm.Key = key.GetBytes(rm.KeySize / 8);
//Get the initialization vector from the encrypted stream
rm.IV = ReadByteArray(ms);
ICryptoTransform decryptor = rm.CreateDecryptor(rm.Key, rm.IV);
using (var cs = new CryptoStream(ms, decryptor, CryptoStreamMode.Read))
{
using (var sr = new StreamReader(cs))
{
//Read bytes from stream to the end
plainText = sr.ReadToEnd();
}
}
}
return plainText;
}
private static byte[] ReadByteArray(Stream s)
{
byte[] rawLength = new byte[sizeof(int)];
if(s.Read(rawLength, 0, rawLength.Length) != rawLength.Length)
{
throw new SystemException("Stream did not contain properly formatted byte array");
}
byte[] buffer = new byte[BitConverter.ToInt32(rawLength, 0)];
if(s.Read(buffer, 0, buffer.Length) != buffer.Length)
{
throw new SystemException("Did not read byte array properly");
}
return buffer;
}
Rfc2898DeriveBytes
implementsIDisposable
, so it's lifetime should be wrapped in ausing
construct. Further, your elidedDispose()
code should be callingrm.Dispose();
at the very least since the class holds anIDisposable
resource in theRijndaelManaged
member. \$\endgroup\$Dispose
method disposes ofrm
. I will add a using statment forRfc2898DeriveBytes
as you suggested. \$\endgroup\$