3
\$\begingroup\$

I share below my password hashing implementation with salt and pepper in C#, for your review and feedback. In my implementation methods like ValidatePassword(), ChangePassword() are in the user class.

I recommend random.org's free random bytes generator coded in hexadecimal to get a secure Pepper for each installation. I prefer 128 bytes length. It separates each byte (every 2 digits in hexadecimal) with space. Copy-paste the pepper generated by random.org to the config file with the with the key "Pepper" like so:

<add key="Pepper" value="25 4d 58 42 05 ba 0a f0 8a 72 .. .. e3"/>

The simplified source code:

using System;
using System.Text;
using System.Configuration;
using System.Globalization;
using System.Security.Cryptography;

namespace CryptoPassword
{
    public static class Password
    {
        private const int SaltLength = 32;
        private const int KeyLength = 32;
        private const int IterationCount = 100000;

        private static readonly byte[] Pepper = ConvertSpacedHexToByteArray(ConfigurationManager.AppSettings["Pepper"]);
        //I recommend random.org's free 128 byte random bytes generator coded in hexadecimal
        //to get a secure pepper for each installation.
        //Copy-paste the pepper generated by the random.org to the config file with the key "Pepper"
        //like so: "<add key="Pepper" value="25 4d 58 42 05 ba 0a f0 8a 72 .. .. e3"/>"


        /// <summary>
        /// hexString should have its double hex digits corresponding to each byte separated by ' '
        /// like so: "A1 23 F3 14".
        /// </summary>
        /// <param name="hexString"></param>
        /// <returns>byte[] form</returns>
        public static byte[] ConvertSpacedHexToByteArray(string hexString)
        {   
            string[] hexValuesSplit = hexString.Split(' ');
            byte[] data = new byte[hexValuesSplit.Length];

            for (var index = 0; index < hexValuesSplit.Length; index++)
            {
                data[index] = byte.Parse(hexValuesSplit[index], NumberStyles.HexNumber, CultureInfo.InvariantCulture);
            }

            return data;
        }

        // I dont recommend playing with salt length a lot. Make it a const,
        // and use the non-parametrized GenerateSalt() unless you know what you are doing.
        // Change the const SaltLength at major releases.
        public static byte[] GenerateSalt()
        {
            return GenerateSalt(SaltLength);
        }
        //this parameterized version is for the flexibility at knowledgable hands.
        public static byte[] GenerateSalt(int saltLength)
        {
            using (RNGCryptoServiceProvider saltCellar = new RNGCryptoServiceProvider())
            {
                byte[] salt = new byte[saltLength];
                saltCellar.GetBytes(salt);
                return salt;
            }
        }

        public static string GenerateHash(string password)
        {
            byte[] salt = GenerateSalt();
            byte[] hash = GenerateHash(password, salt);

            return Convert.ToBase64String(salt) + ":" + Convert.ToBase64String(hash);
            //store salt and hash together with ':' as the separator, coded in Base64.
        }

        public static byte[] GenerateHash(string password, byte [] salt)
        {
            //create an hmac hash of the password using the pepper value as the key
            using (var hmac = new HMACSHA512(Pepper))
            {
                byte[] initialHash = hmac.ComputeHash(Encoding.UTF8.GetBytes(password));

                //generate a key value using pbkdf2 that will serve as the password hash
                using (var pbkdf2 = new Rfc2898DeriveBytes(initialHash, salt, IterationCount)) //int.Parse(ConfigurationManager.AppSettings["IterationCount"])))
                {
                    return pbkdf2.GetBytes(KeyLength);
                }
            }
        }

        public static bool ValidatePassword(string password, string testPassword)
        {
            string[] hashParts = password.Split(':');
            byte[] salt = Convert.FromBase64String(hashParts[0]);
            byte[] hash = Convert.FromBase64String(hashParts[1]);
            byte[] testHash = GenerateHash(testPassword, salt);

            //IMPORTANT!!! The following is required to defend against timing attacks.
            //We dont want to exit at the first byte mismatch but test every byte no matter what
            //so that timing is the same for valid and invalid passwords and
            //we dont want to leak information to the attacker about upto which byte his guess is correct.
            uint differences = (uint)hash.Length ^ (uint)testHash.Length;
            for (int position = 0; position < Math.Min(hash.Length, testHash.Length); position++)
                differences |= (uint)(hash[position] ^ testHash[position]);
            return differences == 0;
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Why not use something like Argon2/Bcrypt, but reinvent the wheel. If it is intentional, you might want to tag it as such. \$\endgroup\$ – Ludisposed Sep 1 '18 at 13:33
  • \$\begingroup\$ Unfortunately, pbkdf2 tag is not present. I think they are all good enough as of 2018. I chose pbkdf2 for compliance. It is good enough for most contexts, it has standard/compliance advantage and been tested long enough, i understand the mechanism in detail and feel confident. I am waiting for Argon2 to get wider adoption/testing and plan to use it in late 2019 or 2020. \$\endgroup\$ – Ersin TASKIN Sep 2 '18 at 12:50
4
\$\begingroup\$

Not bad at all. Some remarks:

  • The class should be parameterized and an instance should be used rather than just static methods.
  • I'd never make the class itself dependent on a configured Pepper value (it's fine for an application to be configurable, but not for a class instance).
  • The salt and pepper can be simply concatenated instead of using HMAC for the password & pepper.
  • The salt and pepper are both 32 bytes, which is a bit much; using 16 bytes for both is fine.
  • The iteration count on the other hand is on the low side and should really be configurable.
  • A lot of static functions are used. It should not be necessary to make all these public (for instance the hex decoder).
  • There should be a protocol description, which should also contain things like base 64 encoding and UTF-8 encoding.

Very clear variable and method naming. However you messed up a bit for the final method:

public static bool ValidatePassword(string password, string testPassword)

... the password is the previously hashed password and testPassword a real password? That's completely unclear.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @Maarteen: Thanks for the thorough analysis. - The real implementation is more OO and involves the User class as well. I preferred the static class and the static methods here to make the mechanism easier to read. My aim was to get review on the crypto-mechanism itself. - password, testPassword also worried me during naming. You got me:) The "testPassword" is the password to be tested (the user entered) against the "password" stored at DB. I am open to proposals for those two parameters. \$\endgroup\$ – Ersin TASKIN Mar 4 at 6:39
  • \$\begingroup\$ @Marteen, Putting the IterationCount in a config file opens the door to the attacker with write access to add an unnoticable 0 to it. Then the users cannot login and it will be very difficult to find the cause of the problem. I have seen a config file issue. It was a nightmare to resolve it. So I prefer to change such delicate parameters which change at major releases only at the source code. Just put it where the parameter consts are. \$\endgroup\$ – Ersin TASKIN Mar 4 at 6:42
  • \$\begingroup\$ You could still use a version identifier for your entire scheme and / or use a property file for that kind of configuration. Even then, I would try and make your methods as specific as possible. (Sorry, too much review, the method being too long comment was for a different code). \$\endgroup\$ – Maarten Bodewes Mar 4 at 11:25

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.