is this a good approach for user login?

Question

Here is a pretty short, simple and straightforward code I wrote to use for my site to log users in. I was hoping someone could check it out and tell me whether there's anything wrong with it what could use some improvement.

Thanks in advance!

session_start();

/* connect to the db */
define("INCLUDED-PUBLIC", true);
include('dbConnection.php');   

/* define safe variables */
$login = mysqli_real_escape_string($connection, $_POST['login-email']);
$pass = md5($_POST['login-pass']);

/* send the query */
$query = mysqli_query($connection, "
  SELECT `user_id`
    FROM `users`
   WHERE `user_contact_email`='$login'
     AND `user_password`='$pass'
   LIMIT 1
");

/* does such account exist? */
$count = mysqli_num_rows($query);

if ($count > 0){

   /* user exists, loggin' in! */
   $data = mysqli_fetch_array($query);
   $userID = $data['user_id'];
   $_SESSION['user']['user_id'] = $userID;

}

Answer 1

Actually, there is quite some room for improvement.

First: You should add a random salt to the hashing algorithm as to protect against pre-computation attacks. (search for rainbow table)

Second: you should fetch the user_password value from the DB and do the comparison on the PHP side.

Important

Statements that invoke PASSWORD() may be recorded in server logs or in a history file such as ~/.mysql_history, which means that passwords may be read by anyone having read access to that information. See Section 5.3.2, “Password Security in MySQL”.

^{Source: http://dev.mysql.com/doc/refman/5.1/en/encryption-functions.html#function_password}

Third: Do not use MD5 it's a nice hashing algorithm but considered broken for security purposes. If you use PHP 5.3 or later, use crypt, with CRYPT_BLOWFISH.
CRYPT_BLOWFISH in PHP is an implementation of the Bcrypt hash. Bcrypt is based on the Blowfish block cipher, making use of it's expensive key setup to slow the algorithm down.

Also: don't use LIMIT in your query, instead put a UNIQUE constraint on the user_contact_email. Using LIMIT is a trick that could mask duplicate user-emails and cause unexpected results when a duplicate email somehow gets entered.

And finally: Why don't you use a standard library.
Security tends to be a lot more complicated and with more invisible screw up possibilities than most programmers could tackle alone, using a standard library is almost always easiest and most (if not the only) secure option available. (also read: Help me make my password storage safe)

Answer 2

The only thing i would change would be:

$pass = md5($_POST['login-pass']);

To something like:

$pass = hash('whirlpool', $_POST['login-pass']);

And also change how it is stored in the database on registration.

Other than that, its all good.

Answer 3

Things to changes

• include shoud change into require_once
• check that password and email are not empty
• you also check for valid email but invalid password
• free the result set after storing into session
• close the mysqli connection
• if user/email is invalid than send back

my way:

session_start();



require_once('dbConnection.php');
/* connect to the db */
define("INCLUDED-PUBLIC", true);


/* define safe variables */
if(!empty($_POST['login-pass']) && !empty($_POST['login-email']) )
{
    $login = mysqli_real_escape_string($connection, $_POST['login-email']);
    $pass = md5($_POST['login-pass']);

    /* send the query */
    $query = mysqli_query($connection, "
      SELECT `user_id`
        FROM `users`
       WHERE `user_contact_email`='$login'
            AND `user_password`='$pass'
        LIMIT 1
    ");

    /* does such account exist? */
    $count = mysqli_num_rows($query);

    if ($count > 0){
       /* user exists, loggin' in! */
       $data = mysqli_fetch_array($query);
       $_SESSION['user']['user_id'] = $data['user_id'];        
       /* free result set */
       mysqli_free_result($query);
       mysqli_close($connection);
    }
    else {
            header("location:login.php");
            exit();
    }

}

Answer 4

In general your code is okay, only minor improvements can be made.

• counting rows is unnecessary, fetching array will do the same with shorter code.
• some error handling is required. You have to check query result and raise an error if it fails
• code structure and readability.
  Writing readable code lets you omit obvious comments. include('dbConnection.php'); is self-explanatory, isn't it?
  $query variable in your code doesn't contain query, but rather query result. So, you're obfuscating your own code, making it less readable. Always use sensible names, it will save you useless comments.

like this

define("INCLUDED-PUBLIC", true);
include('dbConnection.php');   
$login = mysqli_real_escape_string($connection, $_POST['login-email']);
$pass  = md5($_POST['login-pass']);

$query = "SELECT `user_id` FROM `users` WHERE `user_contact_email`='$login'
                                               AND `user_password`='$pass'";

$result = mysqli_query($connection, $query);
if (!$result) trigger_error(mysqli_error($connection).$query);

if ($data = mysqli_fetch_array($query)){
   session_start();
   $_SESSION['user']['user_id'] = $data['user_id'];
}

However, there are some conceptual improvements can be made

• It's always preferred to use placeholders to insert data into query.
• password salting. that's long and obscure story, everyone gets impressed with it and almost noone understands the matter. But it can be boiled down to just simple thing:
  have site specific salt, defined in your dbconnect file, and salt your password with it and user's email. It won't help if the password is weak, but will help if passwords is strong.
• Having some library to ease database calls is a must:

compare your code to this one:

define("INCLUDED-PUBLIC", true);
include('dbConnection.php');   
$pass  = md5(SITE_SALT.$_POST['login-email'].$_POST['login-pass']);
$query = "SELECT `user_id` FROM `users` WHERE `user_contact_email`=? AND `user_password`=?";
$data  = dbgetone($query, $_POST['login-email'],$pass);
if ($data){
   session_start();
   $_SESSION['user']['user_id'] = $data['user_id'];
}