4
\$\begingroup\$

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"); 
}
\$\endgroup\$
11
  • \$\begingroup\$ Would you mind explaining why you call the hash function 65537 times? \$\endgroup\$ – slepic Dec 10 '19 at 11:44
  • 1
    \$\begingroup\$ @slepic The specific number has likely come from \$2^{16} = 65536\$. The purpose is to make the password hashing slower and therefore harder to brute force it. See Roland Illig's answer for an explanation why that might be a good, but not the best possible idea. \$\endgroup\$ – AlexV Dec 10 '19 at 12:07
  • \$\begingroup\$ @AlexV Hmm i see... But wouldnt it be better to limit the rate of requests on the "post login" endpoint to léts say 1 request per second per IP? Regular users probably never do two login attempts within a second And So they dont need to notice the slowdown. While the 1 in million visitors that Is a hacker still can't Brute Force it... \$\endgroup\$ – slepic Dec 10 '19 at 12:43
  • 1
    \$\begingroup\$ Anyway if I post a very long random password. The hash function May Crash your webserver. It Is wise to limit the length of the password before passing it to a hashing algorithm... \$\endgroup\$ – slepic Dec 10 '19 at 12:45
  • 1
    \$\begingroup\$ @slepic, to help you understand, the reason why those links from AlexV helps is, they cover PBKDF2 and other key-stretching algorithms, which are the current standard in password storage. Re-hashing the password 65536 times is a "poor man's PBKDF2" (this is a decent implementation: it puts the salt in the right place). The goal isn't to slow down how long it takes to log in, though that is a side effect. The goal is, if someone gets the key-stretched passwords, it will take them significantly longer to do an offline attack and there is no possibility of a rainbow table. \$\endgroup\$ – Ghedipunk Dec 10 '19 at 18:29
6
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Just wonder if the timing attack is significant enough to be detectable over network delays? I can see how it can be mitigated, but just like to understand the underlying need for these things. \$\endgroup\$ – Nigel Ren Dec 11 '19 at 8:03
  • \$\begingroup\$ Yes, it is. Assuming that a single hashing operation takes a microsecond, 65536 of them takes 65 milliseconds. And even if it we're just a tenth of that time, that's still easily detectable. Maybe you have to perform several measurements to get confident, but still yes. \$\endgroup\$ – Roland Illig Dec 11 '19 at 9:24
3
\$\begingroup\$

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>"; 
    } 
} 
\$\endgroup\$
16
  • \$\begingroup\$ The $_POST variable should be checked for presence of those keys before accessing them. Simple checking if post is empty or not is not enough (well, to be formally complete... Its probably enough for the script to work...). \$\endgroup\$ – slepic Dec 10 '19 at 15:53
  • \$\begingroup\$ @slepic that are you going to do in keys are undefined? \$\endgroup\$ – Your Common Sense Dec 10 '19 at 15:58
  • \$\begingroup\$ Redirect to hp maybe or back to login page. Idk. Its not my project... \$\endgroup\$ – slepic Dec 10 '19 at 16:00
  • \$\begingroup\$ OH And not just presence of keys. But also correct data types of the values under those keys (strings in OP's case). \$\endgroup\$ – slepic Dec 10 '19 at 16:04
  • \$\begingroup\$ @slepic everything sent via HTTP POST is a string. And in case the key is not set, there will be a PHP error. Given your app do care for PHP errors, there will be a "redirect" to the error page. Looks enough to me? \$\endgroup\$ – Your Common Sense Dec 10 '19 at 16:07
1
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ If you're going to check the plaintext password length, you might as well follow a standard while doing it (NIST 800-63B sec 5.1.1.2 says the minimum maximum should be 64 characters), or at least set the limit to match the constraints of your key stretching algorithm -- With password_hash() set at current defaults, this is 72 characters. Despite your claims in comments to the question, even a naive implementation of PBKDF2 is not going to DOS a site when given an overly large plaintext; only the first round will be affected... \$\endgroup\$ – Ghedipunk Dec 11 '19 at 17:32
  • \$\begingroup\$ ... The web server's other resources are going to pass failsafe thresholds (maximum upload size, maximum memory size, etc.) long before a key stretching algorithm can be used to make a CPU unresponsive for extended periods... and if you are attacking a key stretching algorithm using a DDOS attack, the site should (see the NIST standards, linked above) have other rate limiting factors that will keep the authentication system from being leveraged to magnify a DDOS. \$\endgroup\$ – Ghedipunk Dec 11 '19 at 17:35
  • \$\begingroup\$ (Relevant section in NIST 800-63B for rate limiting is section 5.2.2, by the way.) \$\endgroup\$ – Ghedipunk Dec 11 '19 at 17:40
  • \$\begingroup\$ Shouldn’t variable cleansing be done somewhere other than program flow? I see the reason for this if scripting procedurally; but oh my I would hate to have to sift through these if() statements because they are in effect validating input in addition to directing program flow. You can’t look at that four line if() and immediately grasp everything it’s doing; and if you have to make a change to the variables being submitted, your program flow is invalidated. (I don’t have a better solution to propose; still struggling with it myself) \$\endgroup\$ – Tim Morton Dec 21 '19 at 20:31
  • \$\begingroup\$ @TimMorton in real world you probably dont put all the conditions into one if. You have a separáte one for each of them And some specific message May be passed to client in each case. This all May be moved to a validátor class. I just tried to keep it simple And Focus on my main thought, that you should make sure that only expected things make it to places of your code where they wont crash your program or invoke any other unexpected behaviour. \$\endgroup\$ – slepic Dec 22 '19 at 6:50
0
\$\begingroup\$

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.


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() 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.