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

Always

Just for now

@@ -95,7 +95,7 @@
extract( $t_row, EXTR_PREFIX_ALL, 'u' );
$t_ldap = ( LDAP == config_get( 'login_method' ) );
$t_ldap = ( LOGIN_METHOD_LDAP == config_get( 'login_method' ) );
# In case we're using LDAP to get the email address... this will pull out
# that version instead of the one in the DB
@@ -96,7 +96,7 @@
trigger_error( ERROR_EMPTY_FIELD, ERROR );
}
$t_ldap = ( LDAP == config_get( 'login_method' ) );
$t_ldap = ( LOGIN_METHOD_LDAP == config_get( 'login_method' ) );
# Update email (but only if LDAP isn't being used)
# Do not update email for a user verification
@@ -44,16 +44,48 @@
array( false => 'The crypto_master_salt option needs to be specified in config_inc.php with a minimum string length of 16 characters.' )
);
# Login method checks
$t_login_method = config_get_global( 'login_method' );
$t_switch_to_method = ' You should switch to '
. login_method_name( LOGIN_METHOD_HASH_BCRYPT )
. ', which is currently the strongest password storage method supported by MantisBT.';
$t_deprecated_login_methods = array( LOGIN_METHOD_HASH_MD5, LOGIN_METHOD_HASH_CRYPT, LOGIN_METHOD_HASH_CRYPT_FULL_SALT, LOGIN_METHOD_PLAIN );
check_print_test_row(
'login_method is not equal to CRYPT_FULL_SALT',
config_get_global( 'login_method' ) != CRYPT_FULL_SALT,
array( false => 'Login method CRYPT_FULL_SALT has been deprecated and should not be used.' )
'Do not use an outdated login method',
!in_array( $t_login_method, $t_deprecated_login_methods ),
array( false => login_method_name( $t_login_method )
. ' has been deprecated and should no longer be used for security reasons. '
. $t_switch_to_method
)
);
if( config_get_global( 'login_method' ) != LDAP ) {
if( $t_login_method != LOGIN_METHOD_LDAP ) {
$t_plain_text_login_methods = array( LOGIN_METHOD_PLAIN, LOGIN_METHOD_BASIC_AUTH, LOGIN_METHOD_HTTP_AUTH );
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.'
'Passwords should be stored encrypted in the database',
!in_array( $t_login_method, $t_plain_text_login_methods ),
login_method_name( $t_login_method )
. ' causes passwords to be stored in clear text. '
. $t_switch_to_method
);
}
/**
* Returns the login method name
* @param int $p_method One of the login methods constants
* @return string Login method name
*/
function login_method_name( $p_method ) {
switch( $p_method ) {
case LOGIN_METHOD_PLAIN: return 'LOGIN_METHOD_PLAIN';
case LOGIN_METHOD_BASIC_AUTH: return 'LOGIN_METHOD_BASIC_AUTH';
case LOGIN_METHOD_HTTP_AUTH: return 'LOGIN_METHOD_HTTP_AUTH';
case LOGIN_METHOD_HASH_CRYPT: return 'LOGIN_METHOD_HASH_CRYPT';
case LOGIN_METHOD_HASH_CRYPT_FULL_SALT: return 'LOGIN_METHOD_HASH_CRYPT_FULL_SALT';
case LOGIN_METHOD_HASH_MD5: return 'LOGIN_METHOD_HASH_MD5';
case LOGIN_METHOD_HASH_BCRYPT: return 'LOGIN_METHOD_HASH_BCRYPT';
case LOGIN_METHOD_LDAP: return 'LOGIN_METHOD_LDAP';
}
return 'UNKNOWN';
}
@@ -1837,14 +1837,36 @@
#############################################
/**
* Login authentication method. Must be one of
* MD5, LDAP, BASIC_AUTH or HTTP_AUTH.
* Login authentication method.
*
* Possibles values:
* - LOGIN_METHOD_HASH_BCRYPT (default) The password is hashed¹ with the
* CRYPT_BLOWFISH algorithm and stored in the database
* - LOGIN_METHOD_LDAP Authenticates against an LDAP (or Active Directory) server
* - LOGIN_METHOD_BASIC_AUTH
* - LOGIN_METHOD_HTTP_AUTH
*
* The following values are deprecated, and kept for backwards compatibility only:
* - LOGIN_METHOD_HASH_MD5
* - LOGIN_METHOD_HASH_CRYPT
* - LOGIN_METHOD_HASH_CRYPT_FULL_SALT
* - LOGIN_METHOD_PLAIN
* Their use is strongly discouraged for security reasons; LOGIN_METHOD_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, an MD5 hash (LOGIN_METHOD_HASH_MD5) will be upgraded to
* LOGIN_METHOD_HASH_BCRYPT.
*
* @global integer $g_login_method One of the values listed above.
*/
$g_login_method = MD5;
$g_login_method = LOGIN_METHOD_HASH_BCRYPT;
/**
* Re-authentication required for admin areas
@@ -333,10 +333,10 @@ function auth_prepare_username( $p_username ) {
$t_username = null;
switch( config_get( 'login_method' ) ) {
case BASIC_AUTH:
case LOGIN_METHOD_BASIC_AUTH:
$t_username = $_SERVER['REMOTE_USER'];
break;
case HTTP_AUTH:
case LOGIN_METHOD_HTTP_AUTH:
if( !auth_http_is_logout_pending() ) {
if( isset( $_SERVER['PHP_AUTH_USER'] ) ) {
$t_username = $_SERVER['PHP_AUTH_USER'];
@@ -367,10 +367,10 @@ function auth_prepare_username( $p_username ) {
*/
function auth_prepare_password( $p_password ) {
switch( config_get( 'login_method' ) ) {
case BASIC_AUTH:
case LOGIN_METHOD_BASIC_AUTH:
$f_password = $_SERVER['PHP_AUTH_PW'];
break;
case HTTP_AUTH:
case LOGIN_METHOD_HTTP_AUTH:
if( !auth_http_is_logout_pending() ) {
# this will never get hit - see auth_prepare_username
@@ -405,9 +405,9 @@ function auth_prepare_password( $p_password ) {
function auth_auto_create_user( $p_username, $p_password ) {
$t_login_method = config_get( 'login_method' );
if( $t_login_method == BASIC_AUTH ) {
if( $t_login_method == LOGIN_METHOD_BASIC_AUTH ) {
$t_auto_create = true;
} else if( $t_login_method == LDAP && ldap_authenticate_by_username( $p_username, $p_password ) ) {
} else if( $t_login_method == LOGIN_METHOD_LDAP && ldap_authenticate_by_username( $p_username, $p_password ) ) {
$t_auto_create = true;
} else {
$t_auto_create = false;
@@ -661,7 +661,7 @@ function auth_logout() {
helper_clear_pref_cookies();
}
if( HTTP_AUTH == config_get( 'login_method' ) ) {
if( LOGIN_METHOD_HTTP_AUTH == config_get( 'login_method' ) ) {
auth_http_set_logout_pending( true );
}
@@ -674,7 +674,7 @@ function auth_logout() {
* @access public
*/
function auth_automatic_logon_bypass_form() {
return config_get( 'login_method' ) == HTTP_AUTH;
return config_get( 'login_method' ) == LOGIN_METHOD_HTTP_AUTH;
}
/**
@@ -686,11 +686,14 @@ function auth_automatic_logon_bypass_form() {
function auth_get_password_max_size() {
switch( config_get( 'login_method' ) ) {
# Max password size cannot be bigger than the database field
case PLAIN:
case BASIC_AUTH:
case HTTP_AUTH:
case LOGIN_METHOD_PLAIN:
case LOGIN_METHOD_BASIC_AUTH:
case LOGIN_METHOD_HTTP_AUTH:
return DB_FIELD_SIZE_PASSWORD;
case LOGIN_METHOD_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;
@@ -708,7 +711,7 @@ function auth_get_password_max_size() {
function auth_does_password_match( $p_user_id, $p_test_password ) {
$t_configured_login_method = config_get( 'login_method' );
if( LDAP == $t_configured_login_method ) {
if( LOGIN_METHOD_LDAP == $t_configured_login_method ) {
return ldap_authenticate( $p_user_id, $p_test_password );
}
@@ -717,11 +720,25 @@ 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 modern hash methods separately from the legacy ones, to safeguard
# against timing attacks by leveraging password_verify() function.
if( LOGIN_METHOD_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 );
}
return true;
}
# Try older login methods in sequence, and set the password hash
$t_login_methods = array(
MD5,
CRYPT,
PLAIN,
BASIC_AUTH,
LOGIN_METHOD_HASH_MD5,
LOGIN_METHOD_HASH_CRYPT,
LOGIN_METHOD_PLAIN,
LOGIN_METHOD_BASIC_AUTH,
);
foreach( $t_login_methods as $t_login_method ) {
@@ -730,14 +747,19 @@ function auth_does_password_match( $p_user_id, $p_test_password ) {
# Do not support migration to PLAIN, since this would be a crazy thing to do.
# Also if we do, then a user will be able to login by providing the MD5 value
# that is copied from the database. See #8467 for more details.
if( ( $t_configured_login_method != PLAIN && $t_login_method == PLAIN ) ||
( $t_configured_login_method != BASIC_AUTH && $t_login_method == BASIC_AUTH ) ) {
if( ( $t_configured_login_method != LOGIN_METHOD_PLAIN && $t_login_method == LOGIN_METHOD_PLAIN ) ||
( $t_configured_login_method != LOGIN_METHOD_BASIC_AUTH && $t_login_method == LOGIN_METHOD_BASIC_AUTH ) ) {
continue;
}
# 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
|| (
LOGIN_METHOD_HASH_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:
case LOGIN_METHOD_HASH_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:
case LOGIN_METHOD_HASH_MD5:
$t_processed_password = md5( $p_password );
break;
case BASIC_AUTH:
case PLAIN:
case LOGIN_METHOD_HASH_BCRYPT:
# 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 LOGIN_METHOD_BASIC_AUTH:
case LOGIN_METHOD_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;
}
/**
@@ -985,7 +1016,7 @@ function auth_reauthentication_expiry() {
* @access public
*/
function auth_reauthenticate() {
if( !auth_reauthentication_enabled() || BASIC_AUTH == config_get( 'login_method' ) || HTTP_AUTH == config_get( 'login_method' ) ) {
if( !auth_reauthentication_enabled() || LOGIN_METHOD_BASIC_AUTH == config_get( 'login_method' ) || LOGIN_METHOD_HTTP_AUTH == config_get( 'login_method' ) ) {
return true;
}
@@ -141,13 +141,14 @@
define( 'UNREAD', 202 );
# login methods
define( 'PLAIN', 0 );
define( 'CRYPT', 1 );
define( 'CRYPT_FULL_SALT', 2 );
define( 'MD5', 3 );
define( 'LDAP', 4 );
define( 'BASIC_AUTH', 5 );
define( 'HTTP_AUTH', 6 );
define( 'LOGIN_METHOD_PLAIN', 0 );
define( 'LOGIN_METHOD_BASIC_AUTH', 5 );
define( 'LOGIN_METHOD_HTTP_AUTH', 6 );
define( 'LOGIN_METHOD_LDAP', 4 );
define( 'LOGIN_METHOD_HASH_CRYPT', 1 );
define( 'LOGIN_METHOD_HASH_CRYPT_FULL_SALT', 2 );
define( 'LOGIN_METHOD_HASH_MD5', 3 );
define( 'LOGIN_METHOD_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
@@ -604,15 +610,6 @@
define( 'AUTH_COOKIE_LENGTH', 64 );
define( 'API_TOKEN_LENGTH', 32 );
# Obsolete / deprecated constants
# Defined below for backwards-compatibility purposes -- Do not use them
# Constant # Replaced by
define( 'UNABLE_TO_DUPLICATE', 40 ); # UNABLE_TO_REPRODUCE
define( 'ERROR_BUG_RESOLVED_ACTION_DENIED', 1102 ); # N/A
define( 'LOG_SOAP', 64 ); # LOG_WEBSERVICE
define( 'FTP', 1 ); # DISK
define( 'ERROR_FTP_CONNECT_ERROR', 16 ); # N/A
# JQuery
# hashes acquired with command 'cat file.js | openssl dgst -sha256 -binary | openssl enc -base64 -A'
define( 'JQUERY_VERSION', '2.2.4' );
@@ -693,3 +690,21 @@
define( 'MANAGE_CONFIG_ACTION_CREATE', 'create' );
define( 'MANAGE_CONFIG_ACTION_CLONE', 'clone' );
define( 'MANAGE_CONFIG_ACTION_EDIT', 'edit' );
# KEEP THIS SECTION AT THE END OF THE FILE
# Obsolete / deprecated constants
# Defined below for backwards-compatibility purposes -- Do not use them
# Constant # Replaced by
define( 'UNABLE_TO_DUPLICATE', 40 ); # UNABLE_TO_REPRODUCE
define( 'ERROR_BUG_RESOLVED_ACTION_DENIED', 1102 ); # N/A
define( 'LOG_SOAP', 64 ); # LOG_WEBSERVICE
define( 'FTP', 1 ); # DISK
define( 'ERROR_FTP_CONNECT_ERROR', 16 ); # N/A
define( 'PLAIN', LOGIN_METHOD_PLAIN );
define( 'CRYPT', LOGIN_METHOD_HASH_CRYPT );
define( 'CRYPT_FULL_SALT', LOGIN_METHOD_HASH_CRYPT_FULL_SALT );
define( 'MD5', LOGIN_METHOD_HASH_MD5 );
define( 'LDAP', LOGIN_METHOD_LDAP );
define( 'BASIC_AUTH', LOGIN_METHOD_BASIC_AUTH );
define( 'HTTP_AUTH', LOGIN_METHOD_HTTP_AUTH );
define( 'HASH_BCRYPT', LOGIN_METHOD_HASH_BCRYPT );
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.