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
Open
Changes from 7 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -52,8 +52,8 @@
if( config_get_global( 'login_method' ) != LDAP ) {
check_print_test_warn_row(
'login_method is set to MD5',
config_get_global( 'login_method' ) == MD5,
'MD5 password encryption is currently the strongest password storage method supported by MantisBT.'
'login_method is set to SAFE_HASH',
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 6, 2017

Member

Should we replace all SAFE_HASH to HASH_BCRYPT? I suggest find across all files since there are other occurrences in other files.

This comment has been minimized.

Copy link
@dregad

dregad May 6, 2017

Member

Thanks for the heads up, I missed those as I only looked at the code and omitted the comments.

Note that I'm still working on updating the admin checks, that will be covered with an upcoming commit.

config_get_global( 'login_method' ) == SAFE_HASH,
'SAFE_HASH password encryption is currently the strongest password storage method supported by MantisBT.'
);
}
@@ -1837,14 +1837,25 @@
#############################################
/**
* Login authentication method. Must be one of
* MD5, LDAP, BASIC_AUTH or HTTP_AUTH.
* Login authentication method.
*
* The default SAFE_HASH method relies on PHP's password_hash() function, using
* the default algorithm (PASSWORD_DEFAULT, currently CRYPT_BLOWFISH).
* @see http://php.net/function.password-hash
*
* Note: you may not be able to easily switch encryption methods, so this
* should be carefully chosen at install time. However, MantisBT will attempt
* to "fall back" to older methods if possible.
* @global integer $g_login_method
*/
$g_login_method = MD5;
* should be carefully chosen at install time. However, if possible, MantisBT
* will attempt to "fall back" to older methods, and to convert the DB-stored
* hashes to the current login method after a successful login (e.g. with
* default settings, a MD5 hash will be upgraded to SAFE_HASH).
*
* @global integer $g_login_method One of: SAFE_HASH (default), LDAP, BASIC_AUTH
* or HTTP_AUTH. Other values (PLAIN, MD5, CRYPT
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 3, 2017

Member

Should we fail when extremely old algorithms are configured as current? e.g. PLAIN and CRYPT? We should continue supporting converting them to newer SAFE_HASH as users login.

This comment has been minimized.

Copy link
@dregad

dregad May 3, 2017

Member

The Admin Checks will issue a warning in this case (including for MD5 now), I think that's sufficient - let the administrator decide.

We could obsolete them completely if you want to but I think that's outside of this PR's scope.

This comment has been minimized.

Copy link
@vboctor

vboctor May 4, 2017

Member

We should also consider whether we want to call this SAFE_HASH vs. BCRYPT or some specific algorithm that we choose in the future. That builds on using a specific algorithm and being more explicit about it. But we may decide to do one without the other.

This comment has been minimized.

Copy link
@dregad

dregad May 5, 2017

Member

I am not overly keen on the SAFE_HASH name but couldn't think of a better name, so I'm fine with changing it.

Considering the switch from PASSWORD_DEFAULT to PASSWORD_BCRYPT (#1048 (comment)), it makes sense to use a more specific name.

This comment has been minimized.

Copy link
@vboctor

vboctor May 6, 2017

Member

Considering the switch from PASSWORD_DEFAULT to PASSWORD_BCRYPT (#1048 (comment)), it makes sense to use a more specific name.

I would use BCRYPT as the name and internally use PASSWORD_BCRYPT to PHP API. I don't not sure I'm for using PASSWORD_DEFAULT even in the future. I would rather if we manually upgrade to better algorithms with specific names as they become available.

* and CRYPT_FULL_SALT) are deprecated, and
* their use is strongly discouraged for
* security reasons.
*/
$g_login_method = SAFE_HASH;
/**
* Re-authentication required for admin areas
@@ -717,6 +717,20 @@ function auth_does_password_match( $p_user_id, $p_test_password ) {
}
$t_password = user_get_field( $p_user_id, 'password' );
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 3, 2017

Member

For readability, we should probably call rename $t_password to $t_password_hash.

This comment has been minimized.

Copy link
@dregad

dregad May 3, 2017

Member

I thought about it, but decided to leave it as-is because it's not necessarily a hash (think PLAIN or BASIC_AUTH login methods), and the underlying table column is called password

This comment has been minimized.

Copy link
@vboctor

vboctor May 4, 2017

Member

ok, not a big deal.

# Process SAFE_HASH separately from the legacy methods, to safeguard against
# timing attacks leveraging password_verify() function.
if( SAFE_HASH == $t_configured_login_method
&& password_verify( $p_test_password, $t_password )
) {
# Update the password hash if it does not match the current algorithm
if( password_needs_rehash( $t_password, PASSWORD_DEFAULT ) ) {
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 3, 2017

Member

Use PASSWORD_BCRYPT algorithm.

This comment has been minimized.

Copy link
@dregad

dregad May 3, 2017

Member

See #1048 (comment) below

user_set_password( $p_user_id, $p_test_password, true );
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 3, 2017

Member

This is not new. But the user_set_password assumes that it is being called by a user and hence it does change the cookie which is not necessary since we are just no hashing. So no reason to log user out of other browsers for example. Technically speaking, we don't need to worry about verification token, but that should have no side effects.

This comment has been minimized.

Copy link
@dregad

dregad May 3, 2017

Member

Changing a password should cause all sessions to be terminated, in general.

However, in this specific case I agree that this is really a "technical" change (i.e. we have a new hash, but the password itself remains the same) so there is no need to actually log the user out.

Are you suggesting to call user_set_field( $p_user_id, 'password', $p_test_password ) instead ?
Note that this would require special handling for protected accounts (since user_set_fields will trigger an error in this case). Alternatively, adding a new parameter to user_set_field, or defining a new API function that doesn't reset the cookie / token could work as well.

This comment has been minimized.

Copy link
@vboctor

vboctor May 4, 2017

Member

I'm not recommending a specific approach, you can make such decision. I just think we should skip the steps that don't apply when we are changing the hash but the password remains the same.

This comment has been minimized.

Copy link
@dregad

dregad May 5, 2017

Member

OK, I'll implement that.

This comment has been minimized.

Copy link
@dregad
}
return true;
}
# Try older login methods in sequence, and set the password hash
$t_login_methods = array(
MD5,
CRYPT,
@@ -737,7 +751,12 @@ function auth_does_password_match( $p_user_id, $p_test_password ) {
# Check for migration to another login method and test whether the password was encrypted
# with our previously insecure implementation of the CRYPT method
if( ( $t_login_method != $t_configured_login_method ) || (( CRYPT == $t_configured_login_method ) && utf8_substr( $t_password, 0, 2 ) == utf8_substr( $p_test_password, 0, 2 ) ) ) {
if( $t_login_method != $t_configured_login_method
|| (
CRYPT == $t_configured_login_method
&& utf8_substr( $t_password, 0, 2 ) == utf8_substr( $p_test_password, 0, 2 )
)
) {
user_set_password( $p_user_id, $p_test_password, true );
}
@@ -764,29 +783,29 @@ function auth_does_password_match( $p_user_id, $p_test_password ) {
* @access public
*/
function auth_process_plain_password( $p_password, $p_salt = null, $p_method = null ) {
$t_login_method = config_get( 'login_method' );
if( $p_method !== null ) {
$t_login_method = $p_method;
}
$t_login_method = $p_method !== null ? $p_method : config_get( 'login_method' );
switch( $t_login_method ) {
case CRYPT:
# a null salt is the same as no salt, which causes a salt to be generated
# otherwise, use the salt given
$t_processed_password = crypt( $p_password, $p_salt );
break;
case MD5:
$t_processed_password = md5( $p_password );
break;
case SAFE_HASH:
$t_processed_password = password_hash( $p_password, PASSWORD_DEFAULT );
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 3, 2017

Member

We should use PASSWORD_BCRYPT to avoid the option of the algorithm being upgraded with PHP resulting in truncation. We can aways manually update this in future releases as a new algorithm is released whose output can be stored into the available field size. The documentation recommends 255 and we are currently 64. Not sure if we can move to 191 vs. 255. The bcrypt requires 60 characters, so we are covered there.

This comment has been minimized.

Copy link
@dregad

dregad May 3, 2017

Member

Using the default allows the same code base to automatically use the most advanced available algorithm, which I think is nice.

I thought about the database field size given the recommendation in the PHP manual, but given how much advance notice we'd get before a change of default algo would be effective, I think it would be a better approach to make a schema change (can be done now or later) to increase the password field's size.

The documentation recommends 255 and we are currently 64. Not sure if we can move to 191 vs. 255. The bcrypt requires 60 characters, so we are covered there.

I know, I had the BLOWFISH hash length in mind when I increased the field size back in 2015.

The 191 chars limit only applies to MySQL when the column is indexed, which is not the case of the password field. So there should be no issue with using 255 here.

This comment has been minimized.

Copy link
@vboctor

vboctor May 4, 2017

Member

I'm not a fan of auto-rolling algorithm without a specific MantisBT upgrade. But I'm OK with doing it later once the field size is updated to 255. Even if we change the db schema in a new release, using PASSWORD_DEFAULT today would mean that an old MantisBT release deployed on a server with a new PHP version would break.

Ideally, I would rather if we can select a specific algorithm as they become available and as we know it is compatible with our schema.

This comment has been minimized.

Copy link
@dregad

dregad May 5, 2017

Member

using PASSWORD_DEFAULT today would mean that an old MantisBT release deployed on a server with a new PHP version would break.

Good point. I'll switch to PASSWORD_BCRYPT and add a TODO to mention that this should be changed once the the field size has been updated to 255.

This comment has been minimized.

Copy link
@vboctor

vboctor May 6, 2017

Member

I would rather have changes in hash algorithm be synchronized with new MantisBT releases rather than based on PHP version installed. Hence, I would rather never use PASSWORD_DEFAULT. See my comment below about naming of the hashing algorithm which is also related to this.

This comment has been minimized.

Copy link
@dregad

dregad May 6, 2017

Member

OK, that makes sense

break;
case BASIC_AUTH:
case PLAIN:
default:
$t_processed_password = $p_password;
break;
}
# cut this off to DB_FIELD_SIZE_PASSWORD characters which the largest possible string in the database
# cut this off to DB_FIELD_SIZE_PASSWORD characters which is the largest
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 3, 2017

Member

Though not new. This truncation behavior is not great since making the password field wider later will cause users to no longer be able to login. Users will have to reset password at this point.

This comment has been minimized.

Copy link
@dregad

dregad May 3, 2017

Member

That's true, but I can't think of any alternative option. Do you have any suggestions ?

This comment has been minimized.

Copy link
@vboctor

vboctor May 4, 2017

Member

I would error instead of truncate when setting a password that exceeds db schema size. For plain passwords we can enforce the appropriate limit when user types in the password.

This comment has been minimized.

Copy link
@dregad

This comment has been minimized.

Copy link
@dregad
# possible string that can be stored in the database
return utf8_substr( $t_processed_password, 0, DB_FIELD_SIZE_PASSWORD );
}
@@ -148,6 +148,7 @@
define( 'LDAP', 4 );
define( 'BASIC_AUTH', 5 );
define( 'HTTP_AUTH', 6 );
define( 'SAFE_HASH', 7 );
# file upload methods
define( 'DISK', 1 );
@@ -591,8 +592,11 @@
define( 'DB_FIELD_SIZE_PASSWORD', 64 );
define( 'DB_FIELD_SIZE_API_TOKEN_NAME', 128 );
# Maximum size for the user's password when storing it as a hash
define( 'PASSWORD_MAX_SIZE_BEFORE_HASH', 1024 );
# Maximum size for the user's password when storing it as a hash.
# The CRYPT_BLOWFISH algorithm used by password_hash() will silently truncate
# passwords to 72 characters, so there is no point in allowing longer passwords.
# @see http://php.net/function.crypt
define( 'PASSWORD_MAX_SIZE_BEFORE_HASH', 72 );
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 3, 2017

Member

I would rather keep the limit as before. For example, if an auth plugin that validates passwords supports a longer length (e.g. LDAP), then we shouldn't block it. Let's not make decisions based on one specific algorithm.

This comment has been minimized.

Copy link
@dregad

dregad May 3, 2017

Member

Is it worth adding complexity to the code to deal with a theoretical scenario that is unlikely to be an issue ?

I mean, a password longer than 72 chars, really ? This is a really long password that I don't want to type everyday is 66 chars...

An alternative could be to let BCRYPT silently truncate the password, the likelihood of a collision is quite low.

This comment has been minimized.

Copy link
@dregad

dregad May 4, 2017

Member

I'm having second thoughts about this. Maybe it does make sense to continue allowing long passwords prior to hashing, and only capping the length when using blowfish algo.

This comment has been minimized.

Copy link
@vboctor

vboctor May 4, 2017

Member

We really should only enforce limit on plain password if it doesn't fit into the db schema. We should also retire plain password to get out of this business and only offer rolling it over to a secure algorithm.

This comment has been minimized.

Copy link
@dregad

dregad May 5, 2017

Member

We really should only enforce limit on plain password if it doesn't fit into the db schema

IIRC we already do that on the front-end (via input field maxlength).

retire plain password

Per our release management guidelines, removing deprecated feature should only happen in Major versions.

We already have a warning in admin checks recommending to switch to SAFE_HASH when method != LDAP, but we can fine-tune that if you want to make it more explicit.

This comment has been minimized.

Copy link
@vboctor

vboctor May 6, 2017

Member

Per our release management guidelines, removing deprecated feature should only happen in Major versions.

I mean eventually we should retire plain password when it is the right time, till then a good admin check is a good option to use.

IIRC we already do that on the front-end (via input field maxlength).

I recall that we had a fixed max length. The max length in case of using a hash is not really useful. But if we are using plain passwords, it is must to apply a 64 limit. Hence, we should have an app for max length to enforce in the UI that should take into consideration the algorithm used.

define( 'SECONDS_PER_DAY', 86400 );
@@ -274,6 +274,7 @@ function custom_function_default_auth_can_change_password() {
CRYPT,
CRYPT_FULL_SALT,
MD5,
SAFE_HASH,
);
return in_array( config_get( 'login_method' ), $t_can_change );
@@ -15,10 +15,15 @@
<listitem>
<para>Specifies which method will be used to
authenticate. It should be one of the following values
(defaults to <emphasis>MD5</emphasis>):
(defaults to <emphasis>SAFE_HASH</emphasis>):
<itemizedlist>
<listitem>
<para>MD5 - user's password is stored as a hash in the database</para>
<para>SAFE_HASH - the user's password is stored
as a hash in the database, relying on PHP's
<ulink url="http://php.net/function.password-hash">password_hash()</ulink>
function and using the default algorithm
(PASSWORD_DEFAULT, currently CRYPT_BLOWFISH).
</para>
</listitem>
<listitem>
<para>LDAP - authenticates against an LDAP (or Active Directory) server</para>
@@ -30,8 +35,13 @@
<para>HTTP_AUTH</para>
</listitem>
</itemizedlist>
In addition, the following deprecated values are supported for backwards-compatibility, and should no longer be used:
In addition, the following deprecated values are supported
for backwards-compatibility, but their use is strongly
discouraged for security reasons:
<itemizedlist>
<listitem>
<para>MD5 - user's password is stored as a simple, unsalted hash in the database</para>
</listitem>
<listitem>
<para>PLAIN - password is stored in plain, unencrypted text in the database</para>
</listitem>
@@ -41,10 +51,23 @@
<listitem>
<para>CRYPT_FULL_SALT</para>
</listitem>
</itemizedlist></para>
</itemizedlist>
</para>

<para>Note: you may not be able to easily switch encryption methods, so this should be carefully chosen at install time.
However, MantisBT will attempt to &quot;fall back&quot; to older methods if possible.</para>
<warning>
<para>You may not be able to easily switch encryption
methods, so this should be carefully chosen at
install time.
</para>
</warning>
<note>
<para>MantisBT will attempt to &quot;fall back&quot; to older
methods if possible, and to convert the DB-stored
hashes to the current login method after a successful
login (e.g. with default settings, a MD5 hash will
be upgraded to SAFE_HASH).
</para>
</note>

</listitem>
</varlistentry>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.