PHP Brute Force Protection

[ryanfilard]

I am trying to create for the first time a lock out system on my CMS. So is the code below sufficient for locking out impostors. This code is included onto the login page and has a few other files to help support it.

<?PHP
$ip = $_SERVER['REMOTE_ADDR'];
 include("../Connections/default.php");
     mysql_select_db($database_default, $default);
     $query_uvs2 = "SELECT * FROM login WHERE `ip` = '".$ip."' ";
     $uvs2 = mysql_query($query_uvs2, $default) or die(mysql_error());
	 $total_fails = mysql_num_rows($uvs2);
if($_REQUEST["error"] == "1")
{
	 
$lockout = "10";//Maximum lockout attempts.

if($total_fails >= $lockout)
{
	include('functions/standard.php');
	date_default_timezone_set("America/New_York");
	e_log("security", "IP Ban for Brute Force (Possibly Page Refreshing)",$_SERVER['REMOTE_ADDR']);
	die("One does not simply brute force, to appeal this IP ban please empty the table login");
}
else
{
	include("../Connections/default.php");
	mysql_select_db($database_default, $default);
	$addmenu = "INSERT INTO login (`ip`) VALUES ('$ip')";
	mysql_query($addmenu, $default) or die(mysql_error());
}
}
?>

[Psycho]

Several things in there don't make sense to me:

 

1. Why do you do a query using "SELECT *" and then only use the number of rows returned? If you don't need the data, then do a query for the COUNT(). It's also not obvious what the 'login' table represents. Are all logins recorded there or only failures? If all logins are record there then the logic would lock out valid users after 10 logins.

 

2. You only use the data from the first query if ($_REQUEST["error"] == "1"), so it is a waste of running that query if that condition is not true.

 

3. In the else condition, you do a second mysql_select_db() to the same database as before.

 

To help further, I think we need to understand what data is stored in the 'login' table and what ($_REQUEST["error"] == "1") is supposed to represent.

[ryanfilard]

The login table just stores failed attempts.

[oaass]

I think it would be more optimal if you try an approach like the following

$result = mysql_query("SELECT count, last_attempt FROM failed_logins WHERE ip='{$ip}'");
$entry = mysql_fetch_object($result);

if ($entry->attempts <= 10) {
	$now = time();
	$reset = ($entry->last_attempt > ($now - 10));
	$count = ($reset) ? 1 : ($entry->count + 1);

	mysql_query("UPDATE failed_logins SET count={$count},last_attempt='{$now}' WHERE ip='{$ip}'");

	if ($count > 10) {
		$expires = time() * 3600; // Lockout expires in 1 hour
		mysql_query("INSERT INTO lockouts (ip, expires) VALUES('{$ip}', '{$expires}'");
	}
} else {
	$result = mysql_query("SELECT expires FROM lockouts WHERE ip='{$ip}'");
	$entry = mysql_fetch_object($result);

	if ($entry->expires >= time()) {
		// Lockout has expired.
		mysql_query("DELETE FROM lockouts WHERE ip='{$ip}'");
	} else {
		// Not yet expired. Keep the user locked out
	}
}

Instead of having to select count(*) every time you just check against the value of the count field...

 

Be aware, I have not tested this code so bugs should be expected