Jump to content

PHP Brute Force Protection


ryanfilard
 Share

Go to solution Solved by oaass,

Recommended Posts

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());
}
}
?>
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

  • Solution

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

Link to comment
Share on other sites

This thread is more than a year old. Are you sure you have something important to add to it?

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

 Share

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.