C# Security Module for generating secure hashes

Question

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

}

Do you have a good reason not to use something premade? Remember it's rarely a good idea to make your own crypto — yuri
– yuri, Commented Mar 19, 2018 at 13:25
I wouldn't say I am using my own crypto code this is just kind of an extension of the existing code. I was more concerned with the actual hashing process. I have always used the Microsoft Web Api and it takes care of hashing and login for me but I wan to build my own simple re-usable module. — Bailey Miller
– Bailey Miller, Commented Mar 19, 2018 at 13:28

Peter Taylor · Accepted Answer · 2019-07-27 15:07:05Z

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.

I agree with iterations not having to depend on the length of the user name. As with the salt, the number of iterations might also be configurable and stored in the digest. This would also allow for forward-compatibility. — dfhwze
– dfhwze, Commented Jul 27, 2019 at 16:27
Not sure whether you just wanted to fetch the iterations from a config file, or also to store it in the digest. If so, my comment is mute. — dfhwze
– dfhwze, Commented Jul 27, 2019 at 21:33
"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." — Peter Taylor
– Peter Taylor, Commented Jul 27, 2019 at 21:52
that is exactly the point yes; it is not imperative for this information to be kept secret — dfhwze
– dfhwze, Commented Jul 27, 2019 at 21:53

dfhwze · Accepted Answer · 2019-07-27 13:53:33Z

Review

You should obfuscate the equality check return combinedHash.Equals(serverHash); using a SlowEquals implementation.

Khalid Abuhakmeh's post explains the vulnerability.

    /// <summary>
    /// Compares two byte arrays in length-constant time. This comparison
    /// method is used so that password hashes cannot be extracted from
    /// on-line systems using a timing attack and then attacked off-line.
    /// </summary>
    /// <param name="a">The first byte array.</param>
    /// <param name="b">The second byte array.</param>
    /// <returns>True if both byte arrays are equal. False otherwise.</returns>
    private static bool SlowEquals(byte[] a, byte[] b)
    {
        uint diff = (uint)a.Length ^ (uint)b.Length;
        for (int i = 0; i < a.Length && i < b.Length; i++)
            diff |= (uint)(a[i] ^ b[i]);
        return diff == 0;
    }

Stack Exchange Network

C# Security Module for generating secure hashes

2 Answers 2

Review

You must log in to answer this question.

Hot Network Questions

C# Security Module for generating secure hashes

2 Answers 2

Review

You must log in to answer this question.

Related

Hot Network Questions