2
\$\begingroup\$

Does this look like a good method to hash a user password? I'm not very familiar with how this whole hashing process works and below is a bunch of stuff I've seen on many different posts.

My main concern are all of the numbers. Do they all need to match? Do they need to match something like the array.copy?

And then the array.copy lines. I just want to try and understand it better before I move forward with it further.

public const int SALT_BYTE_SIZE = 16;
public const int HASH_BYTE_SIZE = 20;

[ChildActionOnly]
private static void CreateHash(string password)
{
    // Generate Random Salt
    RNGCryptoServiceProvider csprng = new RNGCryptoServiceProvider();
    byte[] salt = new byte[SALT_BYTE_SIZE];
    csprng.GetBytes(salt);

    // hash value
    var pbkdf2 = new Rfc2898DeriveBytes(password, salt, 10000);
    byte[] hash = pbkdf2.GetBytes(20);

    // combine salt + hash
    byte[] hashBytes = new byte[HASH_BYTE_SIZE];
    Array.Copy(salt, 0, hashBytes, 0, 16);
    Array.Copy(hash, 0, hashBytes, 16, 20);

    // Convert hash into string to DB storage
    string savedPasswordHash = Convert.ToBase64String(hashBytes);

    // save to DB

}
\$\endgroup\$
  • \$\begingroup\$ Both RNGCryptoServiceProvider and Rfc2898DeriveBytes implement IDisposable, so you should be wrapping each of their lifetimes in using statements. \$\endgroup\$ – Jesse C. Slicer Feb 20 '14 at 23:07
  • \$\begingroup\$ Rfc2898DeriveBytes has a built in salt generator. \$\endgroup\$ – CodesInChaos Feb 21 '14 at 10:25
  • \$\begingroup\$ @CodesInChaos Does that allow me to grab the salt for storage in the DB? I'll have to look into that and see what it does. Thanks. \$\endgroup\$ – James Wilson Feb 21 '14 at 15:32
  • \$\begingroup\$ hashBytes is an unfortunate name since it contains more than just hash. This amalgamation is generally called digest. \$\endgroup\$ – dfhwze Jun 5 at 4:50
2
\$\begingroup\$

Looks fine for the most part. The main thing here is to make sure that the function to generate and compare the hash with existing matches what you have.

As far as Array.Copy, check out MSDN for this particular overload.

Since you set the salt and hash size to 16 and 20 accordingly, what it's doing below is copying the value from salt and hash into the hashBytes array.

Array.Copy(salt, 0, hashBytes, 0, 16);
Array.Copy(hash, 0, hashBytes, 16, 20);

hashBytes will store bytes from salt on index 1-15, and the next line, bytes from hash will be stored on index 16-35.

The problem with this is your byte[] hashBytes = new byte[HASH_BYTE_SIZE] is being set to 20 which is not equal to the size of the salt (16) and hash (20) combined. It will NOT work since the size of the array is not big enough to hold the range of values being stored.

It should be below to accommodate the combined size of the hash and the salt:

byte[] hashBytes = new byte[SALT_BYTE_SIZE + HASH_BYTE_SIZE];
\$\endgroup\$
-2
\$\begingroup\$

No, your code does not look good.

Since you did not mention that your code is for personal research, I am assuming it is for getting work done. For such a case it contains too many low-level details. Just use an existing password hashing library and be done with the whole task. Your code should then become one-liners:

var hashed = PasswordHashing.Hash(password);

and

if (PasswordHashing.Check(hashed, password)) { … }
\$\endgroup\$
  • \$\begingroup\$ That's kind of harsh. Isn't Rfc2898DeriveBytes the equivalent of 'PasswordHashing' here? He is using existing APIs both for generating the salt and hash. \$\endgroup\$ – dfhwze Jun 4 at 17:50
  • \$\begingroup\$ No, it isn't. When using bcrypt, the code is really a single line. When using bcrypt, there is no hard-coded 10000 or 20 or 16 or another 16, or any mention of Base64. Each of these numbers is a potential source of errors. \$\endgroup\$ – Roland Illig Jun 4 at 20:14
  • \$\begingroup\$ There are indeed packages with less overhead than built-in .NET Framework. As long the framework doesn't foresee an equivalent facade, hiding these remaining details, there is nothing wrong combining the salt and hash yourself. The main argument I like his approach is he used best practices without using third party libraries. \$\endgroup\$ – dfhwze Jun 5 at 4:48

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.