Jump to content

Brute Force prevention - little stuck

moosey_man1988

By moosey_man1988
in PHP Coding Help

 Share
Go to solution Solved by Jacques1,

Recommended Posts

moosey_man1988 Member

moosey_man1988

Posted
Posted (edited)

Hi Everyone

 

So I've been recently working on a script which will store login IP's and ban them on failed attempts

 

Although I have a little problem, for some reason it doesnt redirect to the banned page when they should be, unless there is a successful login or if they come off of the login page

 

so they can keep bruteforcing as much as they like all the time they stay on that page.

 

Have i got something wrong?

 

please bear in mind I haven't put in place when login successful remove the warnings count.

 

here is the login.php 

<!DOCTYPE html>
<?php 

require ('../Functions/functions.php');
require('../database/pdo_connection.php');

//first lets get the IP that logged in
$login_ip = $_SERVER['REMOTE_ADDR'];

securityBanCheck($login_ip);

if (isset($_POST['Login'])) {
//Set session timeout to 2 weeks
session_set_cookie_params(1*7*24*60*60);

session_start();
$error=''; // Currently A Blank Error Message

$username=$_POST['userName'];

//Grab the hash
$selectPasswordHash = "SELECT username,secret FROM MC_users WHERE email=email=:username OR username=:username";
$hashresult= $pdo->prepare($selectPasswordHash);
$hashresult->bindParam(':username', $username);
$hashresult->execute();

$row = $hashresult->fetch(PDO::FETCH_ASSOC);

$hash = $row['secret'];
//got the hash

//lets verify it
if (password_verify($_POST['password'],$hash) === true){
//login correct	
$login_user = $row['username'];
//Set the Session
$_SESSION['login_user']=$login_user;
// Redirect to dashboard.php
header ("Location: ../dashboard.php"); 

} else {

$error = 'Username or Password in invalid';
$error2 = 'Try Logging in with your email instead';

//Bruteforce Detection

//lets check if there is already warnings
$checkWarnings = "SELECT Warning_count FROM MC_login_security WHERE Warning_count > 0 AND Login_IP=:loginIP ORDER BY Timestamp DESC";
$warningsResult = $pdo->prepare($checkWarnings);
$warningsResult->bindParam(':loginIP',$login_ip);
$warningsResult->execute();

$warningAmount = 1;

$banTime = 0;

if ($warningsResult->rowCount() > 0){

$warningRow = $warningsResult->fetch(PDO::FETCH_ASSOC);

$warningRowCount = $warningRow['Warning_count'];

$warningAmount = $warningRowCount + 1;

securityBanCheck($login_ip);

}
//Lets log this in the DB

$insertWarning = "INSERT INTO MC_login_security (Login_user_name,Login_IP,Warning_count,timestamp) VALUES (:loginUser,:Login_ip,:warningAmount,:dateToday)";
$insertResult = $pdo->prepare($insertWarning);
$insertResult->execute(array(':loginUser'=>$username,
							 ':Login_ip'=>$login_ip,
							 ':warningAmount'=>$warningAmount,
							 ':dateToday'=>date('Y-m-d H:i:s')));
}

}
//Lastly if the user is logged in, point them back to the Dashboard
if(isset($_SESSION['login_user'])){
header("location: ../dashboard.php");}


?>

the security check function which is called at the start of the login page AND if the password is entered incorrectly

 

function securityBanCheck($login_ip){

//call the url function for redirects
url();
$todaysDate = date('Y-m-d H:i:s');
	
require (PHP_PATH .'/database/pdo_connection.php');

$checkBan = "SELECT Warning_count,Timestamp,Ban_time FROM MC_login_security WHERE Warning_count > 4 AND Login_IP=:loginIP ORDER BY Timestamp DESC";
$checkResult = $pdo->prepare($checkBan);
$checkResult->bindParam(':loginIP',$login_ip);
$checkResult->execute();


if ($checkResult->rowCount() > 0){
	
$banRow = $checkResult->fetch(PDO::FETCH_ASSOC);
$warningCount = $banRow['Warning_count'];
$timeStamp = $banRow['Timestamp'];
$bantime = $banRow['Ban_time'];

echo "did we get here?";

//if theyre banned direct them to the banned page and stop this script from going any further
if ($bantime > $todaysDate){
header('Location: '. SERVER_PATH .'banned.php');	
	die();
}

//if theyre currently not banned check their warnings and if needed add a ban.

if ($warningCount == 4){
$bantime = date('Y-m-d H:i:s', strtotime($timeStamp . '+1 hour'));

echo $bantime;	
	
}elseif ($warningCount == 9){
$bantime = date('Y-m-d H:i:s', strtotime($timeStamp . '+1 day'));	
}elseif ($warningCount == 14){
	
$bantime = date('Y-m-d H:i:s', strtotime($timeStamp . '+1 month'));	
}

//ultimately if we got to this stage we would be adding a ban
$insertBanTime = "UPDATE MC_login_security SET Ban_time = :banTime WHERE Login_IP = :loginIP ORDER BY Timestamp DESC";
$banResult = $pdo->prepare($insertBanTime);
$banResult->execute(array(':banTime'=>$bantime,
						  ':loginIP'=>$login_ip));;
}
	
}

Any help given is greatly appreciated and i thank everyone in advance for all the support I get form this amazing forum.

 

thanks Mooseh

Edited by moosey_man1988
Link to comment
https://forums.phpfreaks.com/topic/299593-brute-force-prevention-little-stuck/
Share on other sites
  • Solution
Jacques1 Prolific Member

Jacques1

Posted
  • Solution
Posted (edited)

You haven't really solved anything, because the code has fundamental problems beyond this little redirect issue.

  • Your code has a time-of-check-to-time-of-use defect (like in your previous thread, by the way): You first check the current warning counter, and if it's below the limit, you allow the user to log in. But who says there haven't been any attempts in the meantime? If an attacker sends a large number of concurrent requests, your application will accept all of them, because it still “sees” the old counter value. After a short while, the counter will be incremented, but then it's already too late.
  • Banning IP addresses is a bad idea, because it doesn't stop attackers and at the same time hurts legitimate users. An attacker can easily switch to a different IP address and start over (botnets are cheap, and there are countless public proxies). But a legitimate user normally uses just a few IP addresses which may be shared by thousands of other people. If one person exceeds the log-in limit, everybody will be banned for an entire month. In other words: After a short while, you'll have locked out half of the world population. This makes no sense whatsoever.
  • The whole implementation is very cumbersome with lots of duplicate code and no clear structure.

Personally, I think that brute-force checks are rather pointless and give users a false sense of security. Anyway, if you insist on implementing this, you need a very different approach:

  • Assign a counter to each user account, not each IP address. In addition to that, you could maintain a global counter to recognize attacks against multiple accounts.
  • Apply a less user-hostile form of protection. For example: Instead of banning people, make them solve CAPTCHAs. Or implement small delays. If you insist on bans, use realistic time limits (not a whole month!) and give your users a chance to unlock their account (e. g. via the I-forgot-my-password procedure).
  • The check and the increment of the counter must happen in a single atomic operation so that the limit cannot be circumvented with concurrent requests. For example:
// update the counter, and simultaneously save the new value in a user variable
$updateLoginAttemptsStmt = $databaseConnection->prepare('
    UPDATE
      users
    SET
      login_attempts = (@login_attempts := login_attempts + 1)
    WHERE
      user_id = :user_id
');
$updateLoginAttemptsStmt->execute([
    'user_id' => $userID,
]);

// get the counter value of the previous operation
$loginAttempts = $databaseConnection->query('SELECT @login_attempts')->fetchColumn();

if ($loginAttempts <= MAX_LOGIN_ATTEMPTS)
{
    // user may try to log-in
}
else
{
    echo 'You have exceeded the maximum number of log-in attempts.';
}
Edited by Jacques1
  • Like 1
QuickOldCar Regular Member

QuickOldCar

Posted
Posted

I agree with Jacques on this, just marking each users account attempted logins, not ip's

You also have to consider if some clients all using the same ip from same building or households.

 

It's fine if want to try and limit some excessive login attempts, but have to ensure those failed ones wipe out upon successful logins.

Some people don't have that stable an internet connection, private browsing and clears every browser close.

This thread is more than a year old. Please don't revive it unless you have something important to add.

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
Go to topic listing
×
×
  • 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.