Login System Security Part 2

Old code:

Login system security

This is an update on the code and suggestions I received from generous users on the old thread.

So same rules apply, just check it out, tell me what you think. Is it better or worse? is the security getting better and if so what else can I do to make it better? Are the sessions correctly called/executed? Is the password check/storage correct?

script: ../includes/access.inc.php

prepare($sql);
    $s->bindValue(':user', $username);
    $s->execute();
}
catch (PDOException $e) {
    echo "Error" . $e->getMessage();
}

$count = $s->fetch();

if (!$count[0] > 0) {
    return FALSE; //failed
} 
if (!isPasswordGood($username, $passtemp)) {
    return FALSE; //failed
}    

return TRUE;
} //end function

function isPasswordGood($username, $passtemp)
{
include 'connect.php';  

//query hash password in database
try {
    $sql = "SELECT password FROM `users` WHERE username = :user";
    $s   = $pdo->prepare($sql);
    $s->bindValue(':user', $username);
    $s->execute();
}
catch (PDOException $e) {
    echo "Error" . $e->getMessage();
} 

$row  = $s->fetch(); //grab hash array
$hash = array_slice($row, 0, 3); //break up the array

if (password_verify($passtemp, $hash[0])) { //call password_verify() on hash
    return TRUE; //password is good
} else {
    return FALSE; //bad
}

} //end isPasswordGood()

//validate input
function test_input($data)
{
    $data = trim($data);
    $data = stripslashes($data);
    $data = htmlspecialchars($data, ENT_QUOTES, 'UTF-8');

    return $data;
}

//set session variables here
function setSessions($username, $passtemp)
{  
$_SESSION['loggedIn'] = TRUE;
$_SESSION['user']     = $username;    
return TRUE;
}

//unset session variables here
function unsetSessions()
{
unset($_SESSION['session_id']);
unset($_SESSION['loggedIn']);
unset($_SESSION['user']);
return FALSE;
}

//this function sets the session ID
function setSessionID($username).........etc......etc.......

script: reguser.html.php

```
//^

Notes:

• For the sake of reading it efficiently, I placed your code in my IDE to let it format the indentations.

Here's what looks good

-
Thorough comments. It helps!

-
password_verify($passtemp, $hash[0])! Yes! Beautiful!

-
Tiny other details and stuff that I probably just brushed over but really should be mentioned.

Here's what doesn't look good

-
Indent please.

-
Use of $GLOBALS. There's a time and place for it, but I see it as unnecessary here. Everything in this file is already in the global scope, and anything outside of it should already be able to tell if the user is an admin or not.

-
What is $_SESSION['mintime']? I thought it would come to me, but it hasn't. Is it the minimum time? Either document it better, or come up with a stronger name.

-
The first file is named access.inc.php. At a glance I can tell it does many things besides "accessing". Change the name or break apart your file and require them in a main calling file.

The next few bullet-points will focus on userIsLoggedIn()

-
Typically, function names are verbs, i.e getProperty(), authorizeUser(), logoutUser(). I suggest changing the name to something else.

-
The method is very busy. Lot's of ifs and business going on. Currently it's covering login, logout, authentication, and session handling. To help improve readability, reduce the size of functions.

-
I notice you have a lot of $_POST and $_SESSION values. Things have become hectic with all the checking and returns, ask yourself what you can do to consolidate some of these values.

-
Some lines just aren't making sense....?

Here's one:

if (!isset($_POST) and ( !isset($_POST['action']) or $_POST['action'] != 'login')

Let's break it down:

If nothing was sent to the page, and the action is either not there or is not 'login', return false.

If nothing was sent to the page, then of course action won't be set! Correct me if I've over thought this, but it's essentially a useless check.

How about this one:

if (isset($_SESSION['loggedIn']) and ! empty($_SESSION['user']))

If the user's session says they're logged in, and they have a username in their session, check their session

Basically that's what it's doing. Don't tell me that's not redundant! Moving back to consolidating $_POST and $_SESSION values: how does 'loggedIn' improve your application? It's clutter! Simply, if they have a username, check 'em. If they don't have a username, they're not logged in.

Next up:

if ($username == 'admin') {
    $GLOBALS["isAdmin"] = TRUE;
}

This is currently behind:

databaseContainsUser($username, $passtemp)

Before both of these is $GLOBALS['adminPage']. You didn't mention how this superglobal is set... What if for some reason it's 0. If someone tries to log in with username 'admin', then an attacker could brute-force send passwords to your page easily and possibly gain access to the admin page.

I've rewritten your function below. Mainly re-factoring, because it's hard to tell your intentions so I left something unchanged, because they're up to you to change!

function userIsLoggedIn() {
    $postUsername = $_POST['user'];
    $postPassword = $_POST['password'];
    if (!isset($_POST['action']) or empty($postUsername & $postPassword)) {
        return FALSE;
    }
    switch ($_POST['action']) {
        case 'logout':
            unsetSessions();
            break;
        case 'admin':
            header('Location: ' . $_POST['gotoadmin']);
            exit();
            break;
    }
    if (!empty($_SESSION['user'])) {
        return checkSessionID($_SESSION['user']);
    }
    array_push($_SESSION['mintime'], time());
    array_shift($_SESSION['mintime']);  
    if ($_SESSION['mintime'][1] - $_SESSION['mintime'][0] < 2) {
        return FALSE;
    }
    $username = test_input($postUsername);
    $passtemp = test_input($postPassword);
    if ($GLOBALS['adminPage'] == 1 and $username != 'admin') {
        return FALSE;
    }
    if ($username == 'admin') {
        $GLOBALS["isAdmin"] = TRUE;
    }
    if (!databaseContainsUser($username, $passtemp)) {
        unsetSessions();
        return FALSE;
    }
    if (!isset($_SESSION['session_id'])) {
        setSessions($username);
    } elseif (!checkSessionID($username)) {
        unsetSessions();
    }  
    updateSessionID($username);
    setSessionID($username);
    return TRUE;
}

So it's still pretty long, and since you know the in's and out's of your whole sessions and globals labyrinth, I encourage you to slim that down by at least 10 lines!

Note: empty($postUsername & $postPassword) is experimental. It should work, but I'm not 100% sure it's secure and fool-proof.

End of function!

-
includes are everywhere! Are we just including it, or are we requiring it to be there? You require it!

-
It's good that your catching errors, but echo "Error" . $e->getMessage(); Isn't gunna cut it. It's always a good habit to ensure a clean error. O

StackExchange Code Review Q#54907, answer score: 5