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

Update password.php #2229

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

orchid-hybrid
Copy link

Initial commit adding password hashing support to password fields.

This adds two new options to the password field: Hash (a boolean) and Salt (a string). Hashing is enabled by default but is optional (for backwards compatability) and can be turned off.

We've chosen one iteration and 32 bytes for the size of the derived key. I think these values would be suitable in general.

Note that this means users should not inspect the value of a password field anymore: The way to interface with hashed passwords is to hash the value you want to test it against with the same salt and compare.

Reference for the choice of pbkdf2 with sha1: http://security.stackexchange.com/a/31846/45116

Initial commit adding password hashing support to password fields.

This adds two new options to the password field: Hash (a boolean) and Salt (a string). Hashing is enabled by default but is optional (for backwards compatability) and can be turned off.

We've chosen one iteration and 32 bytes for the size of the derived key. I think these values would be suitable in general.

Note that this means users should not inspect the value of a password field anymore: The way to interface with hashed passwords is to hash the value you want to test it against with the same salt and compare.

Reference for the choice of pbkdf2 with sha1: http://security.stackexchange.com/a/31846/45116
@@ -85,14 +97,19 @@ public function options () {
* @since 2.0
*/
public function schema ( $options = null ) {
$length = (int) pods_var( self::$type . '_max_length', $options, 255 );
if( pods_var( self::$type . '_hash', $options ) ) {
return 'VARCHAR(32)';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be $length = 32;

@sc0ttkclark
Copy link
Member

This looks great, but what steps can we take to ensure values aren't saved and re-hashed over and over?

Improved the schema function so that it sets the length variable.

Added a helper predicate called tagged_hash_string that checks whether a string is of the form "{SHA1}" . 32 hextets.

Updated pre_save to tag the hashes it computes with "{SHA1}" as well as making sure not to re-hash passwords.
@orchid-hybrid
Copy link
Author

I've improved 'schema' by using $length, that's much nicer.

Thanks for pointing this out! I verified that re-hashing like that is an issue in my testing setup and I see two types of solution to this:

(A) Add a tag to hashes so that strings can be tested for whether or not they need to be hashed.
(B) When saving a value into the database, check whether the previous value is equal to the value about to be inserted - only compute the hash if it's not.

I feel that B does not naturally fit and might require performing an extra database lookup so I went ahead and implemented A by adding a "{SHA1}" tag to the hashes and verified that this resolves that issue.

@unknownnf
Copy link
Contributor

This will not work on php 5.2? We need to keep things compatible with the least Wordpress needs, WP is 5.2, hash_pbkdf2 is 5.5.0 or greater.

We could add one of the PHP implementations, maybe, if Sc0tt is allright with it.

@sc0ttkclark
Copy link
Member

What does WordPress use? Why not just use that?

@sc0ttkclark
Copy link
Member

@pglewis
Copy link
Contributor

pglewis commented Jun 11, 2014

I think the best suggestion to avoid re-hashing is also to follow WordPress's lead:

wordpress-password

There is no use in populating the password box after the password has been hashed and saved. The original password is discarded and the hash is of no use. Better to present an empty input and change the label to "change password" or similar.

Also, it would be a nice touch to add another option for "verify", which would display a 2nd input that must match the first before allowing save.

@sc0ttkclark
Copy link
Member

Verify would be cool, but would probably require some more thinking on our side to make it possible to add another field row from input() or another method.

@pglewis
Copy link
Contributor

pglewis commented Jun 11, 2014

One more thing from the brainstorming session in #pods-dev: we should also expose a (public static?) method to check the input against the hash. Implementors need this functionality and the details should be in the field, not external.

To sum up where we are, roughly in order of importance:

  1. Use the WordPress wp_hash_password() function to keep compatibility with older PHP. The default implementation adds salt on its own so the salt option can probably be dropped, one less thing to manage.
  2. Expose a method to check an input against a hash
  3. Do not populate the password field, check for any input to change the password and/or look into using the HTML5 placeholder attribute to distinguish "no password" entered.
  4. A way to clear the password field (use case: post password)
  5. A verify option that would show a 2nd "re-enter password" field that must match the first
  6. An option to display strength meter from WordPress on the password input(s) with the default as being 'on'

If any of these (verify, for example) presents a major technical roadblock then we should create a new enhancement issue for it and push it until later. Having the core features and supporting hashing is more important than having every wish-list item.

@sc0ttkclark says we'll up the bounty on this for the hard work. This is definitely needed functionality so I'm very glad it's getting this attention.

@sc0ttkclark
Copy link
Member

4 and 5 are nice to haves, I'd focus on 1, 2, and 3.

5 may be unrealistic with the current Forms API, there's no way to add another row to certain form UI, and they're not all the same, so it's possible it could get tricky.

I'd like to propose a final number 6: An option to display strength meter from WordPress on the password input(s) with the default as being 'on'.

@orchid-hybrid
Copy link
Author

I think that using wp_hash_password is a great idea since it handles the salts transparently (this is a big improvement on ease-of-use). I've gone and implemented this change to see how it takes and two slight problems came up regarding (2).

First regarding wp_check_password in pluggable.php: This procedure is used by wordpress to test a password against a hash but it also takes a user_id (it automatically fixes up old md5 hashes into bcrypted hashes for the user with that id), if we set user_id to null then it wont perform any database updates like that but it does still call apply_filters( 'check_password', $check, $password, $hash, $user_id ). I'm not sure whether or not that's desirable - to stop these filters being triggered you could duplicate the password testing mechanism like so:

$wp_hasher = new PasswordHash(8, true);
$check = $wp_hasher->CheckPassword($password, $hash);

but the issue with this is that functions in pluggable.php are designed to be replacable, so password testing would actually break on wordpress instances that have a custom password hashing plugin. if calling apply_filters is ok, I'll go ahead and commit that.


Secondly since hashing is optional, the password test mechanism should of course work in both cases - but if a programmer tried to test passwords in the following way:

$pod = pods($name, $id);
$hash = $pod->field("password");
$hash->check_password($pass, $hash);

then check_password has no way to tell if the password is hashed or not, the options set for that field needs to be passed to check_password too or at least a boolean saying whether or not you're comparing against a hash or plaintext (but that doesn't seem good for usability). What kind of interface would work best for this?

@pglewis
Copy link
Contributor

pglewis commented Jun 12, 2014

I'll go over your latest info either late tonight or tomorrow my time. Thanks for continuing on!

@orchid-hybrid
Copy link
Author

Ah the first section of my last post was a bit long, let me summarize:

There's a slight issue with using wp_check_password directly in that it triggers a filter - if that's ok we can use it. If not we could use PasswordHash/CheckPassword from phpass directly but it wouldn't be plugin-replaceable.

I'll investigate (3) now.

@sc0ttkclark sc0ttkclark modified the milestones: Pods 3.0, Pods 3.1 Aug 22, 2014
@sc0ttkclark sc0ttkclark added Status: Help Wanted We have not prioritized this yet, but you can help make it happen to speed it up Status: Need User Feedback Waiting on feedback from user who reported issue Needs Testing and removed Type: Feature Features that add entirely new functionality that was not there before Pulled for Review labels May 29, 2015
@sc0ttkclark sc0ttkclark modified the milestones: Pods 3.1, Pods Future Release May 29, 2015
@lougreenwood
Copy link

Hey, I just wanted to bump this conversation and see whether the core implementation of password hashing is something that could be sponsored so it can be released ASAP?

@lougreenwood
Copy link

bump... :)

@sc0ttkclark
Copy link
Member

@lougreenwood contact me on our Slack to discuss further

@sc0ttkclark sc0ttkclark self-assigned this Dec 12, 2016
@pglewis pglewis removed the Patch label Jun 12, 2018
@sc0ttkclark sc0ttkclark marked this pull request as draft July 15, 2020 16:12
@sc0ttkclark sc0ttkclark changed the base branch from 2.x to main July 15, 2020 16:12
@sc0ttkclark sc0ttkclark dismissed a stale review July 15, 2020 16:12

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Help Wanted We have not prioritized this yet, but you can help make it happen to speed it up Status: Holding Status: Need Research Status: Need User Feedback Waiting on feedback from user who reported issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants