Skip to content
Permalink
Browse files Browse the repository at this point in the history
Security improvements part 1. See Issue #565
* No more emailing plaintext passwords out: password changes and reset requests are dealt with immediately on the login/admin panels.
* Stronger cryptographic hashes for confirmation links that don't leak user ID information.
* Hashes are securely tethered to the user account and auto-expire when logging in or generating a new request. Timeout window of 20 minutes by default for answering reset requests.
* Password input boxes are unmaskable. Saves having a separate confirmation box, simplifies code logic and is better UX.
* Dedicated txp_token table introduced for handling token-based storage and retrieval. Better for plugin authors than abusing txp_discuss_nonce.

More on the way including strength meter and improvements to setup passwords...
  • Loading branch information
Bloke committed Nov 5, 2015
1 parent ce2378f commit 1c09094
Show file tree
Hide file tree
Showing 6 changed files with 313 additions and 83 deletions.
37 changes: 9 additions & 28 deletions textpattern/include/txp_admin.php
Expand Up @@ -132,7 +132,7 @@ function change_pass()
{
global $txp_user;

extract(psa(array('new_pass', 'mail_password')));
extract(psa(array('new_pass')));

if (empty($new_pass)) {
author_list(array(gTxt('password_required'), E_ERROR));
Expand All @@ -143,18 +143,8 @@ function change_pass()
$rs = change_user_password($txp_user, $new_pass);

if ($rs) {
$message = gTxt('password_changed');

if ($mail_password) {
$email = fetch('email', 'txp_users', 'name', $txp_user);

send_new_password($new_pass, $email, $txp_user);

$message .= sp.gTxt('and_mailed_to').sp.$email;
}

$message .= '.';

// Todo: Move full stop to Textpack.
$message = gTxt('password_changed') . '.';
author_list($message);
}
}
Expand Down Expand Up @@ -241,12 +231,12 @@ function new_pass_form()
hed(gTxt('change_password'), 2).
inputLabel(
'new_pass',
fInput('password', 'new_pass', '', '', '', '', INPUT_REGULAR, '', 'new_pass'),
fInput('password', 'new_pass', '', 'txp-maskable', '', '', INPUT_REGULAR, '', 'new_pass'),
'new_password', '', array('class' => 'txp-form-field edit-admin-new-password')
).
graf(
checkbox('mail_password', '1', true, '', 'mail_password').
n.tag(gTxt('mail_it'), 'label', array('for' => 'mail_password')), array('class' => 'edit-admin-mail-password')).
checkbox('unmask', 1, false, 0, 'show_password').
n.tag(gTxt('show_password'), 'label', array('for' => 'show_password')), array('class' => 'edit-admin-show-password')).
graf(fInput('submit', 'change_pass', gTxt('submit'), 'publish')).
eInput('admin').
sInput('change_pass'),
Expand Down Expand Up @@ -714,20 +704,11 @@ function admin_multi_edit()
case 'resetpassword':

foreach ($names as $name) {
$passwd = generate_password(PASSWORD_LENGTH);

if (change_user_password($name, $passwd)) {
$email = safe_field("email", 'txp_users', "name = '".doSlash($name)."'");

if (send_new_password($passwd, $email, $name)) {
$changed[] = $name;
$msg = 'author_updated';
} else {
return author_list(array(gTxt('could_not_mail').' '.txpspecialchars($name), E_ERROR));
}
}
send_reset_confirmation_request($name);
$changed[] = $name;
}

$msg = 'password_reset_confirmation_request_sent';
break;
}

Expand Down
112 changes: 91 additions & 21 deletions textpattern/include/txp_auth.php
Expand Up @@ -71,23 +71,26 @@ function doLoginForm($message)
global $textarray_script, $event, $step;

include txpath.'/lib/txplib_head.php';

$event = 'login';

$stay = (cs('txp_login') && !gps('logout') ? 1 : 0);
$reset = gps('reset');
$confirm = gps('confirm');

if (gps('logout')) {
$step = 'logout';
} elseif (gps('reset')) {
} elseif ($reset) {
$step = 'reset';
} elseif ($confirm) {
$step = 'confirm';
}

pagetop(gTxt('login'), $message);

$stay = (cs('txp_login') and !gps('logout') ? 1 : 0);
$reset = gps('reset');

$name = join(',', array_slice(explode(',', cs('txp_login')), 0, -1));
$out = array();

if ($reset) {
$pageTitle = gTxt('password_reset');
$out[] = hed(gTxt('password_reset'), 1, array('id' => 'txp-login-heading')).
n.tag(
n.tag(gTxt('name'), 'label', array('for' => 'login_name')).
Expand All @@ -98,7 +101,29 @@ function doLoginForm($message)
graf(
href(gTxt('back_to_login'), 'index.php'), array('class' => 'login-return')).
hInput('p_reset', 1);
} elseif ($confirm) {
$pageTitle = gTxt('change_password');
$out[] = hed(gTxt('change_password'), 1, array('id' => 'txp-change-password-heading')).
n.tag(
n.tag(gTxt('new_password'), 'label', array(
'class' => 'txp-form-field-label',
'for' => 'change_password',
)).
fInput('password', 'p_password', '', 'txp-form-field-input txp-maskable', '', '', INPUT_REGULAR, '', 'change_password', false, true),
'div', array('class' => 'txp-form-field change-password')).
graf(
checkbox('unmask', 1, false, 0, 'show_password').n.
tag(gTxt('show_password'), 'label', array('for' => 'show_password'))
, array('class' => 'show-password')).
graf(
fInput('submit', '', gTxt('password_confirm_button'), 'publish').n
).
graf(
href(gTxt('back_to_login'), 'index.php'), array('class' => 'login-return'));
$out[] = hInput('hash', gps('confirm'));
$out[] = hInput('p_alter', 1);
} else {
$pageTitle = gTxt('login');
$out[] = hed(gTxt('login_to_textpattern'), 1, array('id' => 'txp-login-heading')).
n.tag(
n.tag(gTxt('name'), 'label', array('for' => 'login_name')).
Expand All @@ -124,6 +149,8 @@ function doLoginForm($message)
}
}

pagetop($pageTitle, $message);

echo form(
join('', $out), '', '', 'post', 'txp-login', '', 'login_form').

Expand All @@ -136,22 +163,35 @@ function doLoginForm($message)
/**
* Validates the sent login form and creates a session.
*
* During the reset request procedure, it is conceivable to verify the
* token as soon as it is presented in the URL, but that would require:
* a) very similar code in both p_confirm and p_alter branches (unless refactored)
* b) some way (other than via the message) to signal back to doLoginForm() that
* the token is bogus so the 'change your password' form is not displayed.
* Perhaps raise an exception?
*
* @todo Investigate validating confirm token as soon as it's presented in URL (better UX).
* @todo Could this be done via a Validator()?
*
* @return string A localised feedback message
* @see doLoginForm()
*/

function doTxpValidate()
{
global $logout, $txp_user;

$p_userid = ps('p_userid');
$p_password = ps('p_password');
$p_reset = ps('p_reset');
$p_alter = ps('p_alter');
$stay = ps('stay');
$p_confirm = gps('confirm');
$logout = gps('logout');
$message = '';
$pub_path = preg_replace('|//$|', '/', rhu.'/');

if (cs('txp_login') and strpos(cs('txp_login'), ',')) {
if (cs('txp_login') && strpos(cs('txp_login'), ',')) {
$txp_login = explode(',', cs('txp_login'));
$c_hash = end($txp_login);
$c_userid = join(',', array_slice($txp_login, 0, -1));
Expand All @@ -165,7 +205,11 @@ function doTxpValidate()
setcookie('txp_login_public', '', time() - 3600, $pub_path);
}

if ($c_userid and strlen($c_hash) == 32) { // Cookie exists.
if ($c_userid && strlen($c_hash) === 32) {
// Cookie exists.
// @todo Improve security by using a better nonce/salt mechanism. md5 and uniqid are bad.
// @todo Flag cookie-based logins and force confirmation of old password when
// changing it from Admin->Users panel.
$r = safe_row(
"name, nonce",
'txp_users',
Expand Down Expand Up @@ -193,7 +237,8 @@ function doTxpValidate()
setcookie('txp_login_public', '', time() - 3600, $pub_path);
$message = array(gTxt('bad_cookie'), E_ERROR);
}
} elseif ($p_userid and $p_password) { // Incoming login vars.
} elseif ($p_userid && $p_password) {
// Incoming login vars.
$name = txp_validate($p_userid, $p_password);

if ($name !== false) {
Expand All @@ -218,7 +263,7 @@ function doTxpValidate()

setcookie(
'txp_login_public',
substr(md5($nonce), - 10).$name,
substr(md5($nonce), -10).$name,
($stay ? time() + 3600 * 24 * 30 : 0),
$pub_path
);
Expand All @@ -232,25 +277,50 @@ function doTxpValidate()
txp_status_header('401 Could not log in with that username/password');
$message = array(gTxt('could_not_log_in'), E_ERROR);
}
} elseif ($p_reset) { // Reset request.
} elseif ($p_reset) {
// Reset request.
sleep(3);

include_once txpath.'/lib/txplib_admin.php';

$message = ($p_userid) ? send_reset_confirmation_request($p_userid) : '';
} elseif (gps('reset')) {
$message = '';
} elseif (gps('confirm')) {
} elseif ($p_alter) {
// Password change confirmation.
sleep(3);
global $sitename;

$confirm = pack('H*', gps('confirm'));
$name = substr($confirm, 5);
$nonce = safe_field("nonce", 'txp_users', "name = '".doSlash($name)."'");
$pass = ps('p_password');

if ($nonce and $confirm === pack('H*', substr(md5($nonce), 0, 10)).$name) {
include_once txpath.'/lib/txplib_admin.php';

$message = reset_author_pass($name);
if (trim($pass) === '') {
$message = array(gTxt('password_required'), E_ERROR);
} else {
$hash = gps('hash');
$selector = substr($hash, SALT_LENGTH);
$tokenInfo = safe_row("reference_id, token, expires", 'txp_token', "selector = '".doSlash($selector)."' AND type='password_reset'");

if ($tokenInfo) {
if (strtotime($tokenInfo['expires']) <= time()) {
$message = array(gTxt('token_expired'), E_ERROR);
} else {
$uid = assert_int($tokenInfo['reference_id']);
$row = safe_row("name, email, nonce, pass AS old_pass", 'txp_users', "user_id = $uid");

if ($row['nonce'] && ($hash === bin2hex(pack('H*', substr(hash(HASHING_ALGORITHM, $row['nonce'].$selector.$row['old_pass']), 0, SALT_LENGTH))).$selector)) {
if (change_user_password($row['name'], $pass)) {
$body = gTxt('greeting').' '.$row['name'].','.n.n.gTxt('password_change_confirmation');
txpMail($row['email'], "[$sitename] ".gTxt('password_changed'), $body);
$message = gTxt('password_changed');

// Invalidate all reset requests in the wild for this user.
safe_delete("txp_token", "reference_id = $uid AND type = 'password_reset'");
}
} else {
$message = array(gTxt('invalid_token'), E_ERROR);
}
}
} else {
$message = array(gTxt('invalid_token'), E_ERROR);
}
}
}

Expand Down
47 changes: 47 additions & 0 deletions textpattern/lib/constants.php
Expand Up @@ -402,6 +402,53 @@
define('PASSWORD_SYMBOLS', '23456789abcdefghijkmnopqrstuvwxyz');
}

if (!defined('HASHING_ALGORITHM')) {
/**
* Algorithm to use for hashing passwords/reset requests.
*
* This constant can be overridden from the config.php.
*
* @package User
* @since 4.6.0
* @see PHP's hash_algos() function
* @example
* define('HASHING_ALGORITHM', 'whirlpool');
*/

define('HASHING_ALGORITHM', 'ripemd256');
}

if (!defined('SALT_LENGTH')) {
/**
* Length of salt/selector hashes.
*
* This constant can be overridden from the config.php.
*
* @package User
* @since 4.6.0
* @example
* define('SALT_LENGTH', '80');
*/

define('SALT_LENGTH', '64');
}

if (!defined('RESET_EXPIRY_MINUTES')) {
/**
* Length of time (in minutes) that a password reset request remains valid.
*
* This constant can be overridden from the config.php.
* Values under 60 may fall foul of DST changeover times, but meh.
*
* @package User
* @since 4.6.0
* @example
* define('RESET_EXPIRY_MINUTES', '90');
*/

define('RESET_EXPIRY_MINUTES', '20');
}

if (!defined('LOGIN_COOKIE_HTTP_ONLY')) {
/**
* If TRUE, login cookie is set just for HTTP.
Expand Down

0 comments on commit 1c09094

Please sign in to comment.