7

Could you please tell me if the following is a good way to securely hash a password to be stored in a database:

    public string CreateStrongHash(string textToHash) {

        byte[] salt =System.Text.Encoding.ASCII.GetBytes("TeStSaLt");

        Rfc2898DeriveBytes k1 = new Rfc2898DeriveBytes(textToHash, salt, 1000);
        var encryptor = SHA512.Create();
        var hash = encryptor.ComputeHash(k1.GetBytes(16));

        StringBuilder sb = new StringBuilder();
        for (int i = 0; i < hash.Length; i++) {
            sb.Append(hash[i].ToString("x2"));
        }

        return sb.ToString();

    }

Many Thanks in advance.

ScubaSteve2012
  • 117
  • 4
  • 4
    Ideally I think you should have a different salt per user, and store that in the database too. – Bridge Aug 31 '12 at 16:02
  • 1
    Sorry if I am being a bit dense, but if someone gains access to the database, does storing the salt not make it easier for someone to crack the encryption? – ScubaSteve2012 Aug 31 '12 at 16:07
  • @ScubaSteve2012 The salt prevents pre-computed hash attacks ("Rainbow tables"). Because the passwords are salted, your rainbow table would be useless in helping figure out the original passwords. You would have to create a new rainbow table using this hash, which is pretty much a brute-force attack. – Oleksi Aug 31 '12 at 16:11
  • 1
    @ScubaSteve2012 No, keeping a different salt for each user makes it *harder* to crack the encryption, and storing the salt doesn't make it any easier or harder to crack the encryption. If someone gains access to the database and tries to brute-force passwords, keeping a different salt for each user means that each brute-force attack gets only one password, not all the passwords. Knowing the salt doesn't help very much, because once the attacker has the password with salt he knows that the user's real password is a substring of the password + salt combination. – Adam Mihalcin Aug 31 '12 at 16:12
  • Excellent, thanks for that. It makes sense now. So if I move the iteration up to 100,000 say and use a salt per user, do you think this would be fairly secure? Someone also suggested using bcrypt instead, would this be generally agreed upon? – ScubaSteve2012 Aug 31 '12 at 16:17
  • 1
    Your variable name betrays some confusion. SHA512 is not an encryption algorithm. It is also a hash algorithm. –  Aug 31 '12 at 16:18

2 Answers2

7

You use PBKDF2-SHA1, which is decent but not great. Bcrypt is a bit better, and scrypt is even stronger. But since .net already includes a built in PBKDF2 implementation, that's an acceptable choice.

Your biggest mistake is that you didn't get the point of a salt. A salt should be unique for each user. It's standard practice to simply create a random value of at least 64 bits. Store it together with the hash in the database.

If you want to, you can split the salt into two parts. One stored in the database alongside the user, which is different for each, and one shared part stored elsewhere. This gains the the advantages of both.

I also recommend using a higher workfactor than 1000. Figure out what performance is acceptable, and adjust accordingly. I wouldn't go below 10000, and in some situations(disk encryption) a million is acceptable too.

CodesInChaos
  • 106,488
  • 23
  • 218
  • 262
  • If the salt value is stored, as such, in the database, it's of very little help to prevent finding a collision. Its main utility in that case is preventing an attacker discovering by inspection that two users have the same password. It would be better to use data that is not specifically labelled as a "salt", or even better, that is not contained in the same data store; the attacker must then not only compromise the database containing the hashes, but the codebase and/or an additional data store, in order to determine what to use as a salt and how. – KeithS Aug 31 '12 at 17:24
  • @KeithS I never mentioned the word collision, because collisions are irrelevant here. The main point of a salt is being unique, and thus preventing multi-target attacks. Adding a secret that's stored in separate locations is a nice extra, but not the main point of a salt. – CodesInChaos Aug 31 '12 at 17:28
5

It can be improved. First, you should use bcrypt instead. Traditional hashes like SHA-512 can be broken fairly easily with GPUs now-a-days. The problem is that these hashes are designed for speed, and this is the opposite of what you want in a password hash. Bcrypt is an example of an adaptive hash algorithm. It can be configured to take a "long" time (but still won't cause performance issues in your system) to make brute-forcing diffiult.

You also want to make the salts unique for each user.

For more information on how to securely hash passwords, see this question.

Community
  • 1
  • 1
Oleksi
  • 12,947
  • 4
  • 56
  • 80
  • 3
    He's using PBKDF2, which is a bit worse than bcrypt, but still one of the recommended functions. – CodesInChaos Aug 31 '12 at 16:03
  • SHA-512 is literally "good enough for government work"; it is regarded as cryptographically strong and there is no known "shortcut" reducing the complexity of finding the solution. The fact that any problem can be solved "faster" by throwing enough hardware at it is irrelevant; you would have to try 5.2 × 10^72 values before having a 1 in a *billion* chance to collide with any previously-generated hash. – KeithS Aug 31 '12 at 16:19
  • 1
    @KeithS Plain SHA-512 is not considered good enough for applications like password hashing, because it's too fast. In password hashing you use deliberately expensive functions to compensate for the lack of entropy in a password. | PBKFDF2 with SHA-512 on the other hand is decent. – CodesInChaos Aug 31 '12 at 16:23
  • Thanks for the comments, I have tested the code with a work factor of 100000 in the PBKFDF2 and there is a small delay while it works but not so much that would cause a problem. I will take on the other comments and create a random salt when a user is first created to store with the hash in the db. – ScubaSteve2012 Aug 31 '12 at 16:29
  • In password hashing you should be safeguarding the hash against theft, and salting each one with unique "secret data" (as in someone who was able to grab your entire DB should either not have the salts or not know what the salts are). Salting *dramatically* increases entropy of even stupid passwords like "god" and "123", unless your attacker knows how you salt them (the hashing process should not be done client-side, even if the code is obfuscated). – KeithS Aug 31 '12 at 16:32
  • In addition, pass *words* are a really stupid idea. You should encourage pass *phrases*; four random common dictionary words have MUCH more entropy than a single l33t-encoded word (the "gold standard" of password obfuscation), and they're easier for users to remember. – KeithS Aug 31 '12 at 16:34
  • @KeithS The standard assumption is that the salt is not secret, after all your server has just been compromized. Sometimes you manage to keep part of it secret, often you don't. Salt and key-strengthening are designed to make it harder to crack the passwords, even if you didn't manage to keep anything secret from the attacker. Your common 4 dictionary word password is not secure unless significant strengthening is applied. – CodesInChaos Aug 31 '12 at 16:44
  • ... Really? http://xkcd.com/936/ I would posit pretty much any random search of the English dictionary for four words to use as a passphrase would be similar to the example provided. You could, I'm sure find anecdotal counter-examples, but with a quarter-million English words to choose from I doubt very much that you'd end up with four three-letter words (and such a case is trivial to plan for; enforce minimum word length or overall length). – KeithS Aug 31 '12 at 17:15
  • @KeithS If their entropy estimate is correct (44 bits), its trivial to brute-force without strengthening. Without strengthening this sounds like a day of work for a standard graphics card. | A lot depends on how many words you consider, but at about 64 bits of entropy brute-force becomes expensive. | And you have no chance in hell to get an average user a password complexity anywhere near that, unless you generate a random password on the server, and force them to use it. – CodesInChaos Aug 31 '12 at 17:20
  • @KeithS you can't design a security system that relies on people choosing very very good passwords, because this _never_ happens. While PBKDF2 is fine, plain SHA-512 is not good enough for password hashing (with or without a salt). Any hacker with a GPU can brute-force a large chunk of your user's SHA-512 hashed passwords within hours, maybe days. – Oleksi Aug 31 '12 at 18:12