3
\$\begingroup\$

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 <artemkller@gmail.com> <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 <artemkller@gmail.com> <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?

\$\endgroup\$
  • \$\begingroup\$ Hi and welcome to Code Review! This question involves a lot of code and is very open ended, and as such is likely to be closed or go unanswered. I suggest you trim the code down to some specific snippets and ask about those in separate questions. \$\endgroup\$ – l0b0 Jun 18 '13 at 12:20
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I see, any suggestions? \$\endgroup\$ – user1902133 Jun 18 '13 at 13:41
  • \$\begingroup\$ you do a good thing using PDO and prepared statements mistrusting everything that comes from the user. That part of the code should do. But once the coockie is there, you trust that person. But a Coockie is just content send by the browser and is thus easily edited. you should create a hash large enough that it is hard to guess and send that as coockie content. Then when the coockie is send, check wether the hash in the coocke is actually a valid hash. Then proceed. The hash in the coockie acts as a replacement for the username/password combination. \$\endgroup\$ – Pinoniq Jun 18 '13 at 15:01
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ isset is fine. If a checkbox is not checked, it is not sent with the data (as per the HTML specifications -- w3.org/TR/html401/interact/forms.html#h-17.13.3). Browsers typically destroy session cookies when they are closed, thus rendering your second point useless. Also, sessions don't stay alive for 2 weeks on the PHP side (unless you set the session keep alive period to be 2 weeks of course). INSERT ... ON DUPLICATE key is great for MySQL, but why couple yourself to MySQL unnecessarily? \$\endgroup\$ – Corbin Jun 19 '13 at 1:26
  • \$\begingroup\$ isset is fine. Huh, didn't know that. Browsers typically destroy session cookies when they are closed Besides Tor Browser Bundle, I can't think of any that are configured like that out of the box. sessions don't stay alive for 2 weeks True. Use session.gc_maxlifetime for more than one day. why couple yourself to MySQL To avoid repeating yourself. \$\endgroup\$ – Nick ODell Jun 19 '13 at 5:43
  • \$\begingroup\$ Browsers typically destroy session cookies when they are closed Make a page: <?php session_start(); if (!isset($_SESSION['count'])) { $_SESSION['count'] = 0; } echo ++$_SESSION['count'];. Refresh it a few times. Close your browser and go back to it. Notice how it resets? php.net/manual/en/… \$\endgroup\$ – Corbin Jun 19 '13 at 6:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.