Hashing passwords before inserting into database

Question

For the last 2 weeks I have been reading many blogs about website security and hashing passwords.

Many sites have mentioned the pro's and cons about different ways of doing this, which has left me a bit confused about how secure my code is.

If possible, can anyone have a look at the code below and let me know what they think. I have posted this question on https://codereview.stackexchange.com/questions/30814/hashing-passwords-for-website-is-this-secure but as off yet, no one has replied.

The steps I have took are as follows:

1. Create random salt
2. Add random salt and email together to create salt for password (email will be encrypted in database)

3. Hash password and salt and store in database

   public const int HashBytes = 128;
   public const int DefaultIterations = 10000;
   
       //Create random bytes for salt
       public static string SaltSHA256()
       {
           const int minSaltSize = 8;
           const int maxSaltSize = 16;
           var random = new Random();
           int saltSize = random.Next(minSaltSize, maxSaltSize);
           byte[] saltBytes = new byte[saltSize];
           var rng = new RNGCryptoServiceProvider();
           rng.GetNonZeroBytes(saltBytes);
           HashAlgorithm hash = new SHA256Managed();
           byte[] bytes = hash.ComputeHash(saltBytes);
           return Convert.ToBase64String(bytes);
       }
   
       //Create salt using email and public static string SaltSHA256()
       //Store email and public static string SaltSHA256() in database
       public static string SaltRfc2898(string email,string hashedSalt)
       {
           var salt = new Rfc2898DeriveBytes(email, Encoding.UTF8.GetBytes(hashedSalt), DefaultIterations);
           return Convert.ToBase64String(salt.GetBytes(HashBytes));
       }
   
       //Hash password and salt
       public static string PasswordHashRfc2898(string password,string salt)
       {
           var hashedPassword = new Rfc2898DeriveBytes(password, Encoding.UTF8.GetBytes(salt), DefaultIterations);
           return Convert.ToBase64String(hashedPassword.GetBytes(HashBytes));
       }
       //Get salt and password from database based on username
       //Salt in from data created by public static string SaltSHA256()
   
       public static bool DoPasswordsMatch(string password,string salt)
       {
           //Password would be pulled from db based on username
           byte[] testPassword = Convert.FromBase64String("basestring");
   
           //Salt would be pulled from database based on username
           var saltFromDatabase = salt;
   
           //Hash password and salt
           var hashUserInputAndSalt = PasswordHashRfc2898(password, saltFromDatabase);
   
           //Convert to byte[] ready for comparison
           byte[] convertUserInputFromBase64 = Convert.FromBase64String(hashUserInputAndSalt);
   
           //Compare and return true or false
           return convertUserInputFromBase64.SequenceEqual(testPassword);
   
       }
   

Answer 1

You are using Rfc2898DeriveBytes, which implements PBKDF2: that's good.

You are producing a whooping 128 bytes of output, which is total overkill for password verification. 128 bits, i.e. 16 bytes, are sufficient. 128 bytes don't harm, except that they make for bulkier strings to store in your database.

You are using 10000 iterations, which might be a bit low. Higher is better. 10000 is not bad, but chances are that your server can handle ten times as many, and this would make attacks ten times harder. I suggest that you try several values, and make the count as high as is tolerable on your system, relatively to the expected workload (namely, the number of connecting users per second).

Encoding issues are best kept separate, at the storage level. The salt, the PBKDF2 output, and the password are all sequences of bytes. Rfc2898DeriveBytes can encode a String password for you, but expects the salt to be "some bytes", and produces an output which is "some bytes". For storage into the database, either use the abilities of that database to process binary data, or apply some encoding, like Base64. So, use Convert.FromBase64String() and Convert.ToBase64String() before writing and after reading, respectively. Your mixture of Base64 and UTF-8 is confusing and makes reading (auditing) your code a harder task.

Your salt generation is weird:

• There is no point in making the salt length variable. Just go for 16 bytes.
• There is no point in avoiding bytes of value 0. A salt is a sequence of bytes, not of characters, and 0 is not special.
• There is no point in hashing the salt.

A proper salt for PBKDF2 is a sequence of at least 8 bytes, and unique (as much as possible). 16 bytes filled with the output of a strong PRNG are a good way to do that. A GUID would also work (salts need not be random, they just need to be reused as rarely as possible by normal systems, and GUID do that). In .NET, if you want to generate a good salt, simply do that:

byte[] salt = Guid.NewGuid().ToByteArray();

Alternatively, Rfc2898DeriveBytes can generate the salt for you. Upon user registration or password change, do this:

Rfc2898DeriveBytes h = new Rfc2898DeriveBytes(password, saltSize, iterations);
byte[] hashedPassword = h.GetBytes(hashSize);
byte[] salt = h.Salt;
// then store salt and hashedPassword in the database

and, upon verification:

byte[] salt = ... // read from database
Rfc2898DeriveBytes h = new Rfc2898DeriveBytes(password, salt, iterations);
byte[] hashedPassword = h.GetBytes(hashSize);
// compare hashedPassword with the stored value

You can invoke a separate PRNG like RNGCryptoServiceProvider() and do things yourself; that's what you do, and though it is weird, it is not weak. However, it is simpler to use what the system already provides. Simpler code usually means less bugs.