Simple login script with sha256 hashing

Question

I have been using this code for a while. I got it from a tutorial 5 years back. Now I'm wondering if it still is secure, or if it's time to find a new one? Maybe it has never been secure enough.

LOGIN

<?php 
//Connect to database
require("include/config.php");

//Empty Variable
$submitted_username = '';

//Check if something is posted
if(!empty($_POST)){ 

    //Check if user exists
    $query = "SELECT * FROM usrs WHERE usr_email = :usr_email";
    $query_params = array(':usr_email' => $_POST['usr_email']); 
    try{ 
        $stmt = $db->prepare($query); 
        $result = $stmt->execute($query_params); 
    }
    catch(PDOException $ex){ die("Failed to run query: " . $ex->getMessage()); } 

    //Fetch result
    $row = $stmt->fetch(); 

    //Reset var
    $login_ok = false;

    //If usr exists, check password
    if($row){
        //Check if password matches
        $check_password = hash('sha256', $_POST['usr_password'] . $row['usr_salt']); 
        for($round = 0; $round < 65536; $round++){
            $check_password = hash('sha256', $check_password . $row['usr_salt']);
        }
        if($check_password === $row['usr_password']){
            $login_ok = true;
        } 
    }

//LOGIN OK
    if($login_ok){ 
        //UNSET VARS
        unset($row['usr_salt']); 
        unset($row['usr_password']);

        //LOG USR LOGIN DATA
        $dateTime = date('Y-m-d H:i:s');
        //Create array
        $usr_data = array(
            ':usr_id' => $row['id'],
            ':usr_name' => $row['usr_fname']. " " . $row['usr_lname'],
            ':dateTime' => $dateTime,
            ':ip' => $_SERVER['REMOTE_ADDR']
        );


        //CREATE SESSION 
        $_SESSION['usr'] = $row;

        //REDIRECT TO STARTPAGE
        header("Location: start.php");

    } 
    else{ 
        echo "<script type='text/javascript'>alert('Fel uppgifter..');</script>"; 
        $submitted_username = htmlentities($_POST['usr_email'], ENT_QUOTES, 'UTF-8'); 
    } 
} 
?>

CONFIG

$options = array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'); 
try { $db = new PDO("mysql:host={$host};dbname={$dbname};charset=utf8", $username, $password, $options); } 
catch(PDOException $ex){ die("Failed to connect to the database: " . $ex->getMessage());} 
$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); 
$db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC); 

//SET HEADER
header('Content-Type: text/html; charset=utf-8'); 

//START SESSION
if(!isset($_SESSION)){ 
    session_start([
        'cookie_lifetime' => 86400,
    ]);
}

//SET TIMEZONE
date_default_timezone_set('Europe/Stockholm'); 
?>

CHECK IF LOGGED IN

<?php

//CHECK IF USR IS LOGGED IN
if(empty($_SESSION['user'])){
    echo "<script>window.location = '/index.php'</script>";
    die("Redirecting to index.php"); 
}

Answer 1

There's a timing attack. To see whether a user has registered or not, I can try to login. If it fails fast, the user is not registered. If it takes time, the user is registered. To fix this information disclosure, calculate some dummy hash even if the user cannot be found in the database.

The SQL part is fine.

The $submitted_username calls htmlentities too early. The only correct time to call htmlentities is exactly at the point where you embed a text into an HTML snippet. That doesn't happen in your code, though. To fix this, rename the variable from submitted_username to submitted_username_html, to prevent it from being used in any other context. For example, code that reads db_insert($submitted_username_html) looks wrong enough to warrant a thorough code review.

Your code could benefit from being split into a few well-named functions. After doing that, the main code might read like this:

$logged_in_user = log_in($_POST);
if ($logged_in_user === FALSE) {
    // …
} else {
    // …
}

When you extract all the detailed code into a function, you can use early returns for all the error cases. And when you step through the code using a debugger, you can easily skip over all the details of the login process, if you are not interested in it.

In the config part, you should prefix all variable names with db_, to avoid confusing the username with the submitted_username. Or group them into an object called db_config. Then you can access it as $db_config['username']. There, the identifier username is appropriate since it is qualified by the word db_config, which makes it unambiguous.

The SHA-256 algorithm you use for hashing is not secure enough anymore since specialized hardware can compute it too fast, even with 65536 iterations. PHP has a built-in set of password hashing algorithms. Just use that instead of iterating on your own. Migrating from your custom password hashing to the PHP default will cost a bit of time and work, but it's worth the effort. And since you are not the first to do that, there's already plenty of documentation on it. Probably. Hopefully.

Answer 2

I can see two issues related to security (but both do not pose an immediate threat being rather a potential issue):

• the insufficient hashing algorithm. Nowadays computers are FAST. Means they could calculate billions ordinary hashes per second. And, in case your database gets compromised, it would be rather easy to get raw passwords from it. A secure password hash must be slow. And PHP has one, implemented in the password_hash function. So instead of your own hash you must use a built-in function
• I bet you never paid much attention to the error reporting part of your code, as your application seldom throws an error. Yet, when it happens, it's a complete disaster. An error message could contain a lot of sensitive information about your system. Not likely that it could be used directly to hack into your site, but it can help a hacker a lot. Besides, it just makes no sense to send an error message right away - a site user wouldn't make any sense of it. You chould forget about die() in your scripts. The best thing you can do is to leave the error message alone.

The only other note I can make, there is a lot of rather useless conditions or the unused code. I rewrote your code based on the review above:

<?php 
//Connect to database
require("include/config.php");

//Check if something is posted
if($_POST){ 

    //Check if user exists
    $query = "SELECT * FROM usrs WHERE usr_email = :usr_email";
    $stmt = $db->prepare($query); 
    $stmt->execute([':usr_email' => $_POST['usr_email']]); 
    //Fetch result
    $row = $stmt->fetch(); 

    //If usr exists, check password
    if($row && password_verify($_POST['usr_password'], $row['usr_password'])
    {
        unset($row['usr_password']);

        //CREATE SESSION 
        $_SESSION['usr'] = $row;

        //REDIRECT TO STARTPAGE
        header("Location: start.php");
        exit();
    } else { 
        echo "<script type='text/javascript'>alert('Fel uppgifter..');</script>"; 
    } 
} 

Answer 3

I object that all the required post fields should be checked for presence and expected type and report the missing fields to the client as opposed to letting it convert to php errors.

As a simple solution we might report to client using js alert as it is done in another case already.

<?php 
//Connect to database
require("include/config.php");

//Check if something is posted
if(isset($_POST['usr_password'], $_POST['usr_email'])
   && \is_string($_POST['usr_password'])
   && \is_string($_POST['usr_email'])
   && \strlen($_POST['usr_password']) < 1000){ 

    //Check if user exists
    $query = "SELECT * FROM usrs WHERE usr_email = :usr_email";
    $stmt = $db->prepare($query); 
    $stmt->execute([':usr_email' => $_POST['usr_email']]); 
    //Fetch result
    $row = $stmt->fetch(); 

    //If usr exists, check password
    if($row && password_verify($_POST['usr_password'], $row['usr_password'])
    {
        unset($row['usr_password']);

        //CREATE SESSION 
        $_SESSION['usr'] = $row;

        //REDIRECT TO STARTPAGE
        header("Location: start.php");
        exit();
    } else { 
        echo "<script type='text/javascript'>alert('Fel uppgifter..');</script>"; 
    } 
} else if (!empty($_POST)) {
  echo "<script type='text/javascript'>alert('usr_email and usr_password are required and password must be less than 1000 chars');</script>"; 
}

It could be a different message for each case, but for illustration I believe this should be enough.

Further I have added check for password length to prevent hashing very long strings.

Answer 4

Unused variable, single-use variable

In this section:

//LOG USR LOGIN DATA

$dateTime = date('Y-m-d H:i:s');
//Create array
$usr_data = array(
    ':usr_id' => $row['id'],
    ':usr_name' => $row['usr_fname']. " " . $row['usr_lname'],
    ':dateTime' => $dateTime,
    ':ip' => $_SERVER['REMOTE_ADDR']
);

The variable $dateTime is only used once, so there is not much need to store the value in a variable. The value can be used when creating $usr_data without storing it in the variable. Also, $usr_data doesn't appear to be used in this code.

Redirect with header() instead of Javascript

The last block of code uses a JavaScript redirect:

//CHECK IF USR IS LOGGED IN

if(empty($_SESSION['user'])){
    echo "<script>window.location = '/index.php'</script>";
    die("Redirecting to index.php"); 
}

It would be simpler just to redirect using header('Location: /index.php') as the other code does, unless the user was supposed to see the redirecting message for a set amount of time (e.g. 2 seconds), however it is possible the user might have JavaScript disabled and thus the redirect would not happen.