Conversation

edited by dregad

Today, the MD5 hash algorithm is very weak and Mantis's current hash algorithm does not use salt and key stretching.
The hashed password that created by current algorithm is seriously weak. (The hashed password that stored in database maybe contained in a rainbow-table, or may calculates quickly with CPU and/or GPU)

This pull-request adds new password hashing algorithm and switch default to it.
As of today, new algorithm uses bcrypt via PHP-built-in password_hash() function. (This function available from PHP 5.5)
If bcrypt becomes "weak", the PHP team (or the distros' maintainer) will upgrade algorithm to safer one. Then, stored hashed-password will upgrade automatically when user logged in after update PHP engine. This means "this new algorithm is maintenance-free."

Backward compatibility info: The hashed-password that created by MD5 or other older algorithms are still accepted for authentication. Older password will upgrade automatically when first log in.

Note: The password_hash() function requires PHP 5.5+ and Mantis's current minimal requirement is 5.5.9+. But, the documentation that linked from download page says "5.3.2+" (it looks for Mantis v1.3.x, but Mantis website links to that version now). If we should support PHP 5.3.2-5.4.x, we should include the ircmaxell/password_compat library. (This PR does not so.)

As you may have noticed, I'm not good at English. I feel sorry for my difficult-to-read English sentences. I will saved when feedback is simple and easy English.

This comment has been minimized.

Hello @fetus-hina many thanks for your contribution, this is greatly appreciated as I've had this on my todo-list for ages, but somehow never got around to coding it. I will review your pull request later.

Considering that the CRYPT_BLOWFISH hashing algorithm that we now use
as default to encrypt passwords truncates the the hash at 72 chars [1],
we must limit the users' input accordingly.
Allow 1024 chars for the password is not really useful (the value was
picked arbitrarily), so we now use 72 as the maximum password length
for all password hashes (PASSWORD_MAX_SIZE_BEFORE_HASH).
[1] http://php.net/function.crypt

Salt option is deprecated in PHP 7.0; it is recommended to use the salt
that is generated by default, which is stored as part of the hash.
The fallback to crypt() to generate a password with a given salt is
therefore unnecessary.

This comment has been minimized.

@fetus-hina I rebased your branch on top of latest master, made a few adjustments and updated documentation. Many thanks for your work, let me know if you have any objections with the changes I made.

I ran some basic tests, everything seems to work fine when "upgrading" the login method e.g. from MD5, but additional testing is required to ensure everything works as expected in various conversion scenarios as well.

Is there a way to incorporate the hashing mechanism into the AuthPlugin? #1070

My comment about Auth Plugins was related to discussion for moving core auth mechanism (or new ones) to new plugin model. Though this can be de-coupled from this work. @cproensa was more excited about this than I am. My goal was to enable plugins to add custom plugins rather than move core ones to plugins.

Rename the $p_allow_protected to $p_hash_update, reflecting its original
meaning (introduced in b21c0a6) which
was to allow the automatic update of the password hash following a
change of login method.
The function was modified, so that when $p_hash_update = true, it does
not
- check for protected accounts,
- expire user sessions, or
- clear the account activation token.
This follows discussion in [1].
Fixes #22840
[1] #1048 (comment)

- Apply new constant names wherever the old ones were used in the code
- Keep the old constants in the obsolete section at the end of
constant_inc.php for backwards compatibility
- Update documentation (Admin guide + config_defaults_inc.php)

This comment has been minimized.

By todays standards a plain MD5 hashing would be considered to be a major vulnerability.
If a break-in happens, most passwords could be easily decrypted with rainbow tables.
When is this finally going to be fixed?

This comment has been minimized.

Just to let everyone know that this is still on my todo list; unfortunately real life got in the way, and I am having a hard time investing time on open-source development these days so I'm no able to commit on a date for fixing this issue.

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.