Is my login script secure – how to improve it?

Question

I have a login system for my website that works as follows:

In the database I have a table called users with the fields user, password and authentication_token.

user gets set manually by request in the database.

When a user activates his account, the following script stores his password and an authentication_token to the matching user in the database:

// store passwort and authentication_token when user activtes account
$password = password_hash($_POST['password'], PASSWORD_DEFAULT)
$authentication_token = bin2hex(random_bytes(64));
$stmt = $pdo->prepare("UPDATE users SET password = :password, authentication_token = :authentication_token WHERE user = :user");
$stmt->execute(array(':user' => $_POST['user'], ':password' => $password, ':authentication_token' => $authentication_token));

password contains the hash of the password inserted by the user, created with the bcrypt algorithm.

authentication_token contains a unique string with a length of 128 characters.

Every page, that should only be accessible with login, calls the following script named login_check.php, that is placed at the very top of each page:

// get login data from database
if(isset($_COOKIE['user'])) {
$stmt = $pdo->prepare('SELECT authentication_token FROM users WHERE user = :user;');
$stmt->execute(array(':user' => $_COOKIE['user']));
$results = $stmt->fetchAll();
foreach($results as $row) {
    $authentication_token = $row['authentication_token'];
}
}

// show login form if no data or wrong data
if(empty($_COOKIE['user']) || empty($_COOKIE['authentication_token']) || $_COOKIE['authentication_token'] != $authentication_token) {
    include "login.php";
    exit;
}

This will check if the user and authentication_token stored as cookies match to the data in the users table.

If yes, the script does nothing else and the site gets presented.

If not, the login form page gets included and all further code on this page gets aborted with the exit; command.

The login form page, called login.php, that gets included in case user and authentication_token stored as cookies don't match to the database, is the following:

// get login data from database
if(isset($_POST['user'])) {
$stmt = $pdo->prepare('SELECT password, authentication_token FROM users WHERE user = :user LIMIT 1;');
$stmt->execute(array(':user' => isset($_POST['user']));
$results = $stmt->fetchAll();
foreach($results as $row) {
    $password = $row['password'];
    $authentication_token = $row['authentication_token'];
}
}

// show login form if no data or wrong data
if(empty($_COOKIE['authentication_token']) || $_COOKIE['authentication_token'] != $authentication_token) {
echo "
<form action=\"\" method=\"post\">
<input type=\"text\" name=\"user\">
<input type=\"password\" name=\"password\">
</form>
";
}

// login
if(isset($_POST['user'], $_POST['password']) && password_verify($_POST['password'], $password)) {
    setcookie("user", $_POST['user'], time()+(10*365*24*60*60), "/");
    setcookie("authentication_token", $authentication_token, time()+(10*365*24*60*60), "/");
    unset($_POST);
    header('location: https://'.$_SERVER['HTTP_HOST'].$_SERVER['REQUEST_URI']);
    exit;
}

This code will first show the login form. As soon, as user and password, that get submitted with the form, match to the data in the database, user and authentication_token get stored as cookies and the page will get reloaded.

This will again first include the login_check.php. This will check again if user and authentication_token, that are stored as cookies, match to the database. If yes, what is the case now, the page won't get blocked with exit; any more and is accessible.

Is the described scenario recommended? Is this a good way to do that?

Is there anything I made wrong or I missed?

I also have the following script in the login_check.php file, that handles the logout process if the user presses on the logout button:

if($_POST['logout']) {
    unset($_COOKIE['user']);
    unset($_COOKIE['authentication_token']);
    setcookie('user', '', time() - 3600, "/");
    setcookie('authentication_token', '', time() - 3600, "/");
    unset($_POST);
    header('location: https://'.$_SERVER['HTTP_HOST'].$_SERVER['REQUEST_URI']);
    exit;
}

But I don't really rate the logout function as security feature, because users may not use the logout button.

I think my code is pretty safe against hacking (assumed that the passwords are safe enough). My code is lightweight and clear. I'm using secure hashes. No password gets stored on the clients side. I'm only using prepared statements. The authentication_token is unique.

The only thing I'm concerning about is the following: What if the user doesn't use his own device and doesn't log out? Since the authentication_token is always the same, anybody else could visit the pages without the need to have the login data, since it's already logged in and the login doesn't expire.

How do you handle this case typically? Change the authentication_token stored in the database frequently, to force the user to login again from time to time? Change the authentication_token at every new login on another device? Set a cookie expiration time of few hours?

Answer 1

This is not a code review site so don't expect a full code review here. But a few things stand out:

• The comparison of given token to expected token uses !=. This checks for non-equality instead of non-identity (i.e. !==) and might lead to authentication bypass using type confusion. See PHP type confusion on password comparison and the explicit warning in the documentation for more information.
• There does not seem to be any protection against credential brute forcing and it is unclear if it is possible to use easy to brute-force passwords (trivial passwords, known from public dumps, simple variations) as a user.
• The cookie for the authentication token is not httponly, which means that it can be accessed on the client side with JavaScript and thus might be stolen with a XSS attack.
• There does not seem to be any server side enforced expiration of the authentication token.
• The token does not gets deleted from the server after explicit logout, so it is still possible for an attacker to use a stolen token even after the user logged out.

Answer 2

I'll be going section by section through your code. I hope this isn't too much and that it pushes you forward, I went through it bit by bit :)

1. User activation

1. Bcrypt with a cost of 10 (the default) is okay, but should be pushed to 12 when possible (almost all hardware in our current age handles it).
2. Set a limit for the password to 64 as truncation happens around the 72nd character.
3. No password policy is implemented.
4. 32 bytes are enough randomness. I'd recommend using base64 instead of hex.
5. Unset the password once the hash is stored in the database.

2. Login check file

1. Check the authentication cookie instead of the user's, that's your session identifier.
2. Grab the user (roles, etc.) from the session, not the other way around.

3. Login file

1. Session hijacking and Session Fixation! You need to generate a token for every login that happens, not re-grab the token on activation. That should be removed after logging out the first time.
2. Timing attacks. This results in user enumeration. You're using the user to grab the password from the DB, if the user data is rejected you are showing the login form. They should be in the same condition. The user and the password should be checked in the same condition, then if both fail, return the login form.
3. The user cookie doesn't help, at all. Remove it.
4. The authentication token is set for 10 years! That's whole lot of time :) Shorten it to a shorter duration, something that fits your use case.
5. Check that the token is still valid, otherwise the time set is useless.
6. Set security attributes to your cookies. You can have a __Host- prefix, a Secure, HTTPOnly and SameSite attributes to properly lock it down, as it should be. Read more here on WSTG - Testing for cookies attributes
7. $_SERVER['HTTP_HOST'] is vulnerable to Host Header Injection! Don't use it. Set the domain this is deployed on in a global variable manually and use it.

4. Logout Script

1. Destroy the token from the database.
2. Fix the Host issue described in the Login File.

5. General tips

1. For critical checks (if possible, always use it), always use the strict === comparison versus the == loose comparison. A lot of abuse happened around type juggling.
2. Captcha and request limiting are essential to protect your users that use the service.

Some references to help you with this:

• Password Storage CheatSheet
• Authentication CheatSheet

Answer 3

You use 2 cookies "user" and "authentication_token". This makes code more complexe without any need. Actually cookie "user" is redundant. I'd suggest not to use it at all.

Better approach would be following:

• If user credentials (user name, password) present, validate them. If valid, proceed to the next start page. If not valid, forward to the login page.
• Check if cookie "authentication_token" is present. If not present, forward to the login page.
• Check if the "authentication_token" is not outdated. If outdated, forward to the login page.
• Check if there is a user account for "authentication_token". If user account not found, forward to the login page.
• If user found, but is locked or outdated, display corresponding message.
• Otherwise proceed to the main part of the target page.