Secure password hashing

Question

I have found a password hashing article and an implementation.

Is this code secure if I increase the salt to 64 bytes, hash key size to 128 bytes and the iterations to 10000? Are there vulnerabilities or incorrectly implemented sections in this code?

I know that Rfc2898DeriveBytes is making use of HMAC-SHA1, but with a high salt bit count, the SHA1 risk is lowered (if my googling can be trusted).

Any advice or words of caution or ways to secure or correct this code would be welcome.

// The following constants may be changed without breaking existing hashes.
public const int SALT_BYTE_SIZE = 24;
public const int HASH_BYTE_SIZE = 24;
public const int PBKDF2_ITERATIONS = 1000;

public const int ITERATION_INDEX = 0;
public const int SALT_INDEX = 1;
public const int PBKDF2_INDEX = 2;

/// <summary>
/// Creates a salted PBKDF2 hash of the password.
/// </summary>
/// <param name="password">The password to hash.</param>
/// <returns>The hash of the password.</returns>
public static string CreateHash(string password)
{
    // Generate a random salt
    RNGCryptoServiceProvider csprng = new RNGCryptoServiceProvider();
    byte[] salt = new byte[SALT_BYTE_SIZE];
    csprng.GetBytes(salt);

    // Hash the password and encode the parameters
    byte[] hash = PBKDF2(password, salt, PBKDF2_ITERATIONS, HASH_BYTE_SIZE);
    return PBKDF2_ITERATIONS + ":" +
        Convert.ToBase64String(salt) + ":" +
        Convert.ToBase64String(hash);
}

/// <summary>
/// Validates a password given a hash of the correct one.
/// </summary>
/// <param name="password">The password to check.</param>
/// <param name="correctHash">A hash of the correct password.</param>
/// <returns>True if the password is correct. False otherwise.</returns>
public static bool ValidatePassword(string password, string correctHash)
{
    // Extract the parameters from the hash
    char[] delimiter = { ':' };
    string[] split = correctHash.Split(delimiter);
    int iterations = Int32.Parse(split[ITERATION_INDEX]);
    byte[] salt = Convert.FromBase64String(split[SALT_INDEX]);
    byte[] hash = Convert.FromBase64String(split[PBKDF2_INDEX]);

    byte[] testHash = PBKDF2(password, salt, iterations, hash.Length);
    return SlowEquals(hash, testHash);
}

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

/// <summary>
/// Computes the PBKDF2-SHA1 hash of a password.
/// </summary>
/// <param name="password">The password to hash.</param>
/// <param name="salt">The salt.</param>
/// <param name="iterations">The PBKDF2 iteration count.</param>
/// <param name="outputBytes">The length of the hash to generate, in bytes.</param>
/// <returns>A hash of the password.</returns>
private static byte[] PBKDF2(string password, byte[] salt, int iterations, int outputBytes)
{
    Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(password, salt);
    pbkdf2.IterationCount = iterations;
    return pbkdf2.GetBytes(outputBytes);
}

Answer 1

1. Number of iterations is pretty low. Since .NET's PBKDF2 implementation is very slow, you can't afford a good number of iterations. But even with it, 20000 should be affordable for server side hashing. For client side hashing you can go much higher.

2. You're outputting more than the natural size (20 bytes for SHA-1), which decreases performance twofold without a security gain. At the same cost you could double the number of iteration, which improves security.

   => Increasing the output size beyond 20 bytes decreases security for a given hashing cost.

3. SlowEquals is a bad name. I'd call it ConstantTimeEquals.
4. No point in having large salts. Salts should be globally unique. 16 bytes is enough for that, so going beyond that doesn't increase security.
5. There are no known weaknesses in SHA-1 (or even MD5!) that apply to password hashing. Collision attacks are no threat in this context. The most important distinction between different hashes is the relative software (CPU) vs. hardware (GPU, FPGA, ASIC) performance. So on a 64 bit CPU, SHA-512 is probably a decent choice since it relies on 64 bit arithmetic.

Answer 2

First, the code is instantiating a couple of IDisposable class objects, and therefore, they should be properly disposed of with a using statement:

    public static string CreateHash(string password)
    {
        byte[] salt = new byte[SALT_BYTE_SIZE];

        // Generate a random salt
        using (RNGCryptoServiceProvider csprng = new RNGCryptoServiceProvider())
        {
            csprng.GetBytes(salt);
        }

        // Hash the password and encode the parameters
        byte[] hash = PBKDF2(password, salt, PBKDF2_ITERATIONS, HASH_BYTE_SIZE);
        return PBKDF2_ITERATIONS + ":" + Convert.ToBase64String(salt) + ":" + Convert.ToBase64String(hash);
    }

and

    private static byte[] PBKDF2(string password, byte[] salt, int iterations, int outputBytes)
    {
        using (Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(password, salt))
        {
            pbkdf2.IterationCount = iterations;
            return pbkdf2.GetBytes(outputBytes);
        }
    }

Second, naming conventions in C#. C# abhors SCREAMING_CAPS. Rename as such:

    // The following constants may be changed without breaking existing hashes.
    public const int SaltByteSize = 24;
    public const int HashByteSize = 24;
    public const int PBKDF2Iterations = 1000;

    public const int IterationIndex = 0;
    public const int SaltIndex = 1;
    public const int PBKDF2Index = 2;

Third, the signature of SlowEquals can be made more generic and accept a IList<byte> instead of byte[] (adhering to the "program to interfaces" maxim):

    private static bool SlowEquals(IList<byte> a, IList<byte> b)
    {
        var diff = (uint)a.Count ^ (uint)b.Count;

        for (var i = 0; (i < a.Count) && (i < b.Count); i++)
        {
            diff |= (uint)(a[i] ^ b[i]);
        }

        return diff == 0;
    }

Fourth, and this is a matter of personal preference, I like var because it can eliminate the boilerplate of noisy variable declarations as such:

    public static bool ValidatePassword(string password, string correctHash)
    {
        // Extract the parameters from the hash
        var delimiter = new[] { ':' };
        var split = correctHash.Split(delimiter);
        var iterations = int.Parse(split[IterationIndex]);
        var salt = Convert.FromBase64String(split[SaltIndex]);
        var hash = Convert.FromBase64String(split[PBKDF2Index]);
        var testHash = PBKDF2(password, salt, iterations, hash.Length);

        return SlowEquals(hash, testHash);
    }