3
\$\begingroup\$

I am working on a project where users have to log in then if it is correct it creates a cookie to process requests. I would like to find a way to auto-renew sessions every day without affecting the user or requiring the user to log in again.

I would like to also know if I overlooked any security flaws such as SQL injection or session-hijacking and all those, are there any other one that I could prevent or improve?

The login page is

<?php
//Login.php
if(isset($_POST['username']) && isset($_POST['password'])){
    $username = filter_input(INPUT_POST, "username", FILTER_SANITIZE_ENCODED, FILTER_FLAG_STRIP_HIGH); //Changes to unicode characters to prevent injection
    $password = $_POST['password']; //Get password

    $result = LoginManager::Login($username, $password); //Run login method

    if($result == "invalid_username"){
        echo 'Invalid Username!';
    }

    if($result == "invalid_login"){
        echo 'Invalid Login!';
    }

    $expires = gmdate('Y-m-d', strtotime($Date. ' + 1 days')); //sessions expire in 1 day
    DataBase::query("INSERT INTO sessions VALUES(:session, :userid, :expires)", array(':session' => $result, ':userid' => $userid, ':expires' => $expires)); //insert into database

    setcookie("session", $result); //set as cookie
    return;
}

echo '<form action="Login.php" method="post">
        <p>Username: <input type="text" name="username" placeholder="Username"/></p>
        <p>Password: <input type="password" name="password" placeholder="Password"/></p>
        <p><input type="submit" name="login" value="Login"/></p>
    </form>';
?>

The LoginManager class

<?php
//LoginManager.php
public static function Login($username, $password){
    if(strlen($username) > 16 || strlen($password) > 50 || strlen($password) < 5){ //usernames less than 16, password from 5-50 char
        return 'invalid_bounds';
    }

    $sql = "SELECT * from accounts WHERE username = :username";
    $users = Database::query($sql, array(':username' => $username)); //get account

    if(sizeof($users) == 0){ //no accounts found
        return 'invalid_username';
    }

    $userdata = $users[0];

    $salt = '-45dfeHK/__yu349@-/klF21-1_\/4JkUP/4'; //salt for password
    if(password_verify($password . $salt, $userdata['password'])){ //check if password is correct
        $session = com_create_guid(); //create unique session
        return $session;
    }else{
        return 'invalid_login';
    }
}

?>

And the main page / feed

<?php
//Main.php
$session = $_COOKIE['session']; //get cookie session

$sessions = DataBase::query('SELECT * FROM sessions WHERE id=:id', array(':id' => $session));

if(sizeof($sessions) == 0){
    //Redirect to login page
    return;
}

//get first session
$session = $sessions[0];

$date = new DateTime($session['expires']);
$now = new DateTime();

if($date < $now) {
    //Expired session : Redirect to login
    return;
}

//Display feed

?>
\$\endgroup\$
3
  • \$\begingroup\$ I'm no expert on this but I think people usually employ prepared statements to avoid SQL injections. \$\endgroup\$ Commented Apr 30, 2018 at 8:33
  • \$\begingroup\$ @yuri so he does \$\endgroup\$ Commented Apr 30, 2018 at 9:01
  • \$\begingroup\$ Where does $userid come from? \$\endgroup\$ Commented Apr 30, 2018 at 9:06

2 Answers 2

Reset to default
3
\$\begingroup\$

Magic variables

You are using invalid_username, invalid_login and some more magic names that are directly related to the login procedure. Factor those out and put them as static properties on your LoginManager class, so the actual contents are irrelevant and you can simply check against LoginManager::INVALID_LOGIN to see if that is the state of the session.

Factor out your session management

Creation and renewing of sessions should happen in their own class. Other code doesn't need to know how to make sessions, just that a session exists and who is authenticated by that session.

Factor out the salt

Move the salt to a config file somewhere. You don't want to have to dig through your code when you have to change the salt. If you do not want to have a config file, at least put it as a static or private variable in your LoginManager.

Odd sessions

You are making a session even if the login was invalid. Later you only check if a session exists, not if that session was valid. This allows someone to log in by entering a wrong username, or a right username with a wrong password.

Your LoginManager should also not disqualify usernames or passwords based on length, as this is a thing that should be enforced when setting a password or username. The login will always fail in those cases, so don't worry about it. You want to do the same amount of work for every login attempt to prevent an attacker gathering information about valid usernames/passwords based on the time it takes for a request to return.

Timezones

You are creating a date with gmdate(..) which is a UTC date. Later you are parsing it with new DateTime(..) as a local date and comparing it with a local date. This causes sessions to last longer or shorter based on where the server is.

Undefined variables

You are making sessions where the $userid is undefined. This means you cannot do anything useful with the session, as you lose information about who is logged in.

You are also setting the expiration date with $Date, a misspelled variable that is not defined either.

Queries can return false

You should always keep in mind that queries may fail. If your username query fails, it will return false, and sizeof(false) === 1. Your code would then error out on $userdata['password'], which is not that great.

Expired sessions

You are currently not doing anything with your expired sessions. That means they are not removed from the database after they expired.

Various

You specifically asked about sql injection and session hijacking. I believe your code to be reasonably safe on both accounts. If you are using truely prepared statements (not a setting that uses sprintf behind the scenes), you should be safe from sql injection. Your queries may still fail, but you should not be able to execute unintended sql queries.

Your sessions are based around com_create_guid, which I believe is cryptographically secure. The generated string is also long enough that guessing a session guid is practically impossible. You are making sessions less secure by essentially wanting a session to last forever, increasing the time they could be used by some malicious third party. I do not believe this to be a great risk in this case though.

\$\endgroup\$
2
\$\begingroup\$

One thing to note with your SQLstatements, you are retrieving more fields than you need to. This results in more memory usage and slower performance. While it may not make a huge difference for you, it's still best practice to only select the fields you need.

Change

'SELECT * FROM sessions WHERE id=:id'

to 

'SELECT expires FROM sessions WHERE id=:id'

and

$sql = "SELECT * from accounts WHERE username = :username";

to 

$sql = "SELECT password from accounts WHERE username = :username";

You will also want to make sure that the username field is unique, as otherwise you would get back the first password for that username but not necessarily the correct one.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.