4
\$\begingroup\$

I have spent about 2 weeks reading about hashing passwords and website security. As there seems to be many different ways to achieve this, I'm a bit confused as to whether my code is secure.

Can anyone please have a look through and let me know what they think and what needs to be changed?

I have not added any error trapping yet, as to allow me to concentrate on just the hashing.

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);

    }
\$\endgroup\$
  • \$\begingroup\$ RNGCryptoServiceProvider, SHA256Managed and Rfc2898DeriveBytes all implement the IDisposable interface and therefore should be wrapped in using statements. \$\endgroup\$ – Jesse C. Slicer Sep 5 '13 at 13:59
  • \$\begingroup\$ Hi Jesse thanks forgot about that, code updated \$\endgroup\$ – George Phillipson Sep 5 '13 at 17:14
  • \$\begingroup\$ It looks like your SaltSHA256 method generates a random saltBytes array. Won't this be different every time your method is called? It just seems like one instance will never be able to access a previous instance, so how will you ever be able to check a password later? I'm probably missing something. \$\endgroup\$ – jp2code Aug 26 '16 at 13:55
4
\$\begingroup\$
  1. Random length salt is silly

  2. Hashing the salt with SHA-256 is silly. Makes it longer without increasing entropy.

    A salt just needs to be unique. Best practice is using RNGCryptoServiceProvider to get 16 random bytes. Using more than 16 bytes doesn't improve security.

  3. It's a bad idea to derive more than 20 bytes with Rfc2898DeriveBytes since that only decreases performance on your system, but not for the attacker. Deriving 20 bytes should speed your code up 7x. More that 20 bytes certainly doesn't improve security, I'd probably even go down a bit. 15 and 18 bytes encode nicely without padding using Base64.

  4. If you can afford the performance hit, increase the number of iterations a bit. If you could afford your original code, you should now be able to afford 70000 iterations thanks to the speedup from the previous suggestion.

  5. Don't use UTF-8 encoding on the salt. You can pass it to Rfc2898DeriveBytes directly as a byte[]. When you really need to convert it to/from text, use Base64.

  6. It's best practice to use a constant time comparison instead of SequenceEquals to avoid timing side-channels.

\$\endgroup\$
  • \$\begingroup\$ Good tips, especially the DeriveBytes and increased iteration advice. \$\endgroup\$ – Dan Sep 5 '13 at 18:26
  • \$\begingroup\$ Hi CodesInChaos, can you please explain number 6, I'm not sure what you mean by constant time comparison. Thanks George \$\endgroup\$ – George Phillipson Sep 5 '13 at 22:46
  • \$\begingroup\$ Using advanced timing attacks, described here, it may be possible to deduce the approximate character position at which the password match fails based on how long the SequenceEquals comparison takes. This article explains the problem and potential solutions in more detail. It is worth noting that the effort required execute a timing attack is high, especially against a poorly optimized website with varying page load times. If timing attacks are your weakest link, you're doing well. \$\endgroup\$ – Dan Sep 12 '13 at 19:56
4
\$\begingroup\$

As CodesInChaos pointed out in the comments, a far more secure way to hash passwords is to use a deliberately expensive hash. This will all but eliminate any hope of a successful brute-force attack.

The "best" algorithm I know of for this purpose would be BCrypt. For .NET, you can use BCrypt.Net. Disclaimer: I don't know who, if anyone, has verified the integrity of this particular port.

See this StackOverflow question for a full discussion regarding the use of BCrypt as a password hashing algorithm.


Original answer

So long as the salt is sufficiently unpredictable, the hashing function sufficiently secure, and the user's password is never stored in plain-text, emailed, or logged, then your implementation should be adequate.

While your salt is unpredictable, 8 bytes may be short for extremely sensitive information. A salt is stored in plain-text along with the hashed password, so we're not worried about keeping it a secret. What we're worried about is that someone may have already pre-computed SHA-256 hashes for common passwords and salts up to 8 bytes salt (though unlikely). Having a random length salt really doesn't add much except computational complexity and potentially wasted storage space. I would settle for a consistent-width salt of 16 bytes.

SHA-256 is a suitable hashing algorithm for most purposes at this time.
Edit: But BCrypt is better for passwords (see above).

\$\endgroup\$
  • \$\begingroup\$ Hi Dan, thanks for the info, Tom who replied on security.stackexchange.com also mentioned using a constistent salt, so I will be modify code. \$\endgroup\$ – George Phillipson Sep 5 '13 at 14:18
  • 2
    \$\begingroup\$ SHA-256 by itself isn't suitable for password hashing. You need some form of iterated scheme, such as Rfc2898DeriveBytes \$\endgroup\$ – CodesInChaos Sep 5 '13 at 18:01
  • \$\begingroup\$ I didn't say it was suitable by itself for password hashing. I said it was a suitable hashing algorithm. The context of the discussion should imply that additional steps, e.g. salting, are required. \$\endgroup\$ – Dan Sep 5 '13 at 18:23
  • 2
    \$\begingroup\$ People use bad solutions like SHA2(salt||password) all the time, so mentioning SHA-2 without mentioning that you need to iterate it at least 10k times is dangerous IMO. In practice most .net programmers are best served with PBKDF2-HMAC-SHA-1 since that's the only standard password hash in the BCL. \$\endgroup\$ – CodesInChaos Sep 6 '13 at 9:31
  • \$\begingroup\$ @CodesInChaos Sorry I didn't incorporate your feedback sooner. There's no excuse really. I've updated the answer to mention that computationally expensive algorithms should be preferred for password hashing and linked to BCrypt. \$\endgroup\$ – Dan Aug 20 '15 at 21:19

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.