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 13 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' ) == HASH_BCRYPT,
'SAFE_HASH password encryption is currently the strongest password storage method supported by MantisBT.'
);
}
@@ -1837,14 +1837,31 @@
#############################################
/**
* Login authentication method. Must be one of
* MD5, LDAP, BASIC_AUTH or HTTP_AUTH.
* Login authentication method.
*
* Possibles values:
* - HASH_BCRYPT (default) The password is hashed¹ with the CRYPT_BLOWFISH
* algorithm and stored in the database
* - LDAP Authenticates against an LDAP (or Active Directory) server
* - BASIC_AUTH
* - HTTP_AUTH
*
* The following values are deprecated, and kept for backwards compatibility
* only: MD5, CRYPT, CRYPT_FULL_SALT and PLAIN. Their use is strongly
* discouraged for security reasons; HASH_BCRYPT should be used instead.
*
* ¹ hashing is performed using PHP's password_hash() function
* @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
* 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 the values listed above.
*/
$g_login_method = MD5;
$g_login_method = HASH_BCRYPT;
/**
* Re-authentication required for admin areas
@@ -691,6 +691,9 @@ function auth_get_password_max_size() {
case HTTP_AUTH:
return DB_FIELD_SIZE_PASSWORD;
case HASH_BCRYPT:
return PASSWORD_MAX_SIZE_BCRYPT;
# All other cases, i.e. password is stored as a hash
default:
return PASSWORD_MAX_SIZE_BEFORE_HASH;
@@ -717,6 +720,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( HASH_BCRYPT == $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_BCRYPT ) ) {
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 +754,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 );
}
@@ -752,6 +774,10 @@ function auth_does_password_match( $p_user_id, $p_test_password ) {
* Encrypt and return the plain password given, as appropriate for the current
* global login method.
*
* It is the caller's responsibility to ensure that a plain-text password fits
* the database field (or the hash algorithm's limitations). The function will
* throw an error if it is bigger to avoid silent truncation.
*
* When generating a new password, no salt should be passed in.
* When encrypting a password to compare to a stored password, the stored
* password should be passed in as salt. If the authentication method is CRYPT then
@@ -764,30 +790,35 @@ 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 HASH_BCRYPT:
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 6, 2017

Member

How about either also adding LOGIN_METHOD_HASH_MD5, LOGIN_METHOD_HASH_CRYPT, LOGIN_METHOD_HASH_CRYPT_FULL_SALT and using them in the code, though keeping MD5 and CRYPT and CRYPT_FULL_SALT for backward compatibility? Or we can just rename this constant to BCRYPT.

This comment has been minimized.

Copy link
@dregad

dregad May 6, 2017

Member

My initial idea was to distinguish "new" hashing methods vs legacy ones. Adding a prefix to all the constants makes sense though.

This comment has been minimized.

Copy link
@dregad

dregad May 7, 2017

Member

Fixed

# Note that the CRYPT_BLOWFISH algorithm used by password_hash()
# will silently truncate the password to 72 characters
$t_processed_password = password_hash( $p_password, PASSWORD_BCRYPT );
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
return utf8_substr( $t_processed_password, 0, DB_FIELD_SIZE_PASSWORD );
if( utf8_strlen( $t_processed_password) > DB_FIELD_SIZE_PASSWORD ) {
error_parameters( 'password', DB_FIELD_SIZE_PASSWORD );
trigger_error( ERROR_FIELD_TOO_LONG, ERROR );
}
return $t_processed_password;
}
/**
@@ -148,6 +148,7 @@
define( 'LDAP', 4 );
define( 'BASIC_AUTH', 5 );
define( 'HTTP_AUTH', 6 );
define( 'HASH_BCRYPT', 7 );
# file upload methods
define( 'DISK', 1 );
@@ -591,9 +592,14 @@
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
# Arbitrary maximum size for the user's password when storing it as a hash.
define( 'PASSWORD_MAX_SIZE_BEFORE_HASH', 1024 );
# The CRYPT_BLOWFISH algorithm used by password_hash() will silently truncate
# passwords to 72 characters, so we limit length when using that algorithm.
# @see http://php.net/function.crypt
define( 'PASSWORD_MAX_SIZE_BCRYPT', 72 );
define( 'SECONDS_PER_DAY', 86400 );
# Auto-generated link targets
@@ -274,6 +274,7 @@ function custom_function_default_auth_can_change_password() {
CRYPT,
CRYPT_FULL_SALT,
MD5,
HASH_BCRYPT,
);
return in_array( config_get( 'login_method' ), $t_can_change );
@@ -1588,31 +1588,37 @@ function user_set_default_project( $p_user_id, $p_project_id ) {
}
/**
* Set the user's password to the given string, encoded as appropriate
* Set the user's password to the given string, encoded as appropriate.
*
* @param integer $p_user_id A valid user identifier.
* @param string $p_password A password to set.
* @param boolean $p_allow_protected Whether Allow password change to a protected account. This defaults to false.
* @param boolean $p_hash_update If True then protected accounts will be
* processed, and the user's sessions will not
* be expired (defaults to False).
* @return boolean always true
*/
function user_set_password( $p_user_id, $p_password, $p_allow_protected = false ) {
if( !$p_allow_protected ) {
function user_set_password( $p_user_id, $p_password, $p_hash_update = false ) {
if( !$p_hash_update ) {
user_ensure_unprotected( $p_user_id );
}
# When the password is changed, invalidate the cookie to expire sessions that
# may be active on all browsers.
$c_cookie_string = auth_generate_unique_cookie_string();
# Delete token for password activation if there is any
token_delete( TOKEN_ACCOUNT_ACTIVATION, $p_user_id );
$t_query = 'UPDATE {user} SET password=' . db_param();
$t_param = array( auth_process_plain_password( $p_password ) );
$c_password = auth_process_plain_password( $p_password );
if( $p_hash_update ) {
This conversation was marked as resolved by dregad

This comment has been minimized.

Copy link
@vboctor

vboctor May 6, 2017

Member

!$p_hash_update

This comment has been minimized.

Copy link
@dregad

dregad May 6, 2017

Member

Fixed, thanks

# When the password is changed, invalidate the cookie to expire all
# active sessions, and delete password activation token if there is any.
token_delete( TOKEN_ACCOUNT_ACTIVATION, $p_user_id );
$t_query .= ', cookie_string=' . db_param();
$t_param[] = auth_generate_unique_cookie_string();
}
$t_query .= ' WHERE id=' . db_param();
$t_param[] = (int)$p_user_id;
db_param_push();
$t_query = 'UPDATE {user}
SET password=' . db_param() . ', cookie_string=' . db_param() . '
WHERE id=' . db_param();
db_query( $t_query, array( $c_password, $c_cookie_string, (int)$p_user_id ) );
db_query( $t_query, $t_param );
return true;
}
@@ -14,37 +14,71 @@

<listitem>
<para>Specifies which method will be used to
authenticate. It should be one of the following values
(defaults to <emphasis>MD5</emphasis>):
authenticate. It must be one of the following values
(defaults to <emphasis>HASH_BCRYPT</emphasis>):
<itemizedlist>
<listitem>
<para>MD5 - user's password is stored as a hash in the database</para>
<para><emphasis>HASH_BCRYPT</emphasis> -
The password is hashed <footnote id="hash">
<para>
Hashing is performed using PHP's
<ulink url="http://php.net/function.password-hash">password_hash()</ulink>
function.
</para>
</footnote>
using the CRYPT_BLOWFISH algorithm, and stored in the database.
</para>
</listitem>
<listitem>
<para>LDAP - authenticates against an LDAP (or Active Directory) server</para>
<para><emphasis>LDAP</emphasis> -
authenticates against an LDAP (or Active Directory) server
</para>
</listitem>
<listitem>
<para>BASIC_AUTH</para>
<para><emphasis>BASIC_AUTH</emphasis></para>
</listitem>
<listitem>
<para>HTTP_AUTH</para>
<para><emphasis>HTTP_AUTH</emphasis></para>
</listitem>
</itemizedlist>
In addition, the following deprecated values are supported for backwards-compatibility, and should no longer be used:
In addition, the following values are deprecated, and kept
for backwards compatibility reasons only. Their use is strongly
discouraged for security reasons; <emphasis>HASH_BCRYPT</emphasis>
should be used instead.
<itemizedlist>
<listitem>
<para>PLAIN - password is stored in plain, unencrypted text in the database</para>
<para><emphasis>MD5</emphasis> -
user's password is stored as a simple, unsalted hash in the database
</para>
</listitem>
<listitem>
<para>CRYPT</para>
<para><emphasis>PLAIN</emphasis> -
password is stored in plain, unencrypted text in the database
</para>
</listitem>
<listitem>
<para>CRYPT_FULL_SALT</para>
<para><emphasis>CRYPT</emphasis></para>
</listitem>
</itemizedlist></para>
<listitem>
<para><emphasis>CRYPT_FULL_SALT</emphasis></para>
</listitem>
</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.