PHP Secure Login Scripts

Question

I have created this login script for a website I'm making; basically I want to make it as secure as possible, but also I want it to keep users logged in for 30 minutes. For example if I've logged in but accidentally go back to the login page for some reason I want it to recognise I've already logged in and take me to the home page. I also want to make sure my script is as secure as possible with no possibility of being SQL injected.

Please could someone have a look at my script and let me know how I can improve it? Also how can I make it so each page checks for a login first and cannot be viewed if the user isn't logged in?

authenticate.php

<?php
ini_set('display_errors', 1); ini_set('display_startup_errors', 1); 
error_reporting(E_ALL);
//Session 
session_start();


require 'connection.php';


//If the POST var "login" exists (our submit button), then we can
//assume that the user has submitted the login form.
if(isset($_POST['login'])) {

//Retrieve the field values from our login form.
$username = !empty($_POST['username']) ? trim($_POST['username']) : null;
$passwordAttempt = !empty($_POST['password']) ? trim($_POST['password']) : null;

//Retrieve the user account information for the given username.
$sql = "SELECT * FROM userAccounts WHERE user_email = :username";
$stmt = $usersConn->prepare($sql);

//Bind value.
$stmt->bindValue(':username', $username);

//Execute.
$stmt->execute();

//Fetch row.
$user = $stmt->fetch(PDO::FETCH_ASSOC);
$activationStat = $user['user_activationStatus'];
//If $row is FALSE.
if($user === false){
//Could not find a user with that username!
//PS: You might want to handle this error in a more user-friendly manner!
die('Incorrect username / password combination!');
} else {
//User account found. Check to see if the given password matches the
//password hash that we stored in our users table.

//Compare the passwords.
$validPassword = password_verify($passwordAttempt, $user['user_password']);

//If $validPassword is TRUE, the login has been successful.
if($validPassword){
    if ($activationStat == 1) { 
        //Provide the user with a login session.
        $_SESSION["user_id"] = $user['user_id'];
        $_SESSION['user_name'] = $user['user_firstName']." ".$user['user_lastName'];
        $_SESSION['user_security'] = $user['user_securityID'];
        $_SESSION["logged_in"] = time();

        //Error Reporting
        //ini_set('display_errors', 1); ini_set('display_startup_errors', 1); error_reporting(E_ALL);

        //Redirect to our protected page, which we called home.php
        ?>
            <script type="text/javascript">
                //alert("Login successful!");
                alert("Login Successful!");
            </script>
        <?php
    } else {
        ?>
            <script type="text/javascript">
                alert("Sorry! Your account has not yet been verified!");
                window.location.href = "../index.php";
            </script>
        <?php        }
} else{
    //$validPassword was FALSE. Passwords do not match.
    die('Incorrect username / password combination!');
}
}

}

?>

login.html

<div class="login">
<h1>Login</h1>
<form action="scripts/authenticate.php" method="post">
    <label for="username">
        <i class="fas fa-user"></i>
    </label>
    <input type="text" name="username" placeholder="Username" id="username" required>
    <label for="password">
        <i class="fas fa-lock"></i>
    </label>
    <input type="password" name="password" placeholder="Password" id="password" required>
    <input type="submit" name="login" value="Login">
</form>

connection.php

    try {
    $usersConn = new PDO('mysql:host=HOSTNAME HERE;dbname=DB NAME HERE;charset=utf8', 'USERNAME HERE', 'PASSWORD HERE');  
    // echo 'client version: ', $conn->getAttribute(PDO::ATTR_CLIENT_VERSION), "\n";
    // echo 'server version: ', $conn->getAttribute(PDO::ATTR_SERVER_VERSION), "\n";
    $usersConn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $usersConn->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
}
catch(PDOException $err) {
    var_dump($err->getMessage());
    die('...');
}

Answer 1

The only thing I see that I would like to see changed is this

if($user === false){
      die('Incorrect username / password combination!');
} else {
    ...
}

Here you are relying on "very" strict conditions to fail the login, if that's not met for any reason they may get logged in....

It's not terrible because the other things that you should be doing your doing, it's just better if the login condition is strict and the failure is easy. If that makes sense.

Just sort of a best practice thing to keep in mind.

Here is an example of what can happen:

<?php
 //$user is undefined
if($user === false){
      die('Incorrect username / password combination!');
} else {
    echo "pass";
}

Output

 pass

Sandbox

As I said it's not terrible because your doing everything else right. But it's something I would change, just because.

The only other thing is you could get rid of some of these local variables that are one time use like this:

//Compare the passwords.
$validPassword = password_verify($passwordAttempt, $user['user_password']);

//If $validPassword is TRUE, the login has been successful.
if($validPassword){

Could just be

 if(password_verify($passwordAttempt, $user['user_password'])){

But, I get that it's easier to debug and all that when it's more verbose, so that is just my preference.

Answer 2

The code is technically safe.

There are some side effects that could help a hacker to penetrate elsewhere, namely a flawed error reporting. The error reporting must be flexible, allowing the same code base behave differently based on the server's role:

• on a dev server all errors are better to be shown on-screen (which your code does)
• but on a live server not a single word from the system error message should be shown outside.

To serve these rather contradicting demands your code should never output errors by itself but let PHP to do it instead. And then PHP could be told what to do with error messages by means of couple configuration settings, one of which, display_errors you are already using.

So, long story short, all this fuss is about this small part, var_dump($err->getMessage());. but you must remember that hackers are cunning. They could overload your database server to make PHP throw an error on connection, and then get a lot of useful information from the error message. To prevent that, just never have a code like this. Most of time an exception thrown must be left alone. But the connection is a special case, as it reveal the database credentials in the stack trace. To prevent this, you can just re-throw the exception, as it shown in my article on connecting to MySQL using PDO. So connection.php could be rewritten this way:

try {
    $usersConn = new PDO('mysql:host=HOSTNAME HERE;dbname=DB NAME HERE;charset=utf8', 'USERNAME HERE', 'PASSWORD HERE');  
    $usersConn->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $usersConn->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
}
catch(PDOException $err) {
    throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

so it will never leak anything sensitive, neither in the browser on a properly configured live server, nor even leak the database password into the logs.