AES class security

Question

I am using the AES class (which I think I got from another post on Stack Overflow) for encrypting strings to a database. How robust and secure (would it be easy to break) is this class? Is there any way I can improve it as it is storing usernames and passwords?

Please be gentle as I have to be honest although I have spent a few days looking into AES encryption. It's still all a little murky and just looking for some advice\input.

When I encrypt a string, the same password is used but the salt is always different.

Example usage:

AES encrypt = new AES("password","Unique string");
encrypt.Encrypt(stringtoencrypt);

Class:

public class AES
{
    // Symmetric algorithm interface is used to store the AES service provider
    private SymmetricAlgorithm AESProvider;

    /// <summary>
    /// Constructor for AES class that takes a byte array for the key
    /// </summary>
    /// <param name="key">256 bit key (32 bytes)</param>
    public AES(byte[] key)
    {
        // Throw error if key is not 256 bits
        if (key.Length != 32) throw new CryptographicException("Key must be 256 bits (32 bytes)");

        // Initialize AESProvider with AES algorithm service
        AESProvider = new AesCryptoServiceProvider();
        AESProvider.KeySize = 256;

        // Set the key for AESProvider
        AESProvider.Key = key;
    }

    /// <summary>
    /// Constructor for AES class that generates the key from a hashed, salted password
    /// </summary>
    /// <param name="password">Password used to generate the key (Minimum of 8 characters)</param>
    /// <param name="salt">Salt used to secure hash from rainbow table attacks (Minimum of 8 characters)</param>
    public AES(string password, string salt)
    {
        // Throw error if the password or salt are too short
        if (password.Length < 8) throw new CryptographicException("Password must be at least 8 characters long");
        if (salt.Length < 8) throw new CryptographicException("Salt must be at least 8 characters long");

        // Initialize AESProvider with AES algorithm service
        AESProvider = new AesCryptoServiceProvider();
        AESProvider.KeySize = 256;

        // Initialize a hasher with the 256 bit SHA algorithm
        SHA256 sha256 = System.Security.Cryptography.SHA256.Create();

        // Hash salted password
        byte[] key = sha256.ComputeHash(UnicodeEncoding.Unicode.GetBytes(password + salt));

        // Set the key for AESProvider
        AESProvider.Key = key;

    }

    /// <summary>
    /// Encrypts a string with AES algorithm
    /// </summary>
    /// <param name="plainText">String to encrypt</param>
    /// <returns>Encrypted string with IV prefix</returns>
    public string Encrypt(string plainText)
    {
        // Create new random IV
        AESProvider.GenerateIV();

        // Initialize encryptor now that the IV is set
        ICryptoTransform encryptor = AESProvider.CreateEncryptor();

        // Convert string to bytes
        byte[] plainBytes = UnicodeEncoding.Unicode.GetBytes(plainText);

        // Encrypt plain bytes
        byte[] secureBytes = encryptor.TransformFinalBlock(plainBytes, 0, plainBytes.Length);

        // Add IV to the beginning of the encrypted bytes
        secureBytes = AESProvider.IV.Concat(secureBytes).ToArray();

        // Return encrypted bytes as a string
        return Convert.ToBase64String(secureBytes);
    }

    /// <summary>
    /// Decrypts a string with AES algorithm
    /// </summary>
    /// <param name="secureText">Encrypted string with IV prefix</param>
    /// <returns>Decrypted string</returns>
    public string Decrypt(string secureText)
    {
        // Convert encrypted string to bytes
        byte[] secureBytes = Convert.FromBase64String(secureText);
        byte[] plainBytes = null;
        // Take IV from beginning of secureBytes
        AESProvider.IV = secureBytes.Take(16).ToArray();

        // Initialize decryptor now that the IV is set
        ICryptoTransform decryptor = AESProvider.CreateDecryptor();

        // Decrypt bytes after the IV
        try
        {
            plainBytes = decryptor.TransformFinalBlock(secureBytes, 16, secureBytes.Length - 16);
        }
        catch (Exception e)
        {
            Logger log = new Logger(DateTime.Now, "Decrypt Error", e.Message);
            log.WriteErrorLog();
        }

        // Return decrypted bytes as a string
        try
        {
            return UnicodeEncoding.Unicode.GetString(plainBytes);
        }
        catch (Exception ex)
        {
            Logger log = new Logger(DateTime.Now, "Decrypt Error - Possible wrong password", ex.Message);
            log.WriteErrorLog();
            return "#1Error#1";
        }
    }

}

Answer 1

You use an interface, but then use new to instantiate everything - that defeats the entire purpose of using interfaces. Take a SymmetricAlgorithm as an input and then use that. Also, I don't really like the name AESProvider - the AES is redundant and it reads like a type, not a variable. I'd prefer provider.

You should have validation logic put into its own methods, i.e. validateKey, validatePassword, validateSalt. Makes it easier to change them, and it provides a better separation of powers.

Not a huge fan of this being hard-wired for 256 bit. Yeah, that's the best from a security standpoint, but there are going to be people and situations that need compatibility with 128 and 192. That should probably be another parameter.

I'd also use a factory to get the hash of a password and salt.

You can consolidate your logic pretty easily to look like this

public class AES
{
    private SymmetricAlgorithm provider;
    private int size;

    public AES(byte[] key, SymmetricAlgorithm provider, int size)
    {
        validateKey(key);
        init(key, provider, size);
    }

    public AES(string password, string salt, SymmetricAlgorithm provider, int size, IHashFactory hashFactory)
    {
        validatePassword(password);
        validateSalt(salt);

        var hasher = hashFactory.getHasher(size);
        var key = hasher.ComputeHash(UnicodeEncoding.Unicode.GetBytes(password + salt));
        init(key, provider, size);
    }

    private void init(byte[] key, SymmetricAlgorithm provider, int size)
    {
        this.provider = provider;
        this.provider.KeySize = size;
        this.provider.Key = key;
    }

    private void validateKey(byte[] key)
    {
        if (key.Length != 32) 
        {
            throw new CryptographicException("Key must be 256 bits (32 bytes)");
        }
    }

    private void validatePassword(string password)
    {
        if (password.Length < 8) 
        {
            throw new CryptographicException("Password must be at least 8 characters long");
        }
    }

    private void validateSalt(string salt)
    {
        if (salt.Length < 8) 
        {
            throw new CryptographicException("Salt must be at least 8 characters long");
        }
    }
}

Beyond that I don't know enough about crypto to address whether or not things are secure. In general, I'd just repeat the old maxim "Don't roll your own". If you can find a library that meets your needs, use it.

Answer 2

With encription it might be important that you don't have memory leaks.

The CreateEncryptor Method () returns a ICryptoTransform Interface that requires IDisposable to be implemented.

For example:

using(var encryptor = AESProvider.CreateEncryptor())
{
    // ...
}

The AesCryptoServiceProvider is disposable as well so should your AES class be and implement the IDisposable interface.