HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Secure password hashing

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
hashingsecurepassword

Problem

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;

///
/// Creates a salted PBKDF2 hash of the password.
///
/// The password to hash.
/// The hash of the password.
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);
}

///
/// Validates a password given a hash of the correct one.
///
/// The password to check.
/// A hash of the correct password.
/// True if the password is correct. False otherwise.
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

Solution


  • 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.



-
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.

  • SlowEquals is a bad name. I'd call it ConstantTimeEquals.



  • 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.



  • 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.

Context

StackExchange Code Review Q#32856, answer score: 12

Revisions (0)

No revisions yet.