3
\$\begingroup\$

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";
        }
    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ You're using multiple objects that implement IDisposable, such as ICryptoTransform, ensure you wrap everything in appropriate using() blocks. \$\endgroup\$ – Dai Aug 9 '16 at 1:29
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.