C# Security Module for generating secure hashes Any pointers on the level of security this hashing function perform at?
I have never had to really write my own modules for security before.

public static class Hashing
{

    public static int SaltSize = 32;

    public static string HashPassword(string passwordToHash, byte[] salt, int numberOfRounds)
    {
        var hashedPassword = HashPassword(Encoding.UTF8.GetBytes(passwordToHash), salt, numberOfRounds);

        return Convert.ToBase64String(salt.Concat(hashedPassword).ToArray());
    }

    public static string HashPassword(string passwordToHash, int numberOfRounds)
    {
        var salt = GenerateSalt();
        var hash =  HashPassword(passwordToHash, salt, numberOfRounds);

        return hash;
    }
    public static byte[] GenerateSalt()
    {
        using (var randomNumberGenerator = new RNGCryptoServiceProvider())
        {
            var randomNumber = new byte[SaltSize];
            randomNumberGenerator.GetBytes(randomNumber);

            return randomNumber;
        }
    }
    public static byte[] HashPassword(byte[] toBeHashed, byte[] salt, int numberOfRounds)
    {
        using (var rfc2898DeriveBytes = new Rfc2898DeriveBytes(toBeHashed, salt, numberOfRounds))
        {
            return rfc2898DeriveBytes.GetBytes(SaltSize);
        }
    }
}

public abstract class HashVerification
{
    public abstract string GetDatabaseHash(string username);

    public bool HashMatch(string username, string password)
    {
        var combinedHash = GetDatabaseHash(username);

        var salt = Convert.FromBase64String(combinedHash).Take(Hashing.SaltSize).ToArray();

        var serverHash = Hashing.HashPassword(password, salt, username.Length * 100);

        return combinedHash.Equals(serverHash);
    }

}
public static class Hashing
{

    public static int SaltSize = 32;



This is public, static, and not const or readonly. That means:


"Anyone" can change it.
It's not thread-safe.
Hash validation is undermined.


I see no reason for it to be public, and every reason for it to be readonly or const. Personally I'd favour readonly and initialised from ConfigurationManager.AppSettings so that an upgrade path can be built in which doesn't require recompiling the code.




    public static string HashPassword(string passwordToHash, byte[] salt, int numberOfRounds)
    ...
    public static string HashPassword(string passwordToHash, int numberOfRounds)
    ...
    public static byte[] GenerateSalt()
    ...
    public static byte[] HashPassword(byte[] toBeHashed, byte[] salt, int numberOfRounds)



Why expose everything? I would expect the only public method to be HashPassword(string password) (or maybe HashPassword(SecureString password), although SecureString is such a nuisance to do anything with that I can understand not supporting it).




        return Convert.ToBase64String(salt.Concat(hashedPassword).ToArray());



Cryptography is an arms war: it's always worth thinking about the upgrade path. That means recording the parameters (name of hash, number of iterations, length of salt) in such a way that they can be extracted when it's time to validate.




        using (var randomNumberGenerator = new RNGCryptoServiceProvider())



I think it's better practice to use RandomNumberGenerator.Create() and leave the selection of the provider to the system-wide configuration.




        using (var rfc2898DeriveBytes = new Rfc2898DeriveBytes(toBeHashed, salt, numberOfRounds))



RFC 2898 was intended as a key derivation function, not a password validation mechanism. It's probably safe, but I'm leery of using it for a different purpose without including a reference to a respected cryptographer who says it's safe.




public abstract class HashVerification
{
    public abstract string GetDatabaseHash(string username);

    public bool HashMatch(string username, string password)



The guideline that one should favour composition over inheritance suggests that hash verification (HashMatch) should probably be in Hashing, and the database fetching should be independent of it.




        var salt = Convert.FromBase64String(combinedHash).Take(Hashing.SaltSize).ToArray();

        var serverHash = Hashing.HashPassword(password, salt, username.Length * 100);



Yikes! Why should people with shorter usernames get less protection? The number of rounds should be another thing that comes from a configuration file so that it can be increased over time, and it should be constant for everyone.




        return combinedHash.Equals(serverHash);



Already addressed in an earlier answer.