I ran into a situation where the "Force password change on next login" option would not work. The "force_change" record for the user would be updated to 0 when the user logged into my system, but the user wouldn't have to change their password.

The circumstance (although not required to understand this bug report) was as such: A custom module was in use that would, if the conditions where correct, call user_save($account, array('custom_property' => NULL)) during a hook_user(op=login);. Seems reasonable, but exposed a bug in Password Policy.

The bug is in password_policy.module's hook_user(op=update):

if ($account->force_password_change && isset($account->pass) && $user->uid == $account->uid) {
  db_query('UPDATE {password_policy_force_change} SET force_change = 0 WHERE uid = %d', $account->uid);
}

The problem being ".. && isset($account->pass) && ..". The 'pass' property of $account would always be set, so it is possible that we can set force_change=0 while saving a user, even if a new password hasn't been supplied.

The fix would be to check to see if $edit['pass'] isn't empty, rather than check to see if $account->pass is set.

CommentFileSizeAuthor
#2 force_change_bug-2096349-2.patch675 bytesaohrvetpv
#1 force_change_bug-2096349-1.patch660 bytesryan_courtnage

Comments

ryan_courtnage’s picture

ryan_courtnage commented
Status: Active » Needs review
StatusFileSize
new force_change_bug-2096349-1.patch660 bytes

6.x.1.x patch attached.

aohrvetpv’s picture

aohrvetpv commented
Issue summary: View changes
StatusFileSize
new force_change_bug-2096349-2.patch675 bytes

This problem had already been fixed in 7.x-1.x-dev, intentionally or unintentionally, but the fix was apparently not backported.

The problem can be reproduced synthetically as follows:
1. Log in as admin.
2. Check "Force password change at next login" on user/1/edit and save.
3. drush php-eval '$account = user_load(1); user_save($account); $account = user_load(1); print_r($account);'
4. Observe that force_password_change has been changed to 0.

Updated patch to apply against the latest 6.x-1.x-dev code. Also changed use of !empty() to isset() for parity with 7.x-1.x-dev.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Reviewed & tested by the community
aohrvetpv’s picture

aohrvetpv commented
Priority: Normal » Critical

I think this is a critical bug that should probably be considered a security issue. Many modules use user_save() to manipulate user data, and in so doing could undo the forced password change. So based on your site configuration, forcing a password change for a user may or may not work, which is a security problem.

For example, if you forced a password change on Drupal 6 for all your users in response to "Heartbleed", who knows if they actually were forced to change their passwords. If their credentials were compromised, they would still be compromised.

aohrvetpv’s picture

aohrvetpv commented

2: force_change_bug-2096349-2.patch queued for re-testing.

  • Commit 50cb1c3 on 6.x-1.x by AohRveTPV: 
    Issue #2096349 by ryan_courtnage, AohRveTPV: Logic error might result in...
aohrvetpv’s picture

aohrvetpv commented
Status: Reviewed & tested by the community » Fixed

Fix committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

aohrvetpv’s picture

aohrvetpv commented

ryan_courtnage, sorry I didn't give proper credit for this commit. At the time I did not realize you had to use the --author parameter to Git.