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

Use modern hash function when save user's password #1048

Open
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@fetus-hina
Copy link

commented Mar 10, 2017

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.

Thank you.

Bugtracker reference: #22839

@dregad

This comment has been minimized.

Copy link
Member

commented Mar 10, 2017

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.

See also related #50 (yes that's old)

Don't worry about your English, it is anyway much better than my Japanese ;)

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).

Thanks for pointing that out; I will fix it. (for the record, even though you won't have access to it #22511)

@dregad

This comment has been minimized.

Copy link
Member

commented Mar 10, 2017

I will fix it. (for the record, even though you won't have access to it #22511)

It's done.

@vboctor

This comment has been minimized.

Copy link
Member

commented Apr 1, 2017

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

fetus-hina and others added some commits Mar 10, 2017

Add SAFE_HASH as login method to keep password safe
Signed-off-by: AIZAWA Hina <hina@bouhime.com>
Switch default login method to SAFE_HASH
Signed-off-by: AIZAWA Hina <hina@bouhime.com>
Remove PASSWORD_MAX_SIZE_BCRYPT constant
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
Process SAFE_HASH separately from legacy methods
This allows usage of password_verify() function to protect against
timing attacks.
No need to use salt with password_hash()
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.

@dregad dregad force-pushed the fetus-hina:use-modern-password-func branch from f3b4203 to 94a0440 May 2, 2017

@dregad

This comment has been minimized.

Copy link
Member

commented May 2, 2017

@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

@vboctor, not sure what you mean by that, can you clarify ?

@vboctor
Copy link
Member

left a comment

I have provided some comments on the PR.

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.

Show resolved Hide resolved core/authentication_api.php Outdated
Show resolved Hide resolved core/constant_inc.php Outdated
Show resolved Hide resolved config_defaults_inc.php Outdated
Show resolved Hide resolved core/authentication_api.php Outdated
Show resolved Hide resolved core/authentication_api.php Outdated
Show resolved Hide resolved core/authentication_api.php Outdated
Show resolved Hide resolved core/authentication_api.php

@vboctor vboctor assigned vboctor and dregad and unassigned vboctor May 3, 2017

@fetus-hina

This comment has been minimized.

Copy link
Author

commented May 3, 2017

@dregad
I have no objections about your work. However, we seem to need discussion about length limitation.
Also I have no opinion about it, at this time.

@dregad

This comment has been minimized.

Copy link
Member

commented May 4, 2017

@vboctor do you have any further comments on my replies following your initial review ?

Also, it would be nice if you could provide feedback on 8e466bb#commitcomment-21980157

@vboctor

This comment has been minimized.

Copy link
Member

commented May 4, 2017

@dregad I responded to your replies. I think these should be addressed or get to an agreement on.

@dregad

This comment has been minimized.

Copy link
Member

commented May 5, 2017

@vboctor thanks. I'll make adjustments per my replies above, and let you know when it's ready for the next round of review.

Note that you have not responded to

Also, it would be nice if you could provide feedback on 8e466bb#commitcomment-21980157

@vboctor

This comment has been minimized.

Copy link
Member

commented May 6, 2017

@dregad I replied to your comment about LDAP check positioning.

dregad added some commits May 6, 2017

Use PASSWORD_BCRYPT instead of PASSWORD_DEFAULT
Following discussion in [1], we are setting the hashing algorithm so
we can keep control over which one is being used, and when to change it.

[1] #1048 (comment)
Throw error if plain password is too big
Previously, the password was silently truncated to fit the database
field.

This places the responsibility to ensure that the password is small
enough on the caller.

Fixes #22841
Do not expire sessions when updating password hash
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)
@dregad

This comment has been minimized.

Copy link
Member

commented May 6, 2017

@vboctor I just pushed updated code which should address your comments. I have not fully tested it yet.

@vboctor
Copy link
Member

left a comment

Looks good. minor comments + 1 bug found.

Show resolved Hide resolved admin/check/check_crypto_inc.php Outdated
Show resolved Hide resolved core/authentication_api.php Outdated
Show resolved Hide resolved core/user_api.php Outdated
PR revision fixes from @vboctor's review
- replace remaining occurences of SAFE_HASH (except in admin checks)
- add missing '!' operator in user_set_password()

dregad added some commits May 7, 2017

Move obsolete constants definitons to end of file
Having these at the end makes maintenance easier
Rename login method constants with prefix
- 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)
@vboctor

vboctor approved these changes May 7, 2017

@atrol

This comment has been minimized.

Copy link
Member

commented May 7, 2017

Maybe we should change the password in the following schema step to prevent any issue we might get when trying to update the hash on first login of a fresh install.

$g_upgrade[51] = array( 'InsertData', array( db_get_table( 'user' ), "(
		username, realname, email, password,
		date_created, last_visit, enabled, protected, access_level,
		login_count, lost_password_request_count, failed_login_count,
		cookie_string
	)
	VALUES (
		'administrator', '', 'root@localhost', '63a9f0ea7bb98050796b649e85481845',
		$t_timestamp, $t_timestamp, '1', '0', 90,
		3, 0, 0, '"
		. md5( mt_rand( 0, mt_getrandmax() ) + mt_rand( 0, mt_getrandmax() ) ) . md5( time() )
		. "'
	)" ) );
@dregad

This comment has been minimized.

Copy link
Member

commented May 7, 2017

prevent any issue we might get when trying to update the hash on first login of a fresh install

I'm not really sure what issues you are thinking about here, but in any case I think it should be safe to be consistent and update the default administrator password's hash from md5 to bcrypt.

@atrol

This comment has been minimized.

Copy link
Member

commented May 7, 2017

I'm not really sure what issues you are thinking about here

Nothing special, just the fact that code which is not executed can't cause a problem.

@dregad

This comment has been minimized.

Copy link
Member

commented Jun 8, 2017

For the record, I am still working on this, I am not satisfied with the current handling of the password upgrade mechanism and would like to implement automated testing too.

@Magissia

This comment has been minimized.

Copy link

commented Sep 25, 2017

Could we merge this as current hashing is clearly lacking security ?

@dregad

This comment has been minimized.

Copy link
Member

commented Sep 25, 2017

@Magissia it is not mergeable in its current form, and I have not gotten around to fixing the issues I have found in password upgrade logic. Too many things to do, and not enough time, unfortunately.

@limesurvey-translations

This comment has been minimized.

Copy link

commented Apr 25, 2018

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?

@dododedodonl

This comment has been minimized.

Copy link

commented May 29, 2018

Any updates regarding the acceptance of this (or a similair) PR?

@luciditee

This comment has been minimized.

Copy link

commented Oct 7, 2018

Is there an update to this PR? I ended up using Bugzilla for a deployment instead of Mantis because of the MD5 usage, and I much prefer the Mantis interface.

@dregad

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.