Showing captcha after 3 attempts

[saud]

I wrote a small PHP with SQLite script which stores the IP with counter, it works fine with one problem, If I enter the correct password 4th time the counter becomes 4 and I get the error to enter my captcha. All the other times, 1-3 and 5 and above count it just works fine. What am I doing wrong with the logic.

 

Attaching the zip file with code.

 

https://drive.google.com/file/d/0B_Wnu0b7d6J-YlVmcWRmbjRuY3M/edit?usp=sharing

[saud]

Forgot to add the code.

<?php
session_start();

$IPaddress = $_SERVER["REMOTE_ADDR"];
$mainpage = "main.php";

$dblogin = new PDO("sqlite:LoginAttempts.db");
$CounterCheck = $dblogin->query("SELECT IP, Counter FROM LoginAttempts WHERE IP = '$IPaddress'");
$fields = $CounterCheck->fetch(PDO::FETCH_ASSOC);

if(isset($_POST['submit']))
{

$error = 0;
$showcaptcha = 0;
$captchaerror = 0;

if (isset($_POST['username']))
{
$username = $_POST['username'];
}
if (isset($_POST['password']))
{
$password = $_POST['password'];
}
if (isset($_POST['imagetext']))
{
$imagetext = $_POST['imagetext'];
}
if(empty($username)) {
$username = 1;
}
if(empty($password)) {
$password = 1;
}


if($fields['Counter'] < 3)
{
if($username == "admin" && $password == "admin")
{
	$_SESSION['logedin'] = 'success';
	
	// Redirect to the page
	header("Location: $mainpage");
	$dblogin->query("UPDATE LoginAttempts SET Counter = '0' WHERE IP = '$IPaddress'");
	exit();
}
else
{
	$error == 1;
	$errormessage = 'Invalid Username or Password';
	$UpdateAttempt = $dblogin->query("UPDATE LoginAttempts SET Counter = Counter + 1 WHERE IP = '$IPaddress'");
	$Updatecount = $UpdateAttempt->rowCount();
	if ($Updatecount == 0)
	{
	$dblogin->exec("INSERT INTO LoginAttempts (IP, Counter) VALUES('$IPaddress', '1')");
	}
}
}

if($fields['Counter'] >= 3)
{
// $showcaptcha = 1;
if(empty($imagetext)) {
    $error = 1;
	$captchaerror = 1;
  } else {
    include "captcha/securimage.php";
    $img = new Securimage();
    $valid = $img->check($imagetext);

    if(!$valid) {
      $errormessagecaptcha = "Invalid Captcha";
	  $captchaerror = 1;
    }
}

if($captchaerror == 1)
{
	$error == 1;
	$errormessagecaptcha = 'Invalid Captcha';
	$UpdateAttempt = $dblogin->query("UPDATE LoginAttempts SET Counter = Counter + 1 WHERE IP = '$IPaddress'");
	$Updatecount = $UpdateAttempt->rowCount();
	if ($Updatecount == 0)
	{
	$dblogin->exec("INSERT INTO LoginAttempts (IP, Counter) VALUES('$IPaddress', '1')");
	}
}
else if($username == "admin" && $password == "admin" && $captchaerror == 0)
{
	$_SESSION['logedin'] = 'success';
	
	// Redirect to the page
	header("Location: $mainpage");
	$dblogin->query("UPDATE LoginAttempts SET Counter = '0' WHERE IP = '$IPaddress'");
	exit();
}
else
{
	$error == 1;
	$errormessage = 'Invalid Username or Password';
	$UpdateAttempt = $dblogin->query("UPDATE LoginAttempts SET Counter = Counter + 1 WHERE IP = '$IPaddress'");
	$Updatecount = $UpdateAttempt->rowCount();
	if ($Updatecount == 0)
	{
	$dblogin->exec("INSERT INTO LoginAttempts (IP, Counter) VALUES('$IPaddress', '1')");
	}
}
}
}
?>

[mac_gyver]

i was going to write a bunch of verbiage about the posted code, but it will be simpler to just post an example -

session_start();

// define these values up front
$mainpage = "main.php";
$hard_user = 'admin';
$hard_pwd = 'admin';

if(isset($_SESSION['logedin']))
{
    header("Location: $mainpage"); // already logged in, go elsewhere and don't run the following code
    die;
}

$IPaddress = $_SERVER["REMOTE_ADDR"]; // not sure why would use the ip, it's the username you are counting bad attempts against
    
// i'm guessing you are using the $fields['Counter'] value later to control the display of the captcha?
$dblogin = new PDO("sqlite:LoginAttempts.db");
$CounterCheck = $dblogin->query("SELECT IP, Counter FROM LoginAttempts WHERE IP = '$IPaddress'");
$fields = $CounterCheck->fetch(PDO::FETCH_ASSOC);
    
if(isset($_POST['submit']))
{ // form was submitted

    $errors = array(); // set values using $errors[] = "the error message you want";
    // if you want to use and test for specific errors, you would use a specific index name $errors['some_index'] = "the error message you want";
    // for form fields, the index name would typically be the form field name so that you can match up errors with the specific location in the form
    
    // you would filter/validate the submitted username/password (empty, min/max length...) and set any $errors[] elements as needed
    $username = $_POST['username']; // in lieu of actual filter/validation code, get a copy for demo purposes
    $password = $_POST['password'];

    // if there are $errors at this point, the username/password values are bad (empty, min/max length...)
    if(empty($errors))
    {
        // no errors, check if the captcha was required
        // if attempt 3,4,... this form submission requires the captcha value and it must match before attempting to login
        if($fields['Counter'] >= 3){
            $imagetext = trim($_POST['imagetext']);
            // check for empty
            if(empty($imagetext)) {
                $errors[] ="The captcha phrase cannot be empty";
            } else {
                include "captcha/securimage.php";
                $img = new Securimage();
                $valid = $img->check($imagetext);
                // check if it matches the expected value
                if(!$valid) {
                    $errors[] = "Invalid Captcha";
                }
            }
        }
    }
    
    // if there are $errors at this point, the username/password values are bad and/or the captcha was required but did not match
    if(empty($errors))
    {
        // no errors, attempt to authenticate the user
        if($username == $hard_user && $password == $hard_pwd)
        {
            // login worked
            $_SESSION['logedin'] = 'success';
            $dblogin->query("UPDATE LoginAttempts SET Counter = '0' WHERE IP = '$IPaddress'");

            // Redirect to the page            
            header("Location: $mainpage");
            exit();
        } else {
            $errors[] = 'Invalid Username or Password';
        }
    }
    
    // at this point, login was unsuccessful, either due to bad username/password values, a bad captcha (when required), and/or wrong username/password
    $query = "INSERT OR IGNORE INTO LoginAttempts (IP, Counter) VALUES ('$IPaddress', 0)"; // start at zero
    $dblogin->exec($query);
    $query = "UPDATE LoginAttempts SET Counter = Counter + 1 WHERE IP = '$IPaddress'";
    $dblogin->exec($query);
}
?>

Edited June 23, 2014 by mac_gyver

[Jacques1]

The approach itself is already wrong.

 

First of all, binding the attempts to the IP address is nonsensical, as mac_gyver already said. One IP does not equal one user. This is a widespread belief, but it's false nonetheless.

 

You're actually helping attackers while punishing legitimate users. An attacker has access to hundreds and thousands of different IP addresses, be it legally through the use of proxies or Tor, be it illegally through botnets. That means they can easily switch to a new address after 3 attempts and start over without having to solve any CAPTCHA. On the other hand, a legitimate user may share her address with many other people due to a proxy, a VPN, a public access point or whatever. That means they may constantly have to solve CAPTCHAs just because somebody with the same IP address entered a wrong password on your site.

 

The next problem is that you've completely forgotten about simultaneous requests. You save the current number of attempts, time passes, and then let the user log in if that number is below the limit. But what if there have been 100 other attempts in the meantime? Then you accept them all based on the obsolete counter value.

 

So an attacker can circumvent your limit simply by sending many simultaneous requests. You again help attackers while punishing legitimate users who dutifully make only one request at a time.

 

To fix this problem, you need to increment the counter and get the new value in one atomic operation. And then you check if that number is 3 or less. So instead of counting the failed log-in attempts, it's more like taking a number: The requests who got 1, 2 or 3 are allowed to submit the credentials directly, everybody else also has to solve a CAPTCHA.

 

Implementing this may be tricky in SQLite, because it's a very simple database system which lacks most advanced features. In MySQL, you would do this with a user variable or LAST_INSERT_ID():

// update the counter and save the new value in a single atomic operation
UPDATE
	users
SET
	login_attempts = (@current_attempt := login_attempts + 1)
WHERE
	user_id = 123
;

// get the updated value
SELECT @current_attempt;