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

Default config uses md5 to hash passwords #2936

Closed
Marv51 opened this issue Dec 10, 2019 · 6 comments
Closed

Default config uses md5 to hash passwords #2936

Marv51 opened this issue Dec 10, 2019 · 6 comments
Labels

Comments

@Marv51
Copy link

Marv51 commented Dec 10, 2019

I believe the default configuration uses salted md5 password hashing? That is no longer considered secure.

I would suggest using the password_hash ( string $password) function with the default hashing algorithm by default.

@mprins
Copy link
Contributor

mprins commented Dec 10, 2019

https://github.com/splitbrain/dokuwiki/blob/master/_test/tests/inc/auth_password.test.php shows a list of supported algo's, I'm not sure what the default is in DW but you can choose to use a more secure algo.
using the default PHP hashing is definitely a recipe for disaster, as it says:

PASSWORD_DEFAULT - Benutzt den bcrypt-Algorithmus (Standard in PHP 5.5.0). Beachte, dass sich diese Konstante mit der Zeit ändern wird, wenn stärkere Algorithmen in PHP implementiert werden. Aus diesem Grund kann sich die Länge des zurückgegebenen Strings mit der Zeit ändern. Es wird deshalb empfohlen das Ergebnis in einem Datenbankfeld zu speichern, das mehr als 60 Zeichen speichern kann. (z.B. 255 Zeichen).

@mprins
Copy link
Contributor

mprins commented Dec 10, 2019

yes, it seems that salted md5 is the default:
https://github.com/splitbrain/dokuwiki/blob/6e0d35582815257160c7d90ff45f03cb2088834b/conf/dokuwiki.php#L58

@phy25 phy25 added the Security label Dec 10, 2019
@Marv51
Copy link
Author

Marv51 commented Dec 11, 2019

For inspiration maybe you could look at the Symfony implementation? https://github.com/symfony/symfony/blob/c732122b574ecfb083d44324ac18f9508e95471d/src/Symfony/Component/Security/Core/Encoder/NativePasswordEncoder.php

The native PHP methods store the password in an encoded form like this (first and second part are algo and parameters used for hashing): $argon2i$v=19$m=1024,t=2,p=2$YzJBSzV4TUhkMzc3d3laeg$zqU/1IN0/AogfP4cmSJI1vc8lpXRW9/S0sYY2i2jHT0

Of course php has corresponding password_verify.

This enables the upgrade of the hashing algorithm after the initial user is created.

A better default like bcrypt would also solve this problem for quite some time I think.

@phy25
Copy link
Collaborator

phy25 commented Dec 11, 2019

There is already support for bcrypt:

https://github.com/splitbrain/dokuwiki/blob/924cc11c61f9b6b0b947b36046ae4deb179dcb33/inc/PassHash.class.php#L509-L526

The thing is that we need to change the default. I guess smd5 was kept for a long time due to PHP compatibility issue on 5.x.

@splitbrain
Copy link
Collaborator

Yep. I'm happy to switch the default to whatever is deemed more secure. It should basically be a single line change. Just pick what you like and commit ;-)

@phy25
Copy link
Collaborator

phy25 commented Dec 11, 2019

Done. Thanks for raising the issue @Marv51!

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

No branches or pull requests

4 participants