Secure Password Hashes (phpass) and Password module integration

It would be nice somehow resolve the history constraint javascript. We have a requirement that all new password validations should happen without a page refresh.

I'm not sure porting PasswordHash.php is necessary, maybe some AJAX could help, don't like the async: false part of attached patch, but this way it fits nicely without a need to change password_policy.js.

The problem with synchronous request is that it blocks UI for up to a second, which isn't noticable on a fast server but could be really annoying on slower ones.

Truth be told I considered lack of js support in this patch to be a feature, as the current approach of sending history of hashes embedded in html is IMHO a really bad idea, they are enough of a problem in the database, wouldn't want them laying aroud in browser's cache or being collected by some other user with 'administer users' privilege.

Attached patch makes the request asynchronous and appends to passwordDescription when it finishes, providing much better experience. Had to add additional param to evaluatePasswordStrength(), and change how passwordCheck() was invoked from passwordDelayedCheck() as context was lost, maybe it should be changed to outerItemElement altogether, not sure what the intention was, but currently there is separate state kept for each input (password & confirm). Also got rid of the old embedding hashes in HTML code because of security implications mentioned in previous post, so now both phpass and plain hashes are checked via AJAX.

Setting to reviewed for non-JS part as i didn't change anything there and it works as described, assuming recrit reviewed JS part. Just added some empty lines.

...and removed some trailing spaces.

JS aggregation doesn't affect inline scripts, so I don't see how it could be the cause of described problem.

Added missing var keyword and throbber.

I'm inclined to decline this patch cause I have trouble participating in using phpass. What is the upgrade path to Drupal 7 from users on phpass?

As long as you sticked with the defaults for phpass, that is Portable Hash and 8 iterations or at least stayed with or above D7 DRUPAL_MIN_HASH_COUNT (7), IN THEORY you'd just have to, after first upgrading to D7, make a simple query replacing data in column pass in {users} table with content of column hash from {user_phpass} table.

So backup {user_phpass}, set every {users}.pass with 'phpass' in it to some random MD5, uninstall phpass, upgrade, replace {users}.pass with data from {user_phpass}.hash and it SHOULD work. If you didn't stick with the defaults you'd have to reset all users passwords.

I don't use, and can't support phpass. I get why you're doing this, but I'm probably never personally going to commit this. Maybe you can get support from one of the other maintainers to get it in.

With @deekayen 's comment in mind, here is a simpler approach that works with phpass (2.x), or a direct backport of drupal 7 like we are doing at sony.

1) Let drupal handle the hashing. Just load the hash after password changes and save $user->pass to the history table. Make the table 60 wide to handle new hash size.

2) Look for a backport of the drupal 7's pluggable hashing, and use that to check passwords. This means that drupal 7, phpass, or anything else that implements password.inc will work because you must implement user_check_password.

3) I see what was written #5 about the JS requirement, but can we loose the JS checking for just this one thing? I'm not sure how much it adds to the user experience, and putting the entire hash history out there like that seems like bad news anyway. The drupal 7 port is going to have the same issue. Perhaps pulling in the AJAX stuff above if you REALLY think it's necessary.

I've tested this locally with both secure hashing enabled and with it not, and it seems to work well. More feedback is welcome.

Drupal 7 makes the table 128 wide (just in case).

Also, this seems fragile:

+  if (function_exists('user_check_password')) {

e.g. for phpass I'm not sure the .inc file will always be loaded what that's called.

Yeah, I think making it the d7 default is the way to go then. for the function_exitsts(), I think drupal 7 loads password.inc pretty early in the bootstrap. Does it make sense for phpass to load it in hook_init() or something like that?

Same concern about function_exists (didn't work for me with PHP 5.3.5+XCache), also:
- password_policy's hook_user() op 'insert' is triggered before phpass's, which will result in md5 getting into the history when creating new user (/admin/user/user/create)
- recent security update to phpass (SA-CONTRIB-2011-026) introduced in hook_user() for ops submit/update/insert the following code:
$edit['pass'] = NULL;

which means $values['pass'] will be empty in password_policy_password_submit() on user's edit page (user/%/edit) and following condition won't be met
if ($account->uid && !empty($values['pass'])) {
which means no _password_policy_store_hash(), which means it won't work with phpass

No, Drupal 7 does not load password.inc at all unless passwords are being hashed or re-hashed.

I guess it will be loaded any time a user is saving a new password, but I'm not sure that's in time.

For password policy, I'm not too interested in the history part, I'm mostly interested in length/complexity validation working together with phpass. Is it possible to disable the history part?

Setting $edit['pass'] to NULL is necessary to avoid almost any case of an md5 getting saved to the users table.

I have no problem with what phpass does, just pointing out that supplied patch wasn't tested with recent version of phpass, if at all.
Btw. it also means expiration part of password_policy won't work with new phpass.

function _password_policy_store_password() could be changed to run on hook_user 'after_update' and 'insert' and copy the value from {users}.pass if it's not in the table instead of assuming the hash is md5.

Probably you need a flag to be set during user 'validate' if the pass is not empty.

@pwolanin How about using variable_get('password_inc', 'includes/password.inc'); We are backporting that from d7 as well. If phpass is using it, then we can check if it's set and then specifically include it before we make the call to user_check_password?

I agree that the history should probably just be optionally disabled. I wasn't that interested in that feature anyway (at least right now), it was that it was "helpfully" storing md5 passwords.

@tacituseu thanks for testing. We're not using phpass, but a more direct backport. I'm hoping to have this work for both.

The work we are doing on the d7 backport is here: https://bugs.launchpad.net/pressflow/+bug/792093

I've updated it so that we are now using the variable_get('password_inc'); Either module can then set that and then password_policy can check it and load the inc so that we can be more sure that the functions would be available. Does that work?

phpass 1.x is being deprecated.
new branch 2.x started.

also see Password

@frankcarey - no that's not going to work, since normally that variable is not actually set so that call will always return NULL. I did not even bother including the variable in the 2.x versions of phpass.

@LPCA - yes, this discussion relates to the 2.x versions of phpass only.

Better status.

Could there, at the very least, be a warning/note added to its description and/or a readme file, so the security conscious users can make an informed choice ? Option to disable JS part of this constraint would be even better.

Yikes, after spending several hours trying to figure out why password policy wasn't notifying users that their passwords were expiring or updating the warning time for password resets or setting the creation time for password history... i saw comment http://drupal.org/node/598424#comment-4759936 above and now it all makes sense.

Is there no way to get this to work? Should i try one of the patches above? Thanks for all the help and discussion here.

At this point, with D6 usage dropping and a focus on a new branch, I'm not likely to work on this feature request unless someone would like to take it on and submit a patch. Closing for now since this has not been meaningfully updated in almost two years.

You can't just make that active without some explanation...

#2008278: _password_policy_store_password should not use md5() was marked as a duplicate.
Maybe recent security incident will result in a change of perspective.

Implemented this in #2309509: Not compatible with Secure Password Hashes (phpass). Sorry, I was not aware of this issue. This issue is a good check on the implementation, though.

To solve the indeterminate execution ordering problem of Secure Password Hashes using hook_user($op='update') to rehash the password and Password Policy also using hook_user($op='update') to store the hash to history, I weighted the module 1. This is a terrible, regrettable hack and I probably should have used after_update as suggested in #26. (I did not think of it.)

Will look into changing to use after_update and removing the module weighting.

Created new issue to improve existing implementation: #2416407: Improve Secure Password Hashes integration

Here is the commit that added Secure Password Hashes support, for reference:
http://cgit.drupalcode.org/password_policy/commit/?id=3453371