6
\$\begingroup\$

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:

  1. HashPassword is called when a new user is being created by Identity
  2. 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);
    Buffer.BlockCopy(hashValue, 0, valueToSave, SaltByteLength, DerivedKeyLength);
    Buffer.BlockCopy(iterationCountBtyeArr, 0, valueToSave, salt.Length + hashValue.Length, iterationCountBtyeArr.Length);
    return Convert.ToBase64String(valueToSave);
}

private static byte[] GenerateSecureSalt()
{
    using (RNGCryptoServiceProvider rngCSP = new RNGCryptoServiceProvider())
    {
        byte[] salt = new byte[SaltByteLength];
        rngCSP.GetBytes(salt);
        return salt;
    }
}

private static byte[] GenerateSecureHashValue(byte[] password, byte[] salt, int iterationCount)
{
    using (var pbkdf2 = new Rfc2898DeriveBytes(password, salt, iterationCount))
    {
        return pbkdf2.GetBytes(DerivedKeyLength);
    }
}

private static bool ComparePasswordHashes(string guess, string saved)
{
    if (string.IsNullOrEmpty(guess) || string.IsNullOrEmpty(saved))
        return false;

    byte[] passwordGuess = Convert.FromBase64String(guess);
    byte[] savedPassword = Convert.FromBase64String(saved);
    byte[] salt = new byte[SaltByteLength];
    byte[] actualPasswordByteArr = new byte[DerivedKeyLength];
    int iterationCount = savedPassword.Length - (salt.Length + actualPasswordByteArr.Length);
    byte[] iterationCountByteArr = new byte[iterationCount];
    Buffer.BlockCopy(savedPassword, 0, salt, 0, SaltByteLength);
    Buffer.BlockCopy(savedPassword, SaltByteLength, actualPasswordByteArr, 0, actualPasswordByteArr.Length);
    Buffer.BlockCopy(savedPassword, (salt.Length + actualPasswordByteArr.Length), iterationCountByteArr, 0, iterationCount);
    byte[] passwordGuessByteArr = GenerateSecureHashValue(passwordGuess, salt, BitConverter.ToInt32(iterationCountByteArr, 0));
    return ConstantTimeComparison(passwordGuessByteArr, actualPasswordByteArr);
}

private static bool ConstantTimeComparison(byte[] passwordGuessHash, byte[] savedHash)
{
    uint difference = (uint)passwordGuessHash.Length ^ (uint)savedHash.Length;

    for (var i = 0; i < passwordGuessHash.Length && i < savedHash.Length; i++)
    {
        difference |= (uint)(passwordGuessHash[i] ^ savedHash[i]);
    }

    return difference == 0;
}

All connections to my website are done strictly through TLS 1.2. Also, in the application where this code is used, absolutely no information about the user is stored, other than username, password and email. All other information is temporary and is deleted upon the user logging out.

\$\endgroup\$
11
  • 1
    \$\begingroup\$ Welcome to Code Review! Good job on your first question. \$\endgroup\$
    – SirPython
    Jan 5, 2016 at 23:26
  • 2
    \$\begingroup\$ Why have you written your own password hashing function? Why not use scrypt or something else that is well tested? \$\endgroup\$
    – biw
    Jan 5, 2016 at 23:34
  • \$\begingroup\$ Thank you Sir. @user2840324, because the application started as a learning project, that could lead to getting some money. \$\endgroup\$
    – user92754
    Jan 5, 2016 at 23:35
  • 1
    \$\begingroup\$ This seems to be using SHA-512 for the hashing, so the hashing per se probably isn't a problem. \$\endgroup\$ Jan 6, 2016 at 1:12
  • 1
    \$\begingroup\$ By "will this use a lot of CPU", you mean "I require this to use a lot of CPU", right? If you do not understand why a password hasher should use enormous amounts of CPU then you have not thought sufficiently about the security problem that your security algorithm intends to solve. Fast password hashers are a security risk; never try to make one faster! \$\endgroup\$ Jan 10, 2016 at 14:51

2 Answers 2

2
\$\begingroup\$

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

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Would it make it better (performance wise) if I did another check on ConstantTimeComparison before the loop? Since it can only return true if the first operation is 0, seems somewhat useless to do the loop when the first operation didn't return 0 (because of different lengths) \$\endgroup\$
    – user92754
    Jan 6, 2016 at 17:37
  • \$\begingroup\$ @cFrozenDeath optimizing ConstantTimeComparison will be useless. If you run your code through a profiler you'll see that pbkdf2.GetBytes() account for more than 99% of the execution time. So if you're worried about performance, reduce the iteration count \$\endgroup\$
    – Mareek
    Jan 7, 2016 at 8:32
  • 1
    \$\begingroup\$ Thanks for that comment. I'll take it into account if performance becomes a problem. And seeing that you're the only one who dared to answer, I'll take your answer as accepted \$\endgroup\$
    – user92754
    Jan 7, 2016 at 13:14
0
\$\begingroup\$

I'm interested to know what you think you're achieving with a random work factor (iteration count). You want to set it as high as you possibly can you're saying 'I can afford to compute '50000' iterations (max) but I'll sometimes use only '44000' iterations'. That's not improving security, you're making some of your hashes faster to compute. If you can afford 50000 iterations, you should use it all the time not a random amount.

As someone pointed out, the cost of comparing your byte arrays is going to be fairly insignificant compared to computing the hash. Therefore, you should favour readability over clever tricks.

private static bool ByteArrayEquals(byte[] x, byte[] y)
{
    if (x.Length != y.Length)
    {
        return false;
    }

    for (var i = 0; i < x.Length && i < y.Length; i++)
    {
        if (x[i] != y[i])
        {
            return false;
        }
    }
    return true;
}

I'm not a crypto expert so I'm reluctant to say much more... I can't help but feel you have over complicated this code though. I'd rip it all out and use the default implementation see here which (as the comment states) uses PBKDF2 with HMAC-SHA256, 128-bit salt, 256-bit subkey, 10000 iterations.

\$\endgroup\$
2
  • \$\begingroup\$ There's a problem with your for loop however. It's known that those kind of implementations are not totally realiable, as some arrays with one or two different bytes will still return true. At first I couldn't use the default implementation because I wasn't using Identity. The random iteration count, as I read in some post, is to make it more difficult for an attacker to determine it \$\endgroup\$
    – user92754
    Jan 8, 2016 at 12:14
  • \$\begingroup\$ Could you point me to the source for the known problem with this sort of implementation? The random work factor is still pointless: security.stackexchange.com/a/98500 \$\endgroup\$
    – RobH
    Jan 8, 2016 at 12:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.