0

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);

?>
10
  • 11
    Well, at a glance, none of your SQL queries are parameterized, and you appear to be storing passwords in plaintext. Any other issues with this code are likely insignificant compared to those. This isn't the site for a proper security audit or code review though. We focus on specific questions about software design. – Ixrec Mar 24 '16 at 9:44
  • 1
    You forgot the die call after setting the Location header. Probably harmless in this case, but can easily lead to security holes in future versions. – CodesInChaos Mar 24 '16 at 10:16
  • 5
    Don't store plaintext passwords. Hash them using password_hash. – CodesInChaos Mar 24 '16 at 10:18
  • 3
    @CodesInChaos: Wouldn't you like some salt with your hashes? – Marjan Venema Mar 24 '16 at 12:23
  • 2
    Am i the only seeing this? 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. – marstato Apr 23 '16 at 22:41
3

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.

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.