PHP login script

Question

I'm hoping someone would be able to identify if my code is prone to SQL injections, and just overall see if there is anything that could be done better.

This is also my first time using MySQLi prepared statements in a login script.

<?php
include('../include/sessions.php');

if(isset($_POST['username']))
{
  $username = mysqli_real_escape_string($dbc, $_POST['username']);
  $password = mysqli_real_escape_string($dbc, $_POST['password']);

  $select = "SELECT username, firstname, lastname, email, 
             userlevel, `password` FROM users WHERE username = ?;";

  $stmt = mysqli_stmt_init($dbc);

  if(!mysqli_stmt_prepare($stmt, $select))
  {
    echo "Error: " . mysqli_stmt_error($stmt);
  }
  else
  {
    mysqli_stmt_bind_param($stmt, "s", $username);
    mysqli_stmt_execute($stmt);
    $result = mysqli_stmt_get_result($stmt);
    $row = mysqli_fetch_assoc($result);

    $dbusername = htmlentities(stripslashes($row['username'])); 
    $dbfirstname = htmlentities(stripslashes($row['firstname']));
    $dblastname = htmlentities(stripslashes($row['lastname']));
    $dbemail = htmlentities(stripslashes($row['email']));
    $dbuserlevel = htmlentities(stripslashes($row['userlevel']));
    $dbpassword = htmlentities(stripslashes($row['password']));

    if(password_verify($password, $dbpassword))  // used bcrypt to hash password
    {
      // not sure if this is the best way to set the sessions
      $_SESSION['username'] = $dbusername;
      $_SESSION['firstname'] = $dbfirstname;
      $_SESSION['lastname'] = $dblastname;
      $_SESSION['email'] = $dbemail;
      $_SESSION['userlevel'] = $dbuserlevel;

      header("Location: ../cust/home.php");
    }
    else
    {
      echo "The username/password combination does not match our records.  Please try again.";
    }
  }
}

?>

My sessions.php looks like this:

<?php
if(!isset($_SESSION)){session_start();} 
include("database.php");

$username = $_SESSION['username'];
$firstname = $_SESSION['firstname'];
$lastname = $_SESSION['lastname'];
$email = $_SESSION['email'];
$userlevel = $_SESSION['userlevel'];

// I started using the below if statement in conjunction with a .htaccess file to prevent 
// anyone from attempting navigate the directories through the URL.
// I am sure there is another way to do this.
if($username == "" || $_SESSION['username'] == "" || $userlevel != '9')
{
  header('Location: ../index.php');
}
?>

Using this code, I am successfully able to login and set the session variables.

Answer 1

There is one logical inconsistency and two security issues. All other improvements will be just removal of useless or misplaced code.

User not found case.

If a user makes a typo in the username, your code would tell them nothing, which is not very user friendly (and also will raise a notice in PHP, which is not very programmer-friendly as well). Better to send an error message if either a username or a password is incorrect. To do so, just add a condition to your if statement to see whether a user was found or not:

if($row && password_verify($password, $_POST['password']))

Useless, repeated and misplaced code.

There are two functions which shouldn't be in your code at all, mysqli_real_escape_string() and stripslashes().

1) Despite a very common delusion, mysqli_real_escape_string() is not intended to prevent SQL injection, and should never be used for such a purpose. The only use for this function is to escape special characters in the SQL string literals. And as there are none in your code, there should be no such function call as well.

Given you are using prepared statements (as you should!), means you never put a variable into SQL directly, there is absolutely no use for mysqli_real_escape_string() function, so you can forget it forever.

2) While stripslashes() is just useless. There are no circumstances that can make this function's use justified (in case there is, it's a problem with your own code and you should fix the cause, not consequence).

3) Separate variables for the every field are superfluous as well. Why not to make just one assignment,

$_SESSION['user'] = $row;

and then just access any user's property through a nested call, i.e $_SESSION['user']['username']?

4) mysqli_stmt_init() is also superfluous as mysqli_stmt_prepare() already does that.

5) echo "Error: " . mysqli_stmt_error($stmt); beside being insecure, is also superfluous as well, there is a way to avoid this code snippet as well. Instead of checking every mysqli operation's result manually, we can ask mysqli to report errors by itself. To do so, add the following line to your database.php:

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);

and you won't have to use such a condition like if(!mysqli_stmt_prepare($stmt, $select)) anymore.

6) Technically, object syntax is less verbose,

mysqli_stmt_get_result($stmt);

vs.

$stmt->get_result();

so I would advise you to adopt the object syntax as well; there is nothing complicated in it.

7) Finally, htmlentities() call is misplaced.

As you probably know, it is advised to use punctuation characters in passwords. Imagine I am using & symbol in mine. htmlentities() would make & sequence from it, changing the password I entered and and I will never make to your site, which won't make me very happy.

Such a function should be used for the output only. And as there is no output in the present code- there should be no such function call as well.

In the end, the login code could be

<?php
include('../include/sessions.php');

if(isset($_POST['username']))
{
    $select = "SELECT username, firstname, lastname, email,
                 userlevel, `password` FROM users WHERE username = ?;";
    $stmt = $dbs->prepare($select);
    $stmt->bind_param("s", $_POST['username']);
    $stmt->execute();
    $result = $stmt->get_result();
    $row = $result->fetch_assoc();

    if($row && password_verify($_POST['password'], $row['password']))
    {
        $_SESSION['user'] = $row;

        header("Location: ../cust/home.php");
        die;
    } else {
        echo "The username/password combination does not match our records.  Please try again.";
    }
}

Security

Like any other HTTP header, Location is only advisory for the browser. And can be easily ignored. Which means your sessions.php (or a script included it) will continue executing, revealing any sensitive data it is intended to protect.

As a rule of thumb, always follow header('Location: ... statement with the die command, to prevent any further execution. So I would rewrite a condition in sessions.php as follows

if(empty($_SESSION['user']['username']) || $_SESSION['user']['userlevel'] != '9')
{
    header('Location: ../index.php');
    die;
}

Another security issue is revealing an error message to a potential attacker. Error messages could contain vital information that could help a malicious person to break your site. Therefore, no error message should be thrown to the screen unconditionally. Error messages should be shown on-screen only during development, but on a live site they must be silently logged. There is an article I wrote in the error reporting basics which you are welcome to read.