Exploitable holes in login script

Question

This is my most recent login system I have been developing. It is working with Sessions. I want to know if I am doing well, if my code contains any serious exploits, and if my logic is correct.

This is the login class, handling everything related to users:

/**
* UserHandler.class
*
* Handling login/logout and others
*
* @Author Jony <[email protected]> <www.driptone.com>
**/

Class UserHandler
{
    /**
    * Properties
    **/

    protected $pdo;
    private $query;
    private $fetch;
    private $delete;
    private $update;
    private $check;
    private $insert;
    private $get;


    /**
    * Constructor
    *
    * Creating MySQL connection using PDO
    **/

    public function __construct($pdo)
    {
        $this->pdo = $pdo;
    }

    /**
    * Method login
    *
    * Logs the user in, securly.
    *
    * @param username The entered username
    * @param password The entered password
    * @param ip The computer's IP
    **/

    public function login($username, $password, $ip)
    {
        /* Checking if there are any attempts with that ip*/

        $this->check = $this->pdo->prepare("SELECT * FROM login_attempts WHERE ip = :ip");
        $this->check->execute(array("ip" => $ip));

        /* Checking if there are any attempt histories with that ip */

        $this->get = $this->pdo->prepare("SELECT * FROM login_attempts_history WHERE ip = :ip");
        $this->get->execute(array("ip" => $ip));

        /* Fetching from login_attempts */

        $this->fetch = $this->check->fetch(PDO::FETCH_ASSOC);

        /* Fetching from login_attempts_history */

        $this->query = $this->get->fetch(PDO::FETCH_ASSOC);

        // If history attempts is more than 20, block user for 1 day.

        if ($this->query['attempts'] > 20)
        {
            $this->update = $this->pdo->prepare("UPDATE login_attempts_history SET blocked = 1 WHERE ip = :ip");
            $this->update->execute(array("ip" => $ip));
        }

        // If not blocked, process..
        if ($this->query['blocked'] != 1)
        {
            // If attempts is less than 6, continue login.
            if ($this->fetch['attempts'] < 6)
            {
                // checking if details are correct.
                $this->check = $this->pdo->prepare("SELECT * FROM users WHERE username = :user AND password = :pass");
                $this->check->execute(array
                (
                    ":user" => $username,
                    ":pass" => $password
                ));

                //Details are correct, login user.

                if ($this->check->rowCount())
                {
                    $this->delete = $this->pdo->prepare("DELETE FROM login_attempts WHERE ip = :ip");
                    $this->delete->execute(array("ip" => $ip));
                    return true;
                }
                // Login failed, storing attempts.
                else
                {
                    // Checking if there are any attempts again.
                    $this->check = $this->pdo->prepare("SELECT * FROM login_attempts WHERE ip = :ip");
                    $this->check->execute(array("ip" => $ip));

                    // If attempts found, update attempts count.
                    if ($this->check->rowCount())
                    {
                        $this->update = $this->pdo->prepare("UPDATE login_attempts SET attempts = attempts + 1 WHERE ip = :ip");
                        $this->update->execute(array("ip" => $ip));

                    }
                    // No attempts found, create row.
                    else
                    {
                        $this->insert = $this->pdo->prepare
                        ("
                            INSERT INTO login_attempts
                            (attempts, ip)
                            VALUES
                            ('1', :ip)
                        ");

                        $this->insert->execute(array
                        (
                            ":ip" => $ip
                        ));
                    }

                    // Checking if there are any histories again

                    $this->check = $this->pdo->prepare("SELECT * FROM login_attempts_history WHERE ip = :ip");
                    $this->check->execute(array("ip" => $ip));  

                    // If yes, update row.
                    if ($this->check->rowCount())
                    {                       
                        $this->update = $this->pdo->prepare("UPDATE login_attempts_history SET attempts = attempts + 1 WHERE ip = :ip");
                        $this->update->execute(array("ip" => $ip));
                    }
                    else
                    {
                        // Not found, create row.
                        $this->insert = $this->pdo->prepare
                        ("
                            INSERT INTO login_attempts_history
                            (attempts, ip, blocked)
                            VALUES
                            ('1', :ip, 0)
                        ");

                        $this->insert->execute(array
                        (
                            ":ip" => $ip
                        ));
                    }   

                    // Details incorrect error.
                    throw new exception ("Details are incorrect!");
                }
            }
            else
            {
                // Ran out of login attempts error
                throw new exception ("You have ran out of login attempts, please wait 5 minutes.");
            }
        }
        else
        {
            // Blocked from system error
            throw new exception ("You have been blocked from our system for 1 day.");
        }

        //Clear limits.
        $this->clearLimits();

        //Clear history.
        $this->clearLoginHistory();
    }

    /**
    * Method clearLimits
    *
    * Clears all limits older than 5 minutes
    **/

    private function clearLimits()
    {
        $this->delete = $this->pdo->prepare("DELETE FROM login_attempts WHERE date < DATE_SUB(NOW(), INTERVAL 5 MINUTE) AND blocked != 1");
        $this->delete->execute();
    }

    /**
    * Method clearLoginHistory
    *
    * Clears all histories older than 1 day.
    **/

    private function clearLoginHistory()
    {
        $this->delete = $this->pdo->prepare("DELETE FROM login_attempts_history WHERE date < DATE_SUB(NOW(), INTERVAL 1 DAY)");
        $this->delete->execute();
    }

    /**
    * Method logOut
    *
    * Logs the user out.
    *
    * @param name The username.
    **/
    public function logOut($name)
    {
        //Checking if that user exists.
        $this->check = $this->pdo->prepare("SELECT * FROM users WHERE username = :name");
        $this->check->execute(array(":name" => $name));

        //If yes, process.
        if ($this->check->rowCount())
        {
            //Update last login date.
            $this->update = $this->pdo->prepare("UPDATE users SET lastLogin = NOW() WHERE username = :name");
            $this->update->execute(array(":name" => $name));
        }
        //Else throw error.
        else
        {
            throw new excpetion ("An error has occurred!");
        }
    }

    /**
    * Method getLastLoginDate
    *
    * Gets the last login date.
    *
    * @param name The username.
    **/

    public function getLastLoginDate($name)
    {
        //Checking if user exists
        $this->check = $this->pdo->prepare("SELECT * FROM users WHERE username = :name");
        $this->check->execute(array(":name" => $name));

        //If yes, process..

        if ($this->check->rowCount())
        {       
            $this->fetch = $this->check->fetch(PDO::FETCH_ASSOC);

            return $this->fetch['lastLogin'];
        }
        else
        {
            // Else, throw error..
            throw new exception ("An error has occurred!");
        }
    }
}

And this is login.php page that lets the user login:

<?php 

    /**
    * Index.php
    *
    * Recovery System v2.0
    *
    * Password recovery for java games
    * are made easy with this script!
    *
    * @Author Jony <[email protected]> <www.driptone.com>
    **/
    session_start();
    require("includes/db.inc.php");
    include("includes/config.inc.php");

    /* Creating our object */

    $user = new UserHandler($pdo);

    if (isset($_COOKIE['remember_me']))
    {
        $_SESSION['user'] = $_COOKIE['remember_me'];
    }
?>

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Recovery System</title>
<link href="css/style.css" rel="stylesheet" type="text/css" />
<link href="css/bootstrap.css" rel="stylesheet" type="text/css" />
<script src="http://ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js"></script>
</head>
<header>
<?php
    if (!isset($_SESSION['user']))
    {
?>
        <div class="container">
            <div class="voting_box">

                <?php
                    if (isset($_POST['submit']) && isset($_POST['user']) && isset($_POST['pass']) && !empty($_POST['user']) && !empty ($_POST['pass']))
                    {
                        try
                        {
                            $user->login($_POST['user'], $_POST['pass'], $_SERVER['REMOTE_ADDR']);
                            $_SESSION['user'] = $_POST['user'];

                            if (isset($_POST['remember']))
                            {
                                setCookie('remember_me', $_POST['user'], time() + 604800000);
                            }
                            header ("location: index.php");
                        }
                        catch (exception $e)
                        {
                            echo '<div class="alert alert-error">'.$e->getMessage().'</div>';
                        }
                    }
                    else if (isset($_POST['submit']))
                    {
                        echo '<div class="alert alert-error">Your fields are empty!</div>';
                    }
                ?>
                <form action="login.php" method="post" id="form">
                    Username:<br />
                    <input type="text" name="user" class="fieldd1" id="user"><br /><br />
                    Password:<br />
                    <input type="password" name="pass" class="fieldd1" id="pass"><br /><br />
                    Remember me?:<br />
                    <input type="checkbox" name="remember"><br /><br />
                    <input type="submit" name="submit" value="login"  class="btnn">
                </form>
            </div>
        </div> 
<?php
    }
    else
    {
        header ("Location: index.php");
    }
?>
</html>

Many people been telling me that I am using **exceptions for the wrong reason. I really find using sessions the best thing, and easiest to handle errors like password and username is incorrect, and more.

Is that fine to do? Are there any exploits in my code?

Answer 1

If I manually set a cookie in my browser with the name 'remember_me', then I am logged in because you are not doing any validation on the cookie to make sure it is a valid cookie and not a forged cookie.

Answer 2

This review will be entirely focused on your UserHandler class.

There's quite a lot to improve.

Let's analize by method:

__construct()

Lets check the first victim:

public function __construct($pdo)

You don't need the public there, since all methods are public by default (since, at least, PHP5.3).

You should use type hinting. This basically limits the received value to a certain type/class.

This is how you should've done it:

function __construct(PDO $pdo)

---

login()

• You are using $this->check and $this->get.

  Since you are using them as local variables, please, remove them. They only slow down your code. Just create a local variable.

• Your whole model is quite sketchy...

  You have a table where you check login attempts and then you have another one to record attempts!

  Allow me to simplify your whole scheme:

  • Create a table with the following fields:
    • ip (VARCHAR(15) PRIMARY KEY)
    • last_attempt (DATETIME)
    • last_failed_attempt (DATETIME)
    • fails (TINYINT(2))
  • Everytime there is an attempt:
    • Fetch from the database the record for that IP
    • Verify the number of failed attempts (fails)
      • If there's 6 fails, don't allow any logins for 5 minutes
      • If there's 20 or more fails, wait 24 hours or until the next day
      • Else, let it proceed
    • Verify if the user exists
      • If it exists, clear the attempts (by setting the fields fails and last_failed_attempt to nothing)
      • If it doesn't exist add or increment the fails fields

  So much easier to reason and change!

• Why are you calling clearLimits() and clearLoginHistory() after you returned true after a successful authentication?

---

clearLimits() and clearLoginHistory()

Why do you use $this->delete? Again, create a local variable! $stmt is the 'default' name for query results, since it returns a PDOStatement.

---

logOut()

Oh, so, it requires that you run this function to store the last login date?

What if I close my tab/browser and walk away, like how many users do since ever?

This will be storing defective information.

To combat this, you can create a field with the old value of the login, before you login. This way, the value will be always present. And that is the value you use to display.

And please, use local variables!

---

getLastLoginDate()

Seriously, USE LOCAL VARIABLES!

And why in the love of God are you fetching the information again from the database, when you could store it all in a session or similar?

---

General considerations:

• Please, don't switch between single-quote and double-quote.

  Pick one and stick with it!

  My preference is single-quotes, since it doesn't need any expantion/interpolation of the variables within it, and doesn't have to interpret escape sequences (except \'), which results in a VERY TINY speed improvement (less OP codes per string).

• Don't use multi-line comments for a single line.

  Really, just use single-line comments (//). To me, it is a little confusing.

• Some comments are completelly useless!

  Some examples:

  // If not blocked, process..
  if ($this->query['blocked'] != 1)
  
  [...]
  
  /* Fetching from login_attempts */
  $this->fetch = $this->check->fetch(PDO::FETCH_ASSOC);
  /* Fetching from login_attempts_history */
  $this->query = $this->get->fetch(PDO::FETCH_ASSOC);
  

  If the variable names were clear, and the whole process wasn't so obscure and over-complicated, you wouldn't need comments.

Answer 3

Couple of remarks:

• It looks like the "Remember Me" checkbox is always considered checked by your program. Use empty, not isset. See this.
• If somebody notices that remember_me contains their username, they'll try changing it to log into another person's account. What you should do instead: Have two variables in $_SESSION
  1. last_seen The last time() that the user was seen.
  2. time_to_expire Set at login based on the remember checkbox. If they check it, set it to 2 weeks. If they don't, set it to an hour.
     If last_seen + time_to_expire < time(), reject the login.
• Instead of writing two statements, use the INSERT ... ON DUPLICATE KEY syntax. https://stackoverflow.com/questions/12136711/update-if-exists-insert-if-not-exists-multiple-rows-in-single-query-in-php-mys/12137154#12137154
• Don't use a separate table to keep track of who's blocked. Just count how many attempts they have. Granted, this means that sometimes bans will be less than 24 hours, but that doesn't seem like it reduces your security.