4
\$\begingroup\$

I'm creating a custom MembershipProvider for an ASP.NET MVC5 application and am wanting to know if this code is acceptable for creating hashed and salted passwords. Is there anything I can do to improve it?

EncodePassword is passed the plaintext password from the override functions.

I picked 38 hash iterations arbitrarily and I honestly don't know if it's even necessary or valuable. Should it be higher?

_machineKey is auto generated by ASP.NET for each machine/VM the software runs on. I will probably remove this to allow a distributed system and use an application key instead. Though, looking through the documentation for HMACSHA1, would it even seem like this key may be necessary?

EncodePassword() result will be stored directly in the database.

    private bool CheckPassword(string password, string dbpassword)
    {
        var salt = GetSalt(dbpassword);
        password = EncodePassword(password, salt);

        if (password == dbpassword)
        {
            return true;
        }

        return false;
    }

    private string EncodePassword(string password, string salt = "")
    {
        if (string.IsNullOrEmpty(salt))
            salt = CreateSalt();
        var hash = new HMACSHA1 { Key = HexToByte(_machineKey.ValidationKey) };
        var bytePassword = hash.ComputeHash(Encoding.Unicode.GetBytes(password + salt));

        for (int i = 0; i < 38; i++)
        {
            bytePassword = hash.ComputeHash(bytePassword);
        }
        return String.Format("{0}:{1}", bytePassword, salt);
    }

    private static string CreateSalt(int size = 64)
    {
        using (var rng = new RNGCryptoServiceProvider())
        {
            var buff = new byte[size];
            rng.GetBytes(buff);
            return Convert.ToBase64String(buff);
        }
    }

    private static string GetSalt(string password)
    {
        var passwordSplit = password.Split(':');
        if (passwordSplit.Length == 2)
        {
            return passwordSplit[1];
        }

        return String.Empty;
    }
\$\endgroup\$
4
\$\begingroup\$

This is a pretty good start: you've got salting done pretty well (RNGCryptoServiceProvider is one of the most common recommended was to generate a salt), and the idea of using multiple iterations is a good one.

However, doing 38 iterations of HMAC SHA1 yourself is not really going to help you much. The point of iterations is to slow down someone who is trying to brute-force passwords by making it take longer to do so. SHA1 is designed to be fast (it has to be in order to be used for message authentication "at scale"), so doing it multiple times doesn't slow you down all that much.

Instead, consider using an algorithm purpose-built for password hashing, like PBKDF2, bcrypt, or scrypt. They are built on the same cryptographic hash functions, but are tuned for the specific purpose of password hashing. (They also typically recommend more than 1000 iterations, significantly more than your 38.) The easiest thing to do is to use a library like CryptSharp, SimpleCrypto.net, BCrypt.net, or many others.

UPDATE PBKDF2 is built in to the .NET Framework using the Rfc2898DeriveBytes class. Here is how to use it (based on your EncodePassword function:

private string EncodePassword(string password, string salt = "")
{
    int numIterations = 1000;

    if (string.IsNullOrEmpty(salt))
        salt = CreateSalt();

    using (var pbkdf2 = new Rfc2898DeriveBytes(password, Encoding.UTF8.GetBytes(salt), numIterations))
    {
        var hash = pbkdf2.GetBytes(64);
        return String.Format("{0}:{1}", Convert.ToBase64String(key), salt);
    }
}

Another thing you could do to "future-proof" your hashing is to store the number of iterations and the encryption method along with the password. That way, if you decide to change the number of iterations used (for example, you may wish to do so if/when computing power gets to the point that a few thousand iterations isn't slow enough) you won't have to invalidate all existing passwords all at once. For example, PHP's password_hash function (which uses bcrypt, not PBKDF2 - but the principle is the same) stores a code representing the hash function used and the number of iterations along with the salt and the password. (For an easy look at how they do it, see this implementation.)


(A general word of advice: it is very tempting to roll your own password encryption. Resist the urge and Just Don't Do It.)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks, I have no intention of making my own cryptographic hash, I know that would end badly. I'm trying to avoid anything outside of the base .NET (but will if it's necessary), would HMACSHA512 with more iterations be a safer mechanism? Or should I still look at PBKDF2 etc? \$\endgroup\$ – siva.k Jan 6 '14 at 19:41
  • \$\begingroup\$ As an added comment about rolling my own, this will sit behind the standard ASP.NET login system and is a drop in replacement method for UserManager. So all of the security before it will be left intact. \$\endgroup\$ – siva.k Jan 6 '14 at 19:58

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.