Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Password hashing procedure #1635

Open
TomVnz opened this issue Feb 6, 2018 · 2 comments
Open

Password hashing procedure #1635

TomVnz opened this issue Feb 6, 2018 · 2 comments

Comments

@TomVnz
Copy link

TomVnz commented Feb 6, 2018

as of version 1.11.3

I would expect admin password to be generated with a secure hashing function, with a suitably random salt.

This is not the case, instead the password is created using a md5 hash, which whilst salted, the salt generation is a mess, and reliant on md5 hashes to create the salt. A nice circular reference back to md5 hashes...which are just poor.

SALT.php is generated using include.php, specifically:
$sSalt = '<'.'?php //' .md5(microtime(true).rand(1000, 5000)) .md5(microtime(true).rand(5000, 9999)) .md5(microtime(true).rand(1000, 5000));

The password is then hashed using the following calculation (once all the variables are expanded):
$newhashedpassword = md5(md5($sSalt . APP_PRIVATE_DATA_NAME . $sSalt) . $newpass . md5($sSalt . APP_PRIVATE_DATA_NAME . $sSalt));
(APP_PRIVATE_DATA_NAME is by default defined as '_default_')

Would think the following would be more appropriate:
$newhashedpassword = password_hash($newpass, PASSWORD_DEFAULT)

No need to store a SALT.php, no generating multiple md5 hashes, only to hash the hash, resulting in a final md5 hash that probably isn't that great.

@ervee
Copy link
Contributor

ervee commented Feb 7, 2018

Hi Tom, thank you for your code review! I see you just signed up with Github so perhaps you're not yet familiar with all the Git stuff, but could you thy to create a pull request for this? Then the dev can just pull it in (after review)! Thanks!

@plinss
Copy link
Contributor

plinss commented Mar 15, 2020

This issue has been addressed in the above two PRs and can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants