PHP login system with prevention for session hijacking, fixation, injection, etc

Question

I'm creating a login system and I have been reading a lot about the security measures needed to prevent session hijacking, fixation, and injection attacks, etc. These two Stack Overflow posts were particularly helpful: Link Link 2

Below is my login system so far. Have I sufficiently protected myself against attacks?

  <?php 

  // convert password to hash
  function passwordHash($string) {
    return password_hash($string, PASSWORD_DEFAULT);
  }

  // compare password to hash
  function passwordVerify($string, $hash) {
    return password_verify($string, $hash);
  }


  // authenticate login password
  function login($submitted_password, $password, $username) {
    global $link;

    if(passwordVerify($submitted_password, $password)) {
      ini_set('session.use_trans_sid', FALSE);
      ini_set('session.entropy_file', '/dev/urandom');
      ini_set('session.hash_function', 'whirlpool');
      ini_set('session.use_only_cookies', TRUE);
      ini_set('session.cookie_httponly', TRUE);
      ini_set('session.cookie_lifetime', 1200);
      ini_set('session.cookie_secure', TRUE);
      session_start();

      $link->query("SELECT id, password, username, user_level FROM users WHERE username = :username");
      $link->bind(':username', $username);
      $link->execute();
      $row = $link->getOneRow();
      $link->closeStream();

      $id = $row['id'];
      $username = $row['username'];
      $user_level = $row['user_level'];

      $_SESSION['userID'] = $id;
      $_SESSION['username'] = $username;
      $_SESSION['user_level'] = $user_level;
      $_SESSION['user_ip'] = $_SERVER['REMOTE_ADDR'];
      $_SESSION['HTTP_USER_AGENT'] = $_SERVER['HTTP_USER_AGENT'];

      // store in db to use in page_protect()
      $user_ip = $_SERVER['REMOTE_ADDR'];
      $useragent_hash = passwordHash($_SESSION['HTTP_USER_AGENT']);

      $link->query("UPDATE users SET user_ip = :user_ip, useragent_hash = :useragent_hash WHERE id = :id");
      $link->bind(':user_ip', $user_ip);
      $link->bind(':useragent_hash', $useragent_hash);
      $link->bind(':id', $id);
      $link->execute();
      $link->closeStream();
      header("Location: dashboard.php");
      exit();   
    } else {
      $error = "Username or password error"; // password fails
    }
  }


  // function called by every page requiring you to be logged in
  function page_protect() {
    global $link;

    if (session_status() == PHP_SESSION_NONE) {
      session_start();
    }

    if(!isset($_SESSION['user_ip']) || $_SESSION['user_ip'] != $_SERVER['REMOTE_ADDR']) {
      logout();
      exit();
    }

    // referenced in question #5 below
    if (!isset($_SESSION['HTTP_USER_AGENT']) || $_SERVER['HTTP_USER_AGENT'] != $_SESSION['HTTP_USER_AGENT']) {
      logout();
      exit();
    }        

    // check that IP address/useragent hash stored in db at login match current session variables
    $link->query("SELECT useragent_hash, user_ip FROM users WHERE username = :username");
    $link->bind(':username', $_SESSION['username']);
    $link->execute();
    $row = $link->getOneRow();
    $link->closeStream();

    $user_ip = $row['user_ip'];
    $useragent_hash = $row['useragent_hash'];

    if($_SESSION['user_ip'] != $user_ip || !passwordVerify($_SESSION['HTTP_USER_AGENT'], $useragent_hash)) {
      logout();
      exit();
    }

    session_regenerate_id(true);
  }


  function logout() {
    if (session_status() == PHP_SESSION_NONE) {
      session_start();
    }

    unset($_SESSION);
    session_unset();
    session_destroy();
    header("Location: index.php");
    exit();
  }

  ?>

1. Am I missing anything or does anything look wrong?

2. I'm not sure I'm using session_regenerate_id() in a good spot. I know it needs to be called periodically, so I figured every time a protected page is called would be good.

3. All of the ini_set's only appear before the session_start() in the login function. However, the top of each page protected with page_protect() has a session_start(). Should it be preceded by all of the same ini_set's on every page, or once they're set during the initial login, they stay set?

4. Should I remove this line? ini_set('session.cookie_lifetime', 1200); That would require the user to login again after 20 minutes. I think it would be good to log the user out after 20 minutes of inactivity, but not after 20 minutes of moving around the site.

5. In page_protect, when I check the IP and user agent hash, should I use && instead of ||? If both conditions are met, something is definitely amiss.

Answer 1

First of all, you are heavily overthinking it. And, as a result, over-engineer the code, making it mostly overkill. A theory is a good thing, in reality it is always a trade-off between security and user experience. For example, on most sites, including Stack Overflow, a user is "remembered", which is essentially like an endless session which doesn't care for the changed IP address.

Besides, you must take any information that is older than 2-3 years with a pinch of salt. For example, when these 2 answers have been written, HTTPS weren't a commonplace thing. But now it is, making most of these measures obsoleted. Yet at the same time HTTPS is way much more important than most of these tricks.

Besides, I just don't understand some measures you are taking. Why you are hashing a user agent? Or why you're checking a user agent and an ip address both against a session and a database? Isn't just a session enough? Or that decision to set the cookie lifetime. As far as I remember, the cookie would renewed at each request, so it shouldn't be an issue, but really. Did you ever try to work with a site with such a restriction? Even my bank logs me out after the inactivity timeout, not every 20 minutes. And your site is not a bank nor anything of similar level of security.

Some levels of verbosity I just don't understand.

  $id = $row['id'];
  $_SESSION['userID'] = $id;

can't we have it already as $_SESSION['userID'] = $row['id'];?

So in the end I would

• first and foremost make your site https
• use session_set_cookie_params() instead of that set of ini_set before each session_start(), because PHP's execution is atomic, each new instance knows nothing of the settings made in the other instance
• make page_protect() just check the user_id in the session. it should be enough.

There are other issues in this code

Your database wrapper is stateful. Means the first query you will run in a loop will give you unexpected results.

Your wrapper should be either spit in two classes, as PDO itself does, or offer methods that give you the desired result in one run, without storing any state. For example,

  $sql = "SELECT id, password, username, user_level FROM users WHERE username = ?";
  $row = $link->getOneRow($sql, [$username]);

And you may note that the bind function is overkill as well. PDO can accept query parameters as an array, which i way more convenient. I even wrote an article about usual misconceptions in database wrappers linked above, you may find it useful.

Your database wrapper is accessed via global keyword which is unanimously frowned upon. At least make it a singleton, which is also despised but at least cannot be written over.

Answer 2

A few things I noticed,

First of all this is plain wrong (if it's pure PDO):

$link->query("SELECT useragent_hash, user_ip FROM users WHERE username = :username");
$link->bind(':username', $_SESSION['username']);
$link->execute();

PDO::query — Executes an SQL statement, returning a result set as a PDOStatement object

http://php.net/manual/en/pdo.query.php

This should be PDO::prepare like this:

$stmt = $link->prepare("SELECT useragent_hash, user_ip FROM users WHERE username = :username");
$stmt->bindParam(':username', $_SESSION['username']);
$stmt->execute();

Also note you have to bind and execute against the PDOStatment object and not the PDO object itself.

Function arguments, and code responsibility

Then even your function arguments:

function login($submitted_password, $password, $username) {

You most likely wont know $password at this point, nor should you know it. If you redo your function like this:

//note $submitted_password was renamed as $password
function login($password, $username) {
    global $link;

    $stmt = $link->prepare("SELECT id, password, username, user_level FROM users WHERE username = :username");
    //string arguments can be passed as an array directly to execute
    $stmt->execute([':username'=>$username]);

    if(false !== ($row = $stmt->fetch(PDO::FETCH_ASSOC))){
        if(passwordVerify($password, $row['password'])) {
              ///...rest of code here
        } else {
            $error = "Password error"; // password fails
        }
    }else{
        $error = "Username error"; // username fails
    }
}

/*
  you don't have to do separate errors for Username & Password,
  in fact there is a good argument to keep these errors the same 
  from the end users perspective.
*/

This way you only need the submitted username, and password. Which are both easily available from a login form. Another thing to mention, is that after the password check you don't verify the row data. It's probably unnecessary given the larger context. For example if you had to previously (before calling login) pull the user data to get the encrypted password. Then you know that the user must be valid. However, when looking at the code as a single unit, it should be verified right when it's pulled from the DB. Because, if that larger context changes then there is no check being done.

Instead you can consolidate it, by just letting the login function take care of pulling the stored password at the same time it's checking the username (by querying for it).

What I mean by code responsibility is some other code must be getting the password from the DB, otherwise how would you know the stored password. This code whatever it is, really shouldn't be responsible for that. On top of that every time you call login, you will have to pull that password before hand. So it's better to wrap those operations into login function. It just makes more sense to do it that way and have the arguments both be end user submitted data, instead of a mix of stored data and user data.

Hope that makes sense.