14

For all of my hobby web projects, I have coded a login system that these projects share. There is no critical data in these projects, the only critical data could be reused passwords. So I try to hash passwords in a very secure way.

My current password hashing algorithm is nothing I would call really secure. And as I saw that PHP now offers build in hashing with password_hash, I thought about switching.

My question is, what are the benefits of switching for me? In terms of, what extra protection do I get, what hole(s) do I fix, etc.? And is there anything else I have to do to not make it extremely insecure?

I would like to use the answers as a learning experience for myself so I can teach it to others who are in my position.

My current algorithm (pseudo-code):

Salt = "some-salt" 
Repeat 1000000 times
    Password = Sha512(Salt + username + Salt + Password)

I know my site is vulnerable to timing attacks and the PHP built-in functions would fix that.

What else is obviously insecure?

Chris
  • 672
  • 7
  • 12
reggaemuffin
  • 251
  • 2
  • 9
  • 21
    Whatever you do, read [How to securely hash passwords?](http://security.stackexchange.com/questions/211/how-to-securely-hash-passwords/31846#31846) – Gilles 'SO- stop being evil' Sep 12 '14 at 12:50
  • 8
    It's better than nothing (kudos for at least thinking about the problem), but in general pbkdf2/bcrypt/scrypt with decent settings is always better than something you came up with yourself. – Luc Sep 13 '14 at 09:59
  • 4
    TL;DR **yes**, it is insecure. Don't roll your own hashing algorithm – Francisco Presencia Sep 13 '14 at 13:31
  • He doesn't use his own hashing algorithm, he's obviously using sha512. In my opinion it is pretty safe if you only run it 2 or 3 times. Running it 1000000 is just not efficient. –  Sep 13 '14 at 13:39
  • @Tim runnig only 3 turns would be easyly broken with brute force. My algorithm runs 1,5 seconds, but i would say, at least let it run for 0,1 second... – reggaemuffin Sep 13 '14 at 15:34
  • 7
    The answer to "is my built-by-an-amateur-working-alone crypto system insecure against at least one attack by arbitrarily many determined smart attackers with resources at their disposal?" is *yes*. The details of the system are irrelevant. – Eric Lippert Sep 14 '14 at 00:40
  • 9
    "Is my custom _______ insecure?" Almost certainly. – Tim S. Sep 14 '14 at 02:11
  • See http://crypto.stackexchange.com/questions/959/strength-of-multiple-hash-iterations – alecail Sep 14 '14 at 14:07
  • 2
    If you think it's not insecure and actually advances the state of the art, you should have submitted it to https://password-hashing.net/. If not, then use something that's well-tested and aligned with existing best practices. – R.. GitHub STOP HELPING ICE Sep 14 '14 at 16:02
  • @Kostronor No. It is *not* easy to bruteforce this algorithm even if the attacker would know the algorithm. Note: You get this runtime on *one* function run, but expect >1000 people would run it the same time, than you would see what I mean with efficient. –  Sep 15 '14 at 16:59

6 Answers6

33

For starters you rolled your own piece of password hashing algorithm. There are currently but three password hashing algorithms which are considered secure:

  • PBKDF2
  • scrypt
  • bcrypt

You are iterating and you are appending a salt. You also seem to add the username within the algorithm, which does not add any benefit as you are already using a salt.

What I can't deduct from your code and which is very important is the length of your salt and how you generate it. A salt should never be hardcoded, but secure randomly generated when hashing the password. The salt should be unique within the database and preferably globally unique as to avoid identification of similar passwords in different databases (hence a username is not considered a good salt).

Also the way PBKDF2 works (which leans closest to your implementation) is by using a HMAC where the random generated salt is the key used to calculate the message digest, rather than appending everything together. PHP's password_hash is using the bcrypt algorithm.

So to summarize what you gain by using a standard algorithm instead of your own:

  • You will generate random salts
  • You will use a standardized and secure password hashing algorithm
Jeff Ferland
  • 38,170
  • 9
  • 94
  • 172
Lucas Kauffman
  • 54,229
  • 17
  • 113
  • 196
  • 6
    Another pro for using a built-in method: If flaws are found in the algorithm, you might get noticed by a security bulletin, and you'd probably get a fixed version without much work. – Jost Sep 12 '14 at 09:43
  • My current salt is around 32 characters long and not randomly generated. It only is some piece of legacy code from when rainbow tables were an issue. The username was my idea on how to make the salt more unique. But you are right about it not being a good salt. Can you elaborate a bit more (or link me to trusted resources, crypto sadly has a strong opinion when searching for comparison) about what would be the best algorithm for me to choose? My hashing function is isolated and can be changed easily. – reggaemuffin Sep 12 '14 at 11:08
  • 2
    PBKDF2 is NIST and FIPS approved, scrypt is theoretically stronger than bcrypt but less vetted. Either way PHP's function uses bcrypt, so I'd just go with the built in PHP function. All crypto resources can be found on this site (search button at the top) look for Thomas Pornin's answers. – Lucas Kauffman Sep 12 '14 at 12:08
  • @Lucas thanks a lot for your response, will look for his answers! – reggaemuffin Sep 12 '14 at 12:51
  • @LucasKauffman: Do any of those methods make it possible to increase the hash strength of an existing hashed key without needing the original password? Conceptually it would seem like that should be possible if the password table keeps a list of what hashes should be done on a password to yield a particular value. Atomically adding a hash method to the list of actions, feeding the expected result through that hash method, and storing the result of that method into the database, should yield a "more-strongly-hashed" record, without having to have the original password. – supercat Sep 12 '14 at 18:17
  • 1
    @supercat Yes, you can use old_hash_func(password) as the "password" for the improved hash, for old accounts that haven't logged in yet. – user10008 Sep 13 '14 at 10:15
  • Another thing you gain is convenience, the password_hash() function will pack all necessary parameters into the resulting hash-value (salt / cost factor / algo). This string you can store in a single database field. – martinstoeckli Sep 13 '14 at 16:49
  • "Considered secure" - by whom? Citation needed. – Nate Eldredge Sep 15 '14 at 00:53
18

Assuming that some-salt is in fact never repeated, this is an acceptable password hashing method. It combines the three essential elements: no recovery of the hash from the passwords (thanks to the use of SHA-512), intrinsic slowness (due to the high number of SHA-512 iterations), and a unique salt.

The salt needs to be globally unique: it must not be used to hash other passwords, to hash the same password on a password change, or in somebody else's password database. Violating uniqueness is not absolutely critical, but the more it happens, the easier you make it for an attacker. At the extreme of the range, the “salt” is constant, it isn't a salt and your password hashing method is awful. If it's the user name or the record id, it's still bad because multiple databases will share a small set of salts.

Nonetheless, you should use a standard hashing method. There may be a vulnerability in your method that we missed because it rested on some subtle detail. You may have made a mistake in the implementation; a well-supported implementation with security updates is preferable for this reason. A weakness may be discovered in the algorithm; again, it is better to use a well-supported implementation whose implementer keeps abreast of cryptography news and will change the algorithm if necessary.

One thing you need to take care is to increase the number of iterations as time goes by and computers become faster. A good

Read How to securely hash passwords? (yes, all of it). Bcrypt is somewhat superior to PBKDF2, because it is more resistant to GPU-based attacks; PBKDF2 follows the same principles as your custom hash. Using PBKDF2 is not a major vulnerability at this point but I would nonetheless recommend switching to bcrypt or scrypt.

Community
  • 1
Gilles 'SO- stop being evil'
  • 51,415
  • 13
  • 121
  • 180
16

Your algorithm is better by miles than many "homebrew" ones. The reason why I'd still switch to a standardised one is you get the benefit of all the scrutiny that these standards have received which your own construction has not.

Is there definitely no mistake in your construction that allows a trivial break? If not, are you sure your algorithm isn't vulnerable to an accelerated brute-force attack using massively parallel GPUs? Could there be an optimised form of rainbow table attack on your particular algorithm?

The answer is probably no, no and no but more people have looked at PBKDF2 than at your algorithm, so there's less likely to be something nasty that no-one's spotted so far. This is a heuristic argument but don't forget, even if your algorithm is good (and it certainly doesn't look stupid to me), there's hundreds of other people out there who have implemented their own crypto which turned out to be horribly broken. That's why we say as a rule, don't roll your own.

It's the same reason that we don't allow just anyone to install and modify high-voltage electrical connections: having an electrician's qualification doesn't magically protect you from electrocution but it means you've hopefully got enough experience in the area not do do anything too stupid like grab hold of a random wire and see if it's live or not.

8

One drawback to including the username as part of the salt is it means you cannot rename a user (e.g. administratively) without generating a new password hash (which means you'd have to pick a new password and tell the user, or ask the user for their current password).

nobody
  • 365
  • 1
  • 7
2

I can see a few problems with your approach.

  • You appear to be using a constant salt value. It is more secure to generate a new salt for each password.
  • Including username in the salt can be problematic. As pointed out by Andrew, it means you cannot rename a user. It does partially mitigate the problem with a constant salt. But it does not fully mitigate the problem, because the username doesn't change each time the password changes.
  • You only include the password in the first iteration. There is a theoretical risk that entropy from the password slowly disappears as you iterate.
  • High iteration count may make you vulnerable to DoS attacks.

Iteration count is a double-edged sword. High iteration count makes your server vulnerable to DoS attacks. Low iteration count makes your password database vulnerable to offline brute force attacks. The only way you can protect yourself from both of these is to offload part of the computation to the client. But this requires code on the client. In a web service such code could be delivered by javascript, but javascript is not as fast as native code, so the attacker would have an advantage over legitimate use.

kasperd
  • 5,442
  • 1
  • 19
  • 38
  • Your reminder on DOS is really good! When switching I thought to make the function simply run 5 seconds to be extremely secure. But good to know that could also become an attack vector, thanks! – reggaemuffin Sep 13 '14 at 10:18
2

First of all, from a password security perspective you have a pretty good algorithm.

Your salt is technically not a salt, since it is the same for every user, but when you combine it with the username it is a unique piece of information, thus you do have the complete security of a salt. If your "some-salt" string is cryptographically strong (at least 64 bits of entropy), it also works as a pepper.

From a convenience point of view you'd generally want to avoid featuring the username in the password hash algorithm as that makes it complicated to change the username, and you will need the plain text password for the operation. A dedicated salt is easier to manage.

A million hashing iterations seems a bit over the top, it could easily become the bottleneck of a busy website, I'd scale it back to a level where you can do at least 1000 password hashes per second.

Also make sure that you have a mechanism to throttle login attempts, if an attacker can simply burn 1000 login attempts per second he can quickly steal a lot of weak password user accounts, or he can use the password hashing as a denial of service weapon.

I don't think there is any widely agreed upon best practice for login throttling, except that no throttling is pretty much the worst possible solution.

Community
  • 1
aaaaaaaaaaaa
  • 1,027
  • 6
  • 8