Extend lockout time; new activation method

Except my diff above is backwards, DOH!

Here’s the real one:

181a182,188
> 		// If this IP is already locked-down, then just extend the time of the lockdown!
> 		$lockID = isCurrentlyLockedDown();
> 		if($lockID != "") {
> 			$results = $wpdb->query("UPDATE $table_name SET release_date = date_add(now(), INTERVAL " .
> 				$loginlockdownOptions['lockout_length'] . " MINUTE) " .
> 				"WHERE lockdown_ID = " . $wpdb->escape($lockID) . "");
> 		} else {
188a196
> }
205a214,223
> function isCurrentlyLockedDown() {
> 	global $wpdb;
> 	$table_name = $wpdb->prefix . "lockdowns";
> 	$ip = $_SERVER['REMOTE_ADDR'];
> 	$lockID = $wpdb->get_var("SELECT lockdown_ID FROM $table_name " .
> 					"WHERE release_date > now() AND " .
> 					"lockdown_IP = '" . $wpdb->escape($ip) . "'");
> 	return $lockID;
> }
>
375,376c393
< 	$activatestr = str_replace(WP_PLUGIN_DIR . "/", "activate_", __FILE__);
< 	add_action($activatestr, 'loginLockdown_install');
---
> 	register_activation_hook( __FILE__, 'loginLockdown_install' );
426,429c443,447
< 		if ( "" != isLockedDown() ) {
< 			return new WP_Error('incorrect_password', "<strong>ERROR</strong>: We're sorry, but this IP range has been blocked due to too many recent " .
< 					"failed login attempts.<br /><br />Please try again later.");
< 		}
---
> 		// Commented so we can extend the lockout period if they are already locked out (now handled in lockDown())
> 		// if ( "" != isLockedDown() ) {
> 		// 	return new WP_Error('incorrect_password', "<strong>ERROR</strong>: We're sorry, but this IP range has been blocked due to too many recent " .
> 		// 			"failed login attempts.<br /><br />Please try again later.");
> 		// }

ARGH! I’m sorry for spamming this topic, but I found a loophole in my code processing already-locked-out users. It would only extend their lockout period if they had failed the required number of times within the retry period each time. This update will extend their lockout period if their IP address is locked out regardless of how many they have had in the retry period. In other words, if they get locked out, then try again five minutes later (passed the retry period but while their IP is still locked out) they will extend their lockout period.

Another reason for this is to remove any calls to isLockedOut() because it assumes the entire class C subnet should be locked out because of one misbehaving client. If we have an internal user fail to log in properly, it will lock out all local users within their subnet which is unacceptable in our environment.

Here’s the (hopefully) final diff:

181a182,188
> 		// If this IP is already locked-down, then just extend the time of the lockdown!
> 		$lockID = isCurrentlyLockedDown();
> 		if($lockID != "") {
> 			$results = $wpdb->query("UPDATE $table_name SET release_date = date_add(now(), INTERVAL " .
> 				$loginlockdownOptions['lockout_length'] . " MINUTE) " .
> 				"WHERE lockdown_ID = " . $wpdb->escape($lockID) . "");
> 		} else {
188a196
> }
205a214,223
> function isCurrentlyLockedDown() {
> 	global $wpdb;
> 	$table_name = $wpdb->prefix . "lockdowns";
> 	$ip = $_SERVER['REMOTE_ADDR'];
> 	$lockID = $wpdb->get_var("SELECT lockdown_ID FROM $table_name " .
> 					"WHERE release_date > now() AND " .
> 					"lockdown_IP = '" . $wpdb->escape($ip) . "'");
> 	return $lockID;
> }
>
375,376c393
< 	$activatestr = str_replace(WP_PLUGIN_DIR . "/", "activate_", __FILE__);
< 	add_action($activatestr, 'loginLockdown_install');
---
> 	register_activation_hook( __FILE__, 'loginLockdown_install' );
426,429c443,447
< 		if ( "" != isLockedDown() ) {
< 			return new WP_Error('incorrect_password', "<strong>ERROR</strong>: We're sorry, but this IP range has been blocked due to too many recent " .
< 					"failed login attempts.<br /><br />Please try again later.");
< 		}
---
> 		// Commented so we can extend the lockout period if they are already locked out (now handled in lockDown())
> 		// if ( "" != isLockedDown() ) {
> 		// 	return new WP_Error('incorrect_password', "<strong>ERROR</strong>: We're sorry, but this IP range has been blocked due to too many recent " .
> 		// 			"failed login attempts.<br /><br />Please try again later.");
> 		// }
443c461
< 			if ( $loginlockdownOptions['max_login_retries'] <= countFails($username) ) {
---
> 			if ( $loginlockdownOptions['max_login_retries'] <= countFails($username) || isCurrentlyLockedDown() != "" ) {

Another reason for this is to remove any calls to isLockedOut() because it assumes the entire class C subnet should be locked out because of one misbehaving client. If we have an internal user fail to log in properly, it will lock out all local users within their subnet which is unacceptable in our environment.

I should put in an option to not lock out the entire subnet for sure, but either way that will only benefit systems if the web server is on the same network as the users. If it is an external web server often times everyone in the office can have the same IP address (ie. that of the router), so one person who can’t remember their password affects everyone. I’ve run into this before, unfortunately.

Since the plugin isn’t meant to protect sites from trusted users a better option would be IP or even subnet whitelisting, which is what I intended to add in this last release, but didn’t get to. I am going to try and get another version that includes that pushed by next week.

Thanks for the feedback!

-Michael

You betcha. Thanks for the great work on a great plugin!

By the way, it won’t just affect people that have a web server in the same subnet as users … it will also affect folks who have an internal blog server in one internal subnet, and users in another internal subnet; Plus, whitelisting is an EXTREME no-no in our environment, since sometimes even internal users get their systems infected.

Sometimes the worst damage comes from folks inside the perimeter that get compromised.

Anyway, 2 cents.

??