How to secure my authentication code?

Question

I'm working a PHP project where I need to authenticate users to a portal. Just wanted to get your opinion on the code, statistics of the website show some strange behaviour with the login form. Wondering whether the code is strong enough.

conn.php

<?php
if(!isset($_GET['login']) && !isset($_GET['pwd']))
{
    header('Location: index.php');
}
else
{
    if(!preg_match('/^[[:alnum:]]+$/', $_GET['login']) or
!preg_match('/^[[:alnum:]]+$/', $_GET['pwd']))
    {
        echo 'Only alphanumeric characters are allowed';
        exit();
    }
    else
    {
        require('config.php');

        $login = $_GET['login'];
        $pwd = $_GET['pwd'];

        $sql = "SELECT * FROM users WHERE user='".mysql_escape_string($login)."'";

        // Check if user exists
        $request_1 = mysql_query($sql) or die ( mysql_error() );

        if(mysql_num_rows($request_1)==0)
        {
            echo 'User does not exist!';
            exit();
        }
        else
        {
            $request_2 = mysql_query($sql." AND pass='".$pwd."'") or die ( mysql_error() );

            if(mysql_num_rows($request_2)==0)
            {
                $result = mysql_fetch_array($request_1, MYSQL_ASSOC);
                $lastconn = explode(' ', $result["dates"]);
                $lastday = explode('-', $lastconn[0]);

                $nbr_trial = $result["nbr_connect"];
                if($lastday[2]==date("d") && $MAX_trial==$nbr_trial)
                {
                    echo 'Too many connection attempts!<br/>';
                    exit();
                }
                else
                {
                    $nbr_trial++;
                    $update = "UPDATE users SET nbr_connect='".$nbr_trial."', dates=NOW()
WHERE id='".$result["id"]."'";
                    mysql_query($update) or die ( mysql_error() );
                    echo 'Incorrect login or password';
                    exit();
                }
            }
            else
            {
                $result = mysql_fetch_array($request_2, MYSQL_ASSOC);
                $nbr_trial = 0;
                $update = "UPDATE users SET nbr_connect='".$nbr_trial."', dates=NOW()
WHERE id='".$result["id"]."'";

                mysql_query($update) or die ( mysql_error() );

                header('Location: portal/index.php');
            }
        }
    }
}
?>

config.php : 

<?php

$DB_serveur = 'localhost';
$DB_utilisateur = 'root';
$DB_motdepasse = '******';
$DB_base = 'siht_portal';

define('_MAX_trial', 3);

?>

Answer 1

There are a couple of things that come into mind. This list is most likely not complete, though.

1. Are you sure about this if statement?

   if(!isset($_GET['login']) && !isset($_GET['pwd']))

   Maybe you should go to index.php when login OR password is not set (instead of AND).

2. mysql_escape_string is not neccessarily secure, and also mysql_real_escape_string could lead to problems as you can read here: https://security.stackexchange.com/questions/8028/does-mysql-escape-string-have-any-security-vulnerabilities-if-all-tables-using-l

   AFAIK you should go with prepared statements: http://php.net/manual/de/pdo.prepared-statements.php

3. Generally, I wouldn't recommend to give any more information as absolutely neccessary. A potential attacker could try different user names until he/she doesn't get the "User does not exist!"-error message. By then, he/she already found out 50% of the login credentials! I'd recommend to just reply "Login failure" or something alike, REGARDLESS of whether the username or the password was incorrect.

4. Obviously, you store the password in the table as plain text. This is a problem, because if somebody stole the table data (or even the whole database), the attacker has all the passwords. Consider hashing to avoid this problem: http://php.net/manual/en/faq.passwords.php

5. Most people consider it better to store database credentials in a separate file: https://security.stackexchange.com/questions/13353/password-in-file-php

6. You are using GET instead of POST, as pointed out by marstato: The OP is reading username and passwort from $_GET. That almost entirely thwarts SSL and the usage of password input boxes (type="password" instead of type="text"). Credentials will show up in Logfiles on many, many places they really dont belong.

7. You are not escaping the password in your query. Anyone can login as anyone else e.g. user with username "Admin" and password "%' AND ''=" would return valid data in the query result and pass the login tests.