6
\$\begingroup\$

This is my first attempt to do any hashing. I wanted to make a class that handled everything I needed to implement PBKDF2. The only question I have is am I ok to store the iterations in the database record or is this a bad practice? I was planning on creating a column to store salt, another for the hashed password, and possibly one for the amount of iterations. My thought with the iterations was that I read that it should increase every so often so when creating new users or they update their passwords it would pull the value to pass in from the database.

    public static byte[] GenerateSalt()
    {
        using (var randomNumberGenerator = new RNGCryptoServiceProvider())
        {
            var salt = new byte[32];
            randomNumberGenerator.GetBytes(salt);

            return salt;
        }
    }

    public static byte[] HashPassword(byte[] password, byte[] salt, int iterations)
    {
        using (var rfc2898 = new Rfc2898DeriveBytes(password, salt, iterations))
        {
            return rfc2898.GetBytes(32);
        }
    }

    public static bool CompareByteArrays(byte[] array1, byte[] array2)
    {
        if (array1.Length != array2.Length)
        {
            return false;
        }

        for (int i = 0; i < array1.Length; i++)
        {
            if (array1[i] != array2[i])
            {
                return false;
            }
        }

        return true;
    }
\$\endgroup\$
  • \$\begingroup\$ use LINQ SequenceEqual rather than your CompareByteArrays. Simple to use, has the same "short circuit". \$\endgroup\$ – Jesse C. Slicer Jul 16 '18 at 20:27
  • \$\begingroup\$ @JesseC.Slicer Depends what OP wants performance-wise. This version will potentially be much quicker, because the LINQ version is a generic. Depends on what the OP values, but it might be beneficial to keep this version. \$\endgroup\$ – Der Kommissar Jul 16 '18 at 21:06
4
\$\begingroup\$

The code provided might fulfill the functionality however the API is hard to use.

Passwords are usually strings and not binary (byte[]). It would also be nice that the salt could be generated automatically when you hash the password. That way you would call a single method to hash a password instead of calling two methods, in this case that would be GenerateSalt followed by HashPassword.

According to my argument I would suggest you to implement the following API:

// returns hashed password + salt
Tuple<byte[], byte[]> HashPassword(string password, int iterations) 
bool ComparePassword(string attempt, byte[] hashedPassword, byte[] salt)

You can always replace the Tuple type by another class that would have the properties byte[] HashedPassword and byte[] Salt. Also often, but not always, the salt and the password are usually base64 encoded. That usually allows you to have a slightly nicer view of the data that is stored while also avoiding storing binary data in your persistence mechanism.

Additionally Rfc2898DeriveBytes outputs 20 bytes. So return rfc2898.GetBytes(32); should be return rfc2898.GetBytes(20);

Answering your additional question about the iteration count, there is nothing wrong with that. You can even use that as a pepper, different users can have slightly different iteration counts. Which makes the job of a hacker harder. In some scenarios people also store the actually algorithm that was used to hash the password. That might allow you to switch between algorithms in a easier fashion.

\$\endgroup\$
  • \$\begingroup\$ I also always allow one to pass an optional Encoding, just in case the default isn't what they want. \$\endgroup\$ – Der Kommissar Jul 16 '18 at 19:41
  • \$\begingroup\$ That is all the code I need to do PBKDF2 hashing or am I missing something? This is my first time doing it and it seemed like very little code. \$\endgroup\$ – Brandon D Jul 16 '18 at 20:07
  • \$\begingroup\$ @BrandonD yeah it is indeed easy to hash a password. You gotta appreciate the APIs that are rovided for you. If I am not mistaken the only thing that is wrong on your code is that the output of Rfc2898DeriveBytes is 20 bytes and not 32 bytes. owasp.org/index.php/Using_Rfc2898DeriveBytes_for_PBKDF2 \$\endgroup\$ – Bruno Costa Jul 16 '18 at 20:57
  • \$\begingroup\$ @BrunoCosta That's why you should always specify a HashAlgorithmName (I always specify SHA512), because PBKDF2 in .NET is broken (go figure). \$\endgroup\$ – Der Kommissar Jul 16 '18 at 21:04
  • 1
    \$\begingroup\$ @BrunoCosta IIRC different iteration counts per-user don't make things all-that-much harder on the attacker, assuming the attacker has a copy of the database. Everything they need to build the password is in there. If brute-force, still no benefit, all it does is change how long it takes depending on the user. \$\endgroup\$ – Der Kommissar Jul 16 '18 at 21:08
4
\$\begingroup\$

I've finally had a chance to make an answer to this, but I want it noted that Bruno Costa had a great answer, and this is only an improvement on his.

First and foremost:

The only question I have is am I ok to store the iterations in the database record or is this a bad practice? I was planning on creating a column to store salt, another for the hashed password, and possibly one for the amount of iterations.

This is 100% acceptable, and encouraged. It's necessary, especially to change the iteration count (which you should be doing annually), as you'll want to increase it a little regularly. (There's other guidance out there for how often and how much to increase it, but basically do what is necessary to keep the user cost low-ish, and keep attackers having to work for it.)

My thought with the iterations was that I read that it should increase every so often so when creating new users or they update their passwords it would pull the value to pass in from the database.

You should increase the iteration count when necessary, and do the rehashing on user-login. Forget about making them change their password, you can change their iteration and salt each time they login (as at that moment you absolutely must know the password anyway). I even go out-of-the-way to change the salt each time users login to my websites.


Now some notes on the other answer:

While Rfc2898DeriveBytes only gives you 20 bytes of entropy (as indicated), I always use one of the constructors that takes a HashAlgorithmName, and I always provide HashAlgorithmName.SHA512. This gives you entropy up to 64 bytes, so you can use your 32, or go all the way up to the 64. (That's up to you.)

Additionally:

Answering your additional question about the iteration count, there is nothing wrong with that. You can even use that as a pepper, different users can have slightly different iteration counts. Which makes the job of a hacker harder. In some scenarios people also store the actually algorithm that was used to hash the password. That might allow you to switch between algorithms in a easier fashion.

There are some good and bad points here (alright, I shouldn't say "bad", just less optimal):

  1. DO store the ITERATION_COUNT definitely;
  2. DO store the HASH_ALGORITHM_NAME at your discretion (probably worth doing so);
  3. DO NOT intentionally vary ITERATION_COUNT per user, this offers no advantage to securing the user, especially if the attacker gets a copy of the database, and only complicates your code and introduces potential vulnerabilities;
  4. DO consider adding an efficient random delay to prevent side-channel analysis and timing attacks;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.