Take the 2-minute tour ×
Information Security Stack Exchange is a question and answer site for Information security professionals. It's 100% free, no registration required.

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 http://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);
    
        }
    
share|improve this question
add comment

1 Answer

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.

share|improve this answer
1  
Hi Tom, thanks for the reply, after reading a lot of blogs about security and hashing I got a bit confused. Some blogs say do this and some do that. As you have now read my code and explained a few things, it makes it eaiser for me to understand. –  George Phillipson Sep 5 '13 at 14:14
    
Good overall feedback. +1 –  Dan Sep 5 '13 at 14:26
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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