Genoa improved security

12>>>

password storage in database hash

Tags: Security

ez	#1 2012-06-21 19:55
Contributors Thanked: 10 times	Hi, I have donated my improved security project to Trustmaster. It involves salting the stored passwords. Now the passwords are stored as an md5 hash... which can be bruteforced very easily. So the current Genoa branch can be improved. I have made improvements to the user authentication and added features to auto-salt. This means that passwords are salted on the moment people login. So the security is improved after the login. Well Trustmaster has my code for Genoa 0.6.23... Have fun ! I hope he will add it to 0.6.24... ==- I say: Keep it EZ -==

#1 2012-06-21 19:55

Contributors
Thanked: 10 times

Hi,

I have donated my improved security project to Trustmaster.

It involves salting the stored passwords. Now the passwords are stored as an md5 hash... which can be bruteforced very easily.
So the current Genoa branch can be improved.

I have made improvements to the user authentication and added features to auto-salt. This means that passwords are salted on the moment people login.
So the security is improved after the login.

Well Trustmaster has my code for Genoa 0.6.23... Have fun !

I hope he will add it to 0.6.24...

==- I say: Keep it EZ -==

Trustmaster	#2 2012-06-22 07:13
Administrators Thanked: 263 times	Thanks, ez! Here is your patch for the others if they want to test it. May the Source be with you!

ez	#3 2012-06-22 07:29
Contributors Thanked: 10 times	Please remember : Always make backups of code and DB, because this is in testing fase. I will add some more documentation later... Installation requires adding 1 field to the users table: user_salted tinyint default value 0 And after that there is a var that needs to be added to your config.php (datas) $cfg['psalt'] = ''; The saltvalue can be anything you want e.g. '12AB#' ==- I say: Keep it EZ -==

#3 2012-06-22 07:29

Contributors
Thanked: 10 times

Please remember : Always make backups of code and DB, because this is in testing fase.
I will add some more documentation later...

Installation requires adding 1 field to the users table:

user_salted tinyint default value 0

And after that there is a var that needs to be added to your config.php (datas)

$cfg['psalt'] = '';

The saltvalue can be anything you want e.g. '12AB#'

==- I say: Keep it EZ -==

GHengeveld	#4 2012-06-22 07:55
Team Thanked: 52 times	Your code seems acceptable for Genoa. I'm not a big fan of the user_salted column, since you wouldn't want to let a hacker know that the password is salted or not (even though a hacker could find out by reading the open-source code). Why you'd assign the value of this column to a tpl tag doesn't seem like a useful/secure addition either. Anyway, it's more or less what we had in mind. For readability I think some variable names should be changed (e.g. $newspass, 'psalt'). It's consistent with the terrible variable naming practices of Genoa though. I'm assuming your //2basix comments are meant to indicate where your changes are. Don't expect them to be in the final code. Cotonti is a team effort, we don't claim individual 'ownership' of any pieces of code. Modules and plugins are a different story. Added 2 minutes later: #34738 ez: The saltvalue can be anything you want e.g. '12AB#' I suggest a salt of at least 20 random characters.

GHengeveld

#4 2012-06-22 07:55

Team
Thanked: 52 times

Your code seems acceptable for Genoa. I'm not a big fan of the user_salted column, since you wouldn't want to let a hacker know that the password is salted or not (even though a hacker could find out by reading the open-source code). Why you'd assign the value of this column to a tpl tag doesn't seem like a useful/secure addition either. Anyway, it's more or less what we had in mind.

For readability I think some variable names should be changed (e.g. $newspass, 'psalt'). It's consistent with the terrible variable naming practices of Genoa though.

I'm assuming your //2basix comments are meant to indicate where your changes are. Don't expect them to be in the final code. Cotonti is a team effort, we don't claim individual 'ownership' of any pieces of code. Modules and plugins are a different story.

Added 2 minutes later:

#34738 ez:
The saltvalue can be anything you want e.g. '12AB#'

I suggest a salt of at least 20 random characters.

ez	#5 2012-06-22 09:14
Contributors Thanked: 10 times	I made this very quick.. and it is a first draft that I made. About the user_salted, maybe I can improve this code.. youre right about it (I have an idea for that, maybe I can leave the column out of the DB). The TPL Tag was just for admin purposes (if admin, then i can see if the password had changed or not) About the comments: "Don't expect them to be in the final code". I donated the code (like I wrote earlier).. so I have no problems leaving them out. I dont claim any ownership... Did you really think that....? ( These kind of remarks about this are not very motivating.. :( , next time keep them to yourself Gert ) ==- I say: Keep it EZ -==

#5 2012-06-22 09:14

Contributors
Thanked: 10 times

I made this very quick.. and it is a first draft that I made.
About the user_salted, maybe I can improve this code.. youre right about it (I have an idea for that, maybe I can leave the column out of the DB).

The TPL Tag was just for admin purposes (if admin, then i can see if the password had changed or not)

About the comments: "Don't expect them to be in the final code". I donated the code (like I wrote earlier).. so I have no problems leaving them out.
I dont claim any ownership... Did you really think that....? ( These kind of remarks about this are not very motivating.. :( , next time keep them to yourself Gert )

==- I say: Keep it EZ -==

GHengeveld	#6 2012-06-22 09:27
Team Thanked: 52 times	Sorry about that, it wasn't necessary to make a comment about that. I really do appreciate your efforts. I was thinking perhaps it's an idea to add a prefix to the hashed password instead of an extra column. The downside is that the password field has to be longer than 32 chars. Alternatively, it may be a good idea to switch from md5 to sha1/sha256, which results in a 40-char hash. Then it's possible to check the passhash string length to determine whether the user's password is already migrated to the new scheme (with salt). E.g. user_salted = (strlen(user_password) == 40)

GHengeveld

#6 2012-06-22 09:27

Team
Thanked: 52 times

Sorry about that, it wasn't necessary to make a comment about that. I really do appreciate your efforts.

I was thinking perhaps it's an idea to add a prefix to the hashed password instead of an extra column. The downside is that the password field has to be longer than 32 chars.

Alternatively, it may be a good idea to switch from md5 to sha1/sha256, which results in a 40-char hash. Then it's possible to check the passhash string length to determine whether the user's password is already migrated to the new scheme (with salt). E.g. user_salted = (strlen(user_password) == 40)

ez	#7 2012-06-22 19:43
Contributors Thanked: 10 times	Forget about my old version.... I have read more articles, and I was doing it wrong. The salt should be random on every user, so no fixed salts ! This new code uses SHA256 encryption and a 64 character salt... this is super safe. I have made the salting autoupdate... so no difficult migrations, if they login, the new, improved, password is generated. I also made the minimum password length to 6 (was 4). LINK to code: security 0623 v3 Again, this is AS IS... And I donate it to the Cotonti community (BEWARE THIS IS FOR GENOA: 0.6.23). Please test all stuff first... I might have made mistakes ! (logging in and logging out is tested) Run the SQL file to update the database first Let me know your test results ! If all goes well this should really be in 0.6.24 !!! only a md5 is just NOT safe anymore ! ==- I say: Keep it EZ -==
This post was edited by ez (2012-06-22 20:31, 12 years ago)

#7 2012-06-22 19:43

Contributors
Thanked: 10 times

Forget about my old version.... I have read more articles, and I was doing it wrong. The salt should be random on every user, so no fixed salts !

This new code uses SHA256 encryption and a 64 character salt... this is super safe.

I have made the salting autoupdate... so no difficult migrations, if they login, the new, improved, password is generated.
I also made the minimum password length to 6 (was 4).

LINK to code: security 0623 v3

Again, this is AS IS... And I donate it to the Cotonti community (BEWARE THIS IS FOR GENOA: 0.6.23).

Please test all stuff first... I might have made mistakes ! (logging in and logging out is tested)
Run the SQL file to update the database first

Let me know your test results !

If all goes well this should really be in 0.6.24 !!! only a md5 is just NOT safe anymore !

==- I say: Keep it EZ -==

This post was edited by ez (2012-06-22 20:31, 12 years ago)

Trustmaster	#8 2012-06-22 21:08
Administrators Thanked: 263 times	Sorry for sarcasm, but letting your entire database leak in first place is a lot more unsafe. May the Source be with you!

ez	#9 2012-06-23 06:45
Contributors Thanked: 10 times	you know what, forget about the whole thing... ==- I say: Keep it EZ -==

GHengeveld	#10 2012-06-23 07:19
Team Thanked: 52 times	Trust wasn't being serious. Of course the db shouldn't be hacked in the first place, but we've seen in the past that there is always a risk of SQL injection, especially in extensions. I think you're really on the right path to make this thing more secure. I like your idea about changing salts. I haven't looked at your new code yet but I suggest using the user ID and secret key hashed together as salt. Whatever you do, some part of the salt must be stored in a file rather than the db.

GHengeveld

#10 2012-06-23 07:19

Team
Thanked: 52 times

Trust wasn't being serious. Of course the db shouldn't be hacked in the first place, but we've seen in the past that there is always a risk of SQL injection, especially in extensions. I think you're really on the right path to make this thing more secure. I like your idea about changing salts. I haven't looked at your new code yet but I suggest using the user ID and secret key hashed together as salt. Whatever you do, some part of the salt must be stored in a file rather than the db.

Trustmaster	#11 2012-06-23 09:53
Administrators Thanked: 263 times	ez, I apologize and I'm very sorry that you took my note as a personal offense. I appreciate your effort and I agree that these measures are necessary. They will be definitely applied to both Genoa 0.6.24 and Siena 0.9.11. My note was only about the fact that salting passwords is a special measure that mitigates consequences of a database being hacked, it must not be considered as a panacea: a hacked database is not safe still. May the Source be with you!

Trustmaster

#11 2012-06-23 09:53

Administrators
Thanked: 263 times

ez, I apologize and I'm very sorry that you took my note as a personal offense. I appreciate your effort and I agree that these measures are necessary. They will be definitely applied to both Genoa 0.6.24 and Siena 0.9.11.

My note was only about the fact that salting passwords is a special measure that mitigates consequences of a database being hacked, it must not be considered as a panacea: a hacked database is not safe still.

May the Source be with you!

ez	#12 2012-06-23 11:32
Contributors Thanked: 10 times	Gert, Vladimir, When i try to improve stuff, and all i get is strange comments and sarcasm, the fun will quickly dissapear.... But your apologies are accepted. @Gert There is actually no need to have a public salt key... however it does make it safer i guess (if a random and a fixed salt is used). The salting is just to make databases decrypt attacks like lookup tables, reverse lookuptables and rainbow tables on the passwords worthless. The password itself is now SHA256, so bruteforcing all users will take a long time... even if they have the salt... Read this nice article: http://crackstation.net/hashing-security.htm @Trustmaster The salting is a second line of defense against identity theft (because users are often using the same passwords). Any system has security flaws... and can come from everywhere (server access, ftp, bad hosting security, flaws in e.g. MySql or PHP, sql injections..... and so on...) See the download link for the code... Cotonti can use it freely. ==- I say: Keep it EZ -==
This post was edited by ez (2012-06-23 14:47, 12 years ago)

#12 2012-06-23 11:32

Contributors
Thanked: 10 times

Gert, Vladimir,
When i try to improve stuff, and all i get is strange comments and sarcasm, the fun will quickly dissapear.... But your apologies are accepted.

@Gert
There is actually no need to have a public salt key... however it does make it safer i guess (if a random and a fixed salt is used).
The salting is just to make databases decrypt attacks like lookup tables, reverse lookuptables and rainbow tables on the passwords worthless.
The password itself is now SHA256, so bruteforcing all users will take a long time... even if they have the salt...
Read this nice article: http://crackstation.net/hashing-security.htm

@Trustmaster
The salting is a second line of defense against identity theft (because users are often using the same passwords).
Any system has security flaws... and can come from everywhere (server access, ftp, bad hosting security, flaws in e.g. MySql or PHP, sql injections..... and so on...)

See the download link for the code... Cotonti can use it freely.

==- I say: Keep it EZ -==

This post was edited by ez (2012-06-23 14:47, 12 years ago)

GHengeveld	#13 2012-06-23 13:05
Team Thanked: 52 times	Thanks for the article link. Now that I've read it, it makes more sense to me. This is exactly the reason why we like to take this thing slowly. It's better to think this thing over before we start implementing something that's insecure by design. The idea behind storing the salt in a disk file is that it's not stored as plaintext in the database, otherwise it's still useless because a hacker with access to the database would still be able to run a lookup table attack by simply adding the salt before precalculating the lookup table hashes. I understand from the article that a fixed salt like this isn't necessary, since the goal of a salt it to prevent an attacker from pre-calculating hashes from a dictionary. By using a different salt for each password, it becomes impossible to pre-calculate the hashes. An attacker can still use a dictionary, but he'd have to calculate all dictionary hashes individually for each password/salt combination, which would take considerably longer (the lookup table would be multiplied in size by the total number of users). A big advantage of storing the salt in the database instead of in config.php is the reduced risk of losing the salt (and losing all passwords in the process). It's not uncommon to accidently overwrite config.php and lose the configuration variables stored within. Perhaps we can make the 'fixed salt' optional for extremely security-minded administrators. PS. As an experiment I've ran a lookup table attack with only ~100k dictionary words on a major Cotonti website and succesfully found the plaintext passwords of almost 15% of users.

GHengeveld

#13 2012-06-23 13:05

Team
Thanked: 52 times

Thanks for the article link. Now that I've read it, it makes more sense to me. This is exactly the reason why we like to take this thing slowly. It's better to think this thing over before we start implementing something that's insecure by design.

The idea behind storing the salt in a disk file is that it's not stored as plaintext in the database, otherwise it's still useless because a hacker with access to the database would still be able to run a lookup table attack by simply adding the salt before precalculating the lookup table hashes. I understand from the article that a fixed salt like this isn't necessary, since the goal of a salt it to prevent an attacker from pre-calculating hashes from a dictionary. By using a different salt for each password, it becomes impossible to pre-calculate the hashes. An attacker can still use a dictionary, but he'd have to calculate all dictionary hashes individually for each password/salt combination, which would take considerably longer (the lookup table would be multiplied in size by the total number of users).

A big advantage of storing the salt in the database instead of in config.php is the reduced risk of losing the salt (and losing all passwords in the process). It's not uncommon to accidently overwrite config.php and lose the configuration variables stored within. Perhaps we can make the 'fixed salt' optional for extremely security-minded administrators.

PS. As an experiment I've ran a lookup table attack with only ~100k dictionary words on a major Cotonti website and succesfully found the plaintext passwords of almost 15% of users.

ez	#14 2012-06-23 14:25
Contributors Thanked: 10 times	I did what the article recommended :D, and I checked other articles too. This to make sure that storing the salt part inside the user record is not a security threat.... and it wasn't, So my code should be OK. For insane security, we could use a random hash AND just a fixed hash OR using a md5 of the main url (that is likely to change). ? So passwords only work on the same url... one disadvantage using the url is when sites migrate to other urls... BUT that is only for insane security... I think we are good just using the random hash which is safe enough ==- I say: Keep it EZ -==

#14 2012-06-23 14:25

Contributors
Thanked: 10 times

I did what the article recommended :D, and I checked other articles too.
This to make sure that storing the salt part inside the user record is not a security threat.... and it wasn't, So my code should be OK.

For insane security, we could use a random hash AND just a fixed hash OR using a md5 of the main url (that is likely to change). ?
So passwords only work on the same url... one disadvantage using the url is when sites migrate to other urls...

BUT that is only for insane security... I think we are good just using the random hash which is safe enough

==- I say: Keep it EZ -==

GHengeveld	#15 2012-06-23 14:47
Team Thanked: 52 times	The mainurl isn't secret or random, so I don't think it's a useful addition. The current idea is to use hash('sha256', $cfg['salt'].$user_salt.$user_password) with $cfg['salt'] being optional and $user_salt being randomly generated by cot_unique() when the password is changed.

12>>>