2
\$\begingroup\$

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.

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

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.

\$\endgroup\$
  • \$\begingroup\$ I appreciate the detailed response. I'm going through my code page by page updating all the database queries now to start off. I read a lot on phpdelusions.net and I'm basically structuring my queries the way it suggests there now. When I finish updating my code, should I edit the above post or start a new one? \$\endgroup\$ – John Dec 30 '18 at 16:55
  • \$\begingroup\$ The rules for this site say that it should be a new one. Besides, I would suggest to post your question regarding session security on security.stackexchange.com which is specifically dedicated t security \$\endgroup\$ – Your Common Sense Dec 30 '18 at 16:59
  • \$\begingroup\$ Is there a way to create those same ini_set properties through session_set_cookie_params()? I'm having trouble on the first one, session.use_trans_sid, for example. When I search for "use_trans_sid" the only results that come up are related to setting it through ini_set(). \$\endgroup\$ – John Dec 30 '18 at 18:26
  • \$\begingroup\$ session.use_trans_sid's default value is 0. I wonder why would you bother to set it to 0 again, but no, it's impossible through session_set_cookie_params \$\endgroup\$ – Your Common Sense Dec 30 '18 at 18:28
  • \$\begingroup\$ Then how would I set the same set_ini parameters, i.e.the entropy file or hash function, using session_set_cookie_params()? \$\endgroup\$ – John Dec 30 '18 at 18:35
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ There is no PDOStatement:bind() method :) And no, it's not pure PDO \$\endgroup\$ – Your Common Sense Dec 30 '18 at 18:01
  • \$\begingroup\$ The bind method is in my PDO object: public function query($query) { $this->sql = $this->dbConnection->prepare($query); } public function bind($param, $value) { $this->sql->bindParam($param, $value); } However, as I said in my comment above, I'm currently redoing all of my database queries throughout the code to the way suggested on phpdelusions.net \$\endgroup\$ – John Dec 30 '18 at 18:03
  • \$\begingroup\$ Thats right its bindParam To be honest I almost never use that ... lol ... i sort of had a feeling it wasn't "pure" PDO. But what I said about the function args still hold. You souldn't pass the encrypted password to it. \$\endgroup\$ – ArtisticPhoenix Dec 30 '18 at 18:04
  • \$\begingroup\$ What you said about code responsibility makes sense. I'm also now currently redoing my login function per your suggestions. I'm glad I asked for help. Thanks a lot! \$\endgroup\$ – John Dec 30 '18 at 18:20
  • \$\begingroup\$ Sure, I always try to make functions/methods or even classes do just one thing and to it well. They should be a complete operation, taking as little setup to call as possible. They should handle as raw of data as possible and return as finished a product as possible. If that makes sense. What do they call that a black box, where the implementation details of the function don't matter to the outside world. \$\endgroup\$ – ArtisticPhoenix Dec 30 '18 at 18:22

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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