4
\$\begingroup\$

I tried to search and read for 10-12 hours on how to have a secure session, and this is the (simplified version of the) code that I came up with. (found no good book or article with complete guide about PHP Sessions, each had something and missed something else)

Could you kindly please check if I have taken the necessary steps or if I've made a mistake somewhere?

Additional Info: PHP 7.3.x will be used, Webserver is nginx on Ubuntu 18.04, will have a maximum of ~500 visitors per second, usually ~50-100, sessions are written on disk (default of PHP)

function secure_session_start($domain, $ip, $useragent)
{
    // Change PHPSESSID for better security, remove this if set in php.ini
    session_name('app_session');

    // Secure session_set_cookie_params
    session_set_cookie_params(0, '/', $domain, true, true);

    // Don't show any output if session_start fails, die immediately (will add log_error later)
    @session_start() or die();

    // Keep session alive (as suggested by comment on https://www.php.net/manual/en/function.session-start.php#121209)
    $_SESSION['time'] = time();

    // Hash Useragent to be safe from storing malicious useragent text as session data on server
    $useragent_hash = hash('sha256', $useragent);

    // Make sure we have a canary set
    if ( ! isset($_SESSION['canary']))
    {
        // Regenerate & delete old session as well
        session_regenerate_id(true);

        $_SESSION['canary'] = [
            'birth' => time(),
            'ip' => $ip,
            'useragent_hash' => $useragent_hash
        ];
    }

    // Regenerate session ID every 5 minutes:
    if ($_SESSION['canary']['birth'] < time() - 300)
    {
        // Regenerate & delete old session as well
        session_regenerate_id(true);
        $_SESSION['canary']['birth'] = time();
    }

    // If user is logged in, log out user if IP or Useragent is changed (this is intentional, I know users behind load-balancers etc will have issues)
    if (isset($_SESSION['username']) && ($_SESSION['canary']['ip'] !== $ip OR $_SESSION['canary']['useragent_hash'] !== $useragent_hash))
    {
        // Destroy cookie
        setcookie (session_name(), "", time() - 3600, '/', $domain, true, true);

        // Destroy session
        session_unset();
        session_destroy();

        // Redirect (avoid loop by checcking ip_browser_changed)
        if( ! isset($_GET['ip_browser_changed']))
        {
            header('Location: '.URL.'login/?ip_browser_changed');
            exit('IP Address or Browser has been changed, please login again!');
        }
    }
}

Then I will simply do secure_session_start() in my code after validating user IP Address with filter_var($ip, FILTER_VALIDATE_IP)

Edit: I've always relied on PHP Frameworks and their Auth library to handle the sessions etc for me and never worried about them, this time because of too many visitors, I had to skip using frameworks.

I have to make sure users are kept logged in with secure sessions as long as their Browser or IP hasn't changed. Now I'm trying to learn how to securely add the Remember Me feature, just needed to make sure I'm on the right track (with your help)

\$\endgroup\$
  • \$\begingroup\$ Wow, 500 v/s is a lot. I don't think your 'keep session alive' action makes much sense. You never use $_SESSION['time'] anywhere. You would normally keep a session alive (after browser closes) by setting the first parameter of session_set_cookie_params() to something other than zero. I would also use a cookie token, instead of a canary, but that's another matter. You haven't told us what it is you want to do with this code, can you add that? What are your goals? \$\endgroup\$ – KIKO Software Apr 24 at 14:14
  • \$\begingroup\$ @KIKOSoftware 500 is the peak, it will be around 50-100 most of the time. about the $_SESSION['time'] I read it here: php.net/manual/en/function.session-start.php#121209 , so the comment is wrong? \$\endgroup\$ – J. Doe Apr 24 at 14:19
  • \$\begingroup\$ OK, I don't know is that comment is right or wrong. It's just an unlucky choice to put a time in the session and to say that that will keep it alive. Better to use something like $_SESSION['dummy'] = random_bytes(25);. \$\endgroup\$ – KIKO Software Apr 24 at 17:21
  • 1
    \$\begingroup\$ I think How to Create Bulletproof Sessions will give you a better idea of how to write a session manager. I strongly advice you not to mix session handling and user authetication in one single function. \$\endgroup\$ – KIKO Software Apr 25 at 12:38
2
\$\begingroup\$
  • As an overarching rule, I never write scripts with the "stfu operator" (@). It looks like you plan to refine the session starting line, so I won't dwell.

  • You are calling time() 5 separate times in your function. Because there is no benefit in recording their differences in terms of microseconds, I recommend that you call time() once, and cache the value in a variable/constant to be used in all processes in the custom function.

  • I tend to never use OR or AND in my php conditions (only in my SQL) as a matter of consistency. This also prevents unintended hiccups regarding precedence. Separately, I never use or die() in my scripts ...trying not to dwell.

  • Because you are generating a hash for non-cryptographic use, you may enjoy the advice/discussions about performance comparisons between different hash generators: Fastest hash for non-cryptographic uses?

\$\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.