Implement new password hashing (PBKDF2-SHA1)

[lxp]

This change modifies the database and requires a database upgrade
from within the lansuite admin area.

After the database upgrade, the old MD5 user passwords will be
automatically converted to PBKDF2-SHA1 on the next user login.
Clan and team passwords will only be upgraded, when new passwords
are set.

PBKDF2-SHA1 was choosen for compatibility with the ejabberd XMPP
server password storage.

[hnxfirefly]

This change allows an easy way to upgrade the password hash for the users.
For the Tornament Module no password updates are done, but it's not important cause for every party the teams are generated new with new passwords, and there the new hashing algorithm will be used.
For clan passwords the password will also only be updated on a change or on a new creation of a clan, but as the normal usecase the clan passwords are not typed in when sign on it's not very effective to auto update the password.

The PBKDF2-SHA1 hash algo was choosen cause it's the same password hash as the ejabberd XMPP server uses, and in the future we would like to implement an XMPP messaging back end to our lansuite page (www.hnx.at).
But the new code part allows an easy method to implement new Hashing algorithms. With the same auto update modes as used in this update.

This Features were tested on a Debian9 Server with apache2 and php7 (php7.0-fpm) and mariadb

[HnXbigblue]

Es wäre toll, wenn sich jemand für den Review vom Pull Request finden würde.
Wir benötigen die Funktionalität für unsere LanSuite Page, damit wir einen Instant Messenger in die Page integrieren können.

[t-h-e]

MD5 shouldn't even be used anymore as it has extensive security flaws and can be cracked by brute-force attack. see MD5

It should also be mentioned that this commit allows to easily add other password hasing algorithms as well.

[andygrunwald]

I will try to have a look at it during the weekend or starting next week.
One thing: I saw you are modifying the database.
Can you post the SQL-Query to ALTER existing installations? That we can include this in an upgrade guide?

[lxp]

Thank you, that would be great!
We are trying an upgrade scenario tomorrow and provide feedback.

[lxp]

We didn't retry the upgrade scenario yet, but during development we considered the upgrade path.
All changes use already existing LanSuite mechanisms, so no manual database interaction is required.

Here is a short scenario how it should work:

• State: old code, old database, MD5 is used

1. Upgrade code

• State: new code, old database, MD5 is used

2. In "DB-Manager" click "Datenbank updaten und verwalten" / "Update / Manage database"

• State: new code, new database, MD5 is used

3. In "DB-Manager" click "Menueinträge neu schreiben" / "Rewrite Navigation"

• State: new code, new database, PBKDF2-SHA1 is used

4. If you want to switch to a different algorithm, you can switch "Passwort Hashing Algorithmus" in section "Sicherheit und Anti-Spam" on "Admin-Seite"
   Currently supported:
   • PBKDF2-SHA1 (kompatibel mit ejabberd)
   • MD5 (Legacy, DANGEROUS!)

The only scenario I know of that will break the installation is if you click "Menueinträge neu schreiben" / "Rewrite Navigation" before "Datenbank updaten und verwalten" / "Update / Manage database".

[lxp]

I rebased the pull request onto latest master.

[M4LuZ]

The only scenario I know of that will break the installation is if you click "Menueinträge neu schreiben" / +"Rewrite Navigation" before "Datenbank updaten und verwalten" / "Update / Manage database".

That is the only bit that concerns me.
I think we are missing a way to do non-SQL-related (pre-/post-)update tasks.
That shouldn't block this change, but I would feel better if we had a way to run whatever we need without relying on the user running steps in the correct order. Raised issue #90 to discuss this.

[lxp]

Yeah, the current update mechanism of LanSuite does not fit our requirements.
However, I still opt for merging the change now, as getting the new update mechanism ready, especially with the full requirements defined in the issue will take some time.

[Dwarfex]

I thought SHA-1 was broken?

[lxp]

The SHAttered attack only showed that SHA-1 does not fulfil the collision resistance property, which is necessary for a cryptographic hash.
However, for password hashes the collision resistance property is not really relevant.
For password hashes the pre-image resistance is the most important property and how the hash function is used, as cryptographic hashes are not designed for password storage per default.

PBKDF2 is a good variant to use a hash function for password storage.
As already described above, we are sticking to PBKDF2-SHA1 because of the interoperability with ejabberd.
This does not mean we have to use PBKDF2-SHA1 as default for LanSuite in the future.
We are interested in PBKDF2-SHA1, so we implemented it.
It is also a lot more secure than the old pure-MD5 hash, so we suggest to use it as default for now.
If you are interested in an even better algorithm, you can easily implement it after merging this pull request, because we also introduce a new abstraction layer for password storage.

[andygrunwald]

Thanks @lxp and sorry for the late reply. I will try to check this PR out in the next days.
Just to let you know that this is still on my road map.

[lxp]

@andygrunwald Any progress on this?

[M4LuZ]

@andygrunwald Any progress on this? ;)

[andygrunwald]

@M4LuZ Sadly not. Quite busy the recent days. And I want to finish the rework of the last 4 modules. Then i will work in this here.

[m-ober]

In case moving away from MD5 is still an open topic, I suggest using the native password_hash , password_verify and password_needs_rehash functions provided by PHP. They are secure and: "never roll your own crypto".

[hnxfirefly]

Well PBKDF2-SHA1 is also an open implementation from php we just picked this one because it's the same xmpp uses and mainly for a singlesignon feature.
The most part of the code is for compatibility reasons to automatically update the users password on signein without needing to create a new password or a password reset.
The password hash class is able to use different hash methods. And as the design of the module it is suteable to switch to new hash methods in a live environment.

[m-ober]

@hnxfirefly Is using xmpp together with lansuite really that common? I don't know much about xmpp, but "single sign on" usually means doing something with oauth.

Anyway, you can also do silent "upgrades" of the hash using password_hash() et al.

You can even re-hash existing md5 passwords, so all users have an immediate benefit. Like so:

1. Hash all md5 passwords using password_hash(). Store that fact in an extra column (e.g. boolean "legacy_hash")
2. Once a user logs in, check that column
   2a. True: Before passing the user-entered password to password_verify, run it through md5()
   2b. False: Use password_verify
3. In case of 2a. use the entered clear-text password, run it through password_hash, update the password hash in the db, set the "legacy_hash" column to false

Well PBKDF2-SHA1 is also an open implementation from php

I don't see you are using any library, there is just a class PasswordHash. Who wrote that class? Where does it come from? Even the smallest mistake in an implementation can ruin everything. Thus I would recommend using either a well known, well reputated library or password_hash etc.

[hnxfirefly]

@m-ober
the class PasswordHash does the checking if an old passwordhash is used and then the hashing with the php function hash_pbkdf2 if needed and converts the output into an single string compatible with the password_hash function.
It is designed to be extended with new pasword hash funktions if something is outdated again.
The cryptography is done in the hash_pbkdf2 function.
and meant to be extended with new password hashing functions like password_hash()

the class is written by @lxp and then checked by @t-h-e and myself.

And it would be nice to be extended with new hashing methods as sha1 is by now already outdated.

[m-ober]

Thanks for clarifying, guess I read that code a bit too fast ;)

[t-h-e]

I updated the password hash functionality change to work with the current master.
As the original change is rather old, let me know if you intend to merge it. If not, it is fine as well. Then we will keep it in our repository as we have one or two changes that are not of any interest to the main lansuite repo.

[andygrunwald]

First, thanks a lot for this Pull Request. I updated the branch, and I was reading through this issue. It is a significant change in the right direction. Here are a few notes.

Hashing algorithm

md5 is out of the equation. Old, insecure, and should not be used for passwords anymore.

The PBKDF2(SHA512) function is acceptable with at least 10,000 iterations (as recommended by NIST). There are some reports that attacks against PBKDF2-SHA1 are starting to be successful (e.g., https://mjg59.dreamwidth.org/66429.html). Still, it is way better than md5.

argon2id is the current gold standard in the password storage space. I prefer to implement this one as well. See https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id

Changelog

We should add a CHANGELOG.md (or UPGRADE.md) in one of the future PRs, not in this one. Primarily to highlight what is covered and what are the limitations.

## New password hashing algorithm

After the database upgrade, old MD5 user passwords will be automatically converted to PBKDF2-SHA1 on the next user login. 
For clan and team passwords, the password will also only be updated on a change or on a new creation of a clan, but as the normal use-case, the clan passwords are not typed in when signing on; it's not very effective to auto-update the password.

PBKDF2-SHA1 was chosen for compatibility with the ejabberd XMPP server password storage.

### Limitations

No password updates are done for the Tournament Module, but it's not essential because for every party, the teams are generated new with new passwords, and there the new hashing algorithm will be used.

### How to upgrade

1. Upgrade code
2. In "DB-Manager" click "Datenbank updaten und verwalten" / "Update / Manage database"
3. In "DB-Manager" click "Menueinträge neu schreiben" / "Rewrite Navigation"
4. If you want to switch to a different algorithm, you can switch "Passwort Hashing Algorithmus" in section "Sicherheit und Anti-Spam" on "Admin-Seite"
   Currently supported:
   - PBKDF2-SHA1 (kompatibel mit ejabberd)
   - MD5 (Legacy, DANGEROUS!)

Open questions

1. @lxp @hnxfirefly @t-h-e Are you still using ejabberd and XMPP on your site and aim for compatibility? (If yes, I am OK with supporting PBKDF2-SHA1. If not, I would change it to argon2id)

What is left to get this merged

I am happy to take this PR on and get this over the finish line.

1. Answer the open question
2. Code-Review by me
3. Verifying the upgrade procedure and testing this change

Future work

Seen as aftermath, once this is merged:

1. Implementing argon2id as a possible password-hashing variant (Using the native password_hash, password_verify and password_needs_rehash functions provided by PHP)
2. Adding a Changelog and Upgrade guide to the documentation
3. Implementing a "silent upgrade function like described in Implement new password hashing (PBKDF2-SHA1) #83 (comment) by @m-ober

[andygrunwald]

I do not favor making this class with a collection of static functions. I think we could benefit more from streamlining the scope of this class a bit more by e.g., removing the dependency on the configuration and injecting the configuration into the constructor.

This would make unit testing easier as well.

I can take this on and modify the class / this PR.

[andygrunwald]

I am more in favor of

• splitting the long configuration parameter a bit for readability like password_hash_algo
• not using abbreviations and making it explicit, because we are not limited in space here like password_hashing_algorithm

I can take this on to change it.

[andygrunwald]

Same here with the long explicit name.
I can take this on

[andygrunwald]

Same here with the long explicit name.
I can take this on

[andygrunwald]

I am also thinking of adding unit tests and if we should switch away from the self-written class towards a symfony component like https://github.com/symfony/password-hasher or https://github.com/nette/security/

There seems to be an option to make it compatible with this implementation.

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
4 Security Hotspots
26 Code Smells

No Coverage information
4.8% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
4 Security Hotspots
26 Code Smells

No Coverage information
4.8% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[sonarcloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
4 Security Hotspots
19 Code Smells

No Coverage information
4.8% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[lxp]

argon2id is the current gold standard in the password storage space. I prefer to implement this one as well. See https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id

I agree with that.

Open questions

1. @lxp @hnxfirefly @t-h-e Are you still using ejabberd and XMPP on your site and aim for compatibility? (If yes, I am OK with supporting PBKDF2-SHA1. If not, I would change it to argon2id)

We are still using this implementation (with PBKDF2-SHA1) on our site. However, we are not using ejabberd anymore.
So, we could move to argon2id. However, for this to be viable, we still need to support PBKDF2-SHA1 on our site.

I understand that you might not want to support the PBKDF2-SHA1 algorithm and just go for argon2id, since argon2id is a good decision from the security standpoint.
However, since we need to support PBKDF2-SHA1 on our site for migration purposes, we just want to ask you to make the interface extensible, so that we can easily add PBKDF2-SHA1 support in our fork without re-writing the whole codebase.

Edit: What I forgot to add:

This pull request implements the classic upgrade schema that works with any algorithm changes, which we might need in the future anyway. For example, to improve on the argon2id settings. However, it has the drawback that if users do not log-in the insecure md5 hashes stay in the database.

There is also an upgrade schema that allows to instantly migrate all users.
It works by chaining the new secure algorithm with the old insecure algorithm. i.e. argon2id(md5(password)).
Since we still want to get away from the md5 algorithm entirely, I would still keep the classic upgrade schema in addition and migrate users that are logging in from argon2id(md5(password)) to argon2id(password).

We did not implement this upgrade schema, since it introduces more complexity and needs even more testing.
However, this can even be implemented in a second step some time after implementing the classic upgrade schema.

The OWASP Password Cheatsheet has also a section about this problem:
https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#upgrading-legacy-hashes

[sonarcloud]

Quality Gate failed

Failed conditions
4 Security Hotspots
3.4% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[M4LuZ]

Just a note that I've started work on this to finally merge it.

Given the age of the master-branch the PR is based on and the ton of commits that would need to be merged I've created a new branch with the original PRs.

Regarding the choice of algorithm:

• SHA1 is also a bad choice by now, we should not set this by default
• SHA256 and SHA512 are also supported by the SCRAM protocol that ejabberd uses, thus I would vote for making SHA512 the standard
• I've already created default cases that also support any other algo that hash_pbkdf2() supports, the question is just what we expose as config option.
• argon2id is a nice thought, but a bad default choice as it requires explicit compilation into PHP. If anyone really wants it, it can be implemented by extending the switch-blocks without major changes, but I won't spend time on it
• The strongest crypto is picking a strong password in the first place. We need to ensure that the password hash is not broken till users can react and change it where needed, not to ensure that they cannot be broken for a decade. So we may want to enfore minimum requirements, but at the end of the day the achieved security level it is up to the user as long as we do our due diligence here.
• Converting users by chaining MD5+PBKDF2 together: I'm in favour of that, as it appears that it can be covered just by adding a dedicated switch-statement, is still way better than what we have and users are then transparently migrated on first login as the KDF does not match what is set for the installation. and thus user-pw is updated.

[andygrunwald]

@M4LuZ I agree with your comments - Totally make sense. Also big thanks for #1114, I will review it.

I think we should implement and rely on https://github.com/symfony/password-hasher.
They support:

• bcrypt
  sodium
  PBKDF2
  Or create a custom password hasher
  out of the box.

With custom password hasher, we can implement whatever we want. See https://symfony.com/doc/current/security/passwords.html#creating-a-custom-password-hasher

It also supports password migration, see https://symfony.com/doc/current/security/passwords.html#password-migration

On top of this, it takes care to mark sensitive function arguments to avoid leaking those into stack traces and logs, see https://github.com/symfony/password-hasher/blob/7.0/Hasher/SodiumPasswordHasher.php#L72C52-L72C74

It also does all the extension and function checking, see https://github.com/symfony/password-hasher/blob/7.0/Hasher/SodiumPasswordHasher.php#L65

I do not favor using a weaker password algorithm by default, just because some external component does not support a newer one (jabber). What I suggest is two new config variables (in inc/base/config.php):

• password_hash_algorithm
• previous_password_hash_algorithm

With those two configs, we

• offer the possibility for lansuite admins to choose their password algo (e.g. to match jabber)
• can migrate password hashes from MD5 to password_hash_algorithm

What are your thoughts on this?

[M4LuZ]

I think we should implement and rely on https://github.com/symfony/password-hasher.

That does make sense mid to long term.
But I don't see the need or opportunity to rewrite code (yet again) to implement it in scope of this PR.

I do not favor using a weaker password algorithm by default

So do I, thus the selection of SHA512 instead of SHA1.
Yes, there are also other algorithms (bcrypt also appears to provide better protection against GPU hashes), but if it it is trusted and good enough for NIST, then it is good enough for us.

password_hash_algorithm is already the name of the related password configuration option in the new PR.

Adding previous_password_hash_algorithm does not make sense, as:

• There may be a mix of multiple hash configurations in the DB
• The content of the field already provides the algo and additional config (password field contains a value of $<algorithm>$<iterations/other config options>$<hash>)
• That option alone does not reflect a change in iteration count for PBKDF2 or memory / CPU usage for bcrypt / argon2id
• Having that as fixed configuration setting forces the user to change configuration at the point of change instead of taking care of it transparently