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

Secure custom password hashing

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

Problem

My team and I have ended up creating this class, which is called directly from ASP.NET Identity as a custom password hasher. I'd like to know whether this would be "overkill"/use a lot of CPU, specially because the site is going to be hosted in Azure.

To take into account:

  • HashPassword is called when a new user is being created by Identity



  • VerifyHashedPassword is called when a user logs in



```
private const int SaltByteLength = 16;
private const int DerivedKeyLength = 20;
private const int minIterationCount = 44000;
private const int maxIterationCount = 50000;

public string HashPassword(string password)
{
return CreateSecurePasswordHash(password);
}

public PasswordVerificationResult VerifyHashedPassword(string hashedPassword, string providedPassword)
{
providedPassword = Convert.ToBase64String(ComputeHash(providedPassword));
bool result = ComparePasswordHashes(providedPassword, hashedPassword);
return result ? PasswordVerificationResult.Success : PasswordVerificationResult.Failed;
}

private static byte[] ComputeHash(string password)
{
using (MemoryStream ms = new MemoryStream())
using (StreamWriter sw = new StreamWriter(ms))
{
sw.Write(password);
sw.Flush();
ms.Position = 0;

using (SHA512CryptoServiceProvider provider = new SHA512CryptoServiceProvider())
return provider.ComputeHash(ms);
}
}

private static string CreateSecurePasswordHash(string password)
{
byte[] hashedPassword = ComputeHash(password);
byte[] salt = GenerateSecureSalt();
Random rand = new Random();
int iterationCount = rand.Next(minIterationCount, maxIterationCount);
byte[] hashValue = GenerateSecureHashValue(hashedPassword, salt, iterationCount);
byte[] iterationCountBtyeArr = BitConverter.GetBytes(iterationCount);
byte[] valueToSave = new byte[SaltByteLength + DerivedKeyLength + iterationCountBtyeArr.Length];
Buffer.BlockCopy(salt, 0, valueToSave, 0, SaltByteLength);

Solution

At first glance, your code seems to do the right thing security wise and is reasonable performance wise. If performance becomes a problem, you can still reduce the iteration count of pbkdf2. But considering the original recommandation in RFC 2898 (at least 1000 iteration) in 2000 and the increase in processing power since then, 50000 iteration seems pretty conservative to me.

It's also worth considering that your user will enter his password only once per session so the performance cost of checking a password will probably be negligible.

But as always with performance question the best answer is to bench your code and decide wether its performance is acceptable or not.

P.S. I'm not a security expert so don't make any critical decision based on my opinion

Context

StackExchange Code Review Q#115953, answer score: 2

Revisions (0)

No revisions yet.