3
\$\begingroup\$

I've created a class that implements the native SessionHandlerInterface (with added PHP v7+ functionality) while adding some minor security features. My main goal was to maintain native methods for session handling while transparently adding the following features:

  1. Minor protection from session-hijacking by using client user agent as a "request signature." I decided not to use client IP or remote IP as they are highly unreliable and somewhat easy to spoof. I decided to hash the result to save space in the db and because I have no reliable way to know how long a particular user-agent data string will be (new or updated browser data may be much longer than the present).
  2. Built-in protection from session-fixation by forcing sessions to regenerate sid every x number of seconds.
  3. Built-in session expiration without relying on GC method (which does not always happen and is very unpredictable). The native GC was meant to clean up resources rather than manage session time limits. Most devs include timestamps inside of the $_SESSION global itself. Personally, I think this is poor design as they can accidentally be overridden and handling will need to explicitly be called at every request.

I avoided using prepared statements as they take an extra call to the db. I've always advocated against prepared statements for single queries - they are a lazy way of ensuring data is properly escaped and they take almost twice as long to get a result.

I decided against using INSERT ON DUPLICATE KEY UPDATE to ensure that ONLY new sessions would be added to the db (in the case that two duplicate sids are created before either is written - although the chance of this happening is just about non-existent). Also, a single insert or a single update is a tad quicker.

I have been testing this class and it's been working well... except I can see this being too slow for a high traffic site. The main bottleneck is connection and queries to the database (can't do much about that). I'd like to expand on the request signature but I can't think of a reliable way to uniquely identify a client. I'd also appreciate any feedback/criticism about this class as I'd like to expand on it. Thank you!

Here is the code:

<?php
class SessionAuthException extends RuntimeException{
    public function __construct($message = 'Unknown reason', $code = 0, Exception $previous = null){
        parent::__construct('Session could not be authenticated: ' . $message, $code, $previous);
    }
}

class MySQLSessionHandler implements SessionHandlerInterface{   
    private $link;
    private $cOptions = [
        'regen_interval' => 600,
        'idle_timeout' => 3600];

    private $curReqTime;
    private $reqSignature;

    private $sesInitTime;
    private $lastReqTime;
    private $sesExpireTime;
    private $numWrites;

    private $isNew = false;
    private $doExpire = false;

    public function __construct(mysqli $link, bool $autoInit = true, array $cOptions = []){
        $this->link = $link;
        $this->curReqTime = $_SERVER['REQUEST_TIME'];
        $this->reqSignature = md5($_SERVER['HTTP_USER_AGENT']);

        if( $autoInit ){
            session_set_save_handler($this);
        }

        /* TODO: cOptions from args */

        return;
    }

    public function getInitTime(){
        return $this->sesInitTime;
    }

    public function getCurReqTime(){
        return $this->curReqTime;
    }

    public function getLastRequestTime(){
        return $this->lastReqTime;
    }

    public function getExpireTime(){
        return $this->sesExpireTime;
    }

    public function getNumWrites(){
        return $this->numWrites;
    }

    public function setExpire(bool $doExpire = true){
        $this->doExpire = $doExpire;
        return;
    }

    public function isMarkedExpire(){
        return $this->doExpire;
    }

    public function start(array $options = []){
        try{
            @session_start($options);
            if( isset($this->cOptions['regen_interval']) && $this->sesInitTime < $this->curReqTime - $this->cOptions['regen_interval'] ){
                $this->doExpire = true;
                session_regenerate_id(false);
            }
        }catch(SessionAuthException $e){
            /* Unable to authenticate session - setting sid to null will create a new session without destroying the old (possibly hijacked) */
            session_id(null);
            session_start($options);
        }
    }

    public function open($savePath, $sessionName) : bool{
        /* mysqli->ping() returns null if connection has been closed */
        return @$this->link->ping() ?: false;
    }

    public function create_sid() : string{
        $checkCollision = session_status() == PHP_SESSION_ACTIVE;

        $sid_len = ini_get('session.sid_length');
        $sid_bpc = ini_get('session.sid_bits_per_character');
        $bytes_needed = ceil($sid_len * $sid_bpc / 8);
        $mask = (1 << $sid_bpc) - 1;
        $out = '';

        $hexconvtab = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ,-';

        $attempts = 0;
        $maxAttempts = 5;
        do{
            if( $attempts >= $maxAttempts ){
                throw new Exception('Could not generate non-colliding sid after ' . $maxAttempts . ' attempts');
            }
            $random_input_bytes = random_bytes($bytes_needed);

            $p = 0;
            $q = strlen($random_input_bytes);
            $w = 0;
            $have = 0;

            $chars_remaining = $sid_len;
            while( $chars_remaining-- ){
                if( $have < $sid_bpc ){
                    if( $p < $q ) {
                        $byte = ord($random_input_bytes[$p++]);
                        $w |= ($byte << $have);
                        $have += 8;
                    }else{
                        break;
                    }
                }
                $out .= $hexconvtab[$w & $mask];
                $w >>= $sid_bpc;
                $have -= $sid_bpc;
            }
            $attempts++;
        }while( $checkCollision && $this->sessionExists($out) );

        $this->isNew = true;

        return $out;
    }

    public function validateId(string $sid) : bool{
        /* Validate ID is called after create_sid, create_sid already checks for collision */
        return $this->isNew ?: $this->sessionExists($sid);
    }

    private function sessionExists(string $sid) : bool{
        $sid = $this->link->escape_string($sid);

        $result = $this->link->query('SELECT 1 FROM `sessions` WHERE `session_id` = \'' . $sid . '\';');

        if( !$result ){
            throw new Exception('Could not determine if session exists: query failed');
        }

        return $result->num_rows;
    }

    public function read($sid) : string{
        if( $this->isNew ){
            /* New session created from self */
            $this->sesInitTime = $this->curReqTime;
            $this->lastReqTime = null;
            $this->sesExpireTime = null;
            $this->numWrites = 0;
            $out = '';
        }elseif( ($result = $this->querySession($sid)) ){
            /* Existing session - validate now */
            if( $result['request_signature'] && $this->reqSignature !== $result['request_signature'] ){
                throw new SessionAuthException('Client request signature mismatch');
            }elseif( $result['expire_unixtime'] && $result['expire_unixtime'] < $this->curReqTime ){
                throw new SessionAuthException('Session is expired');
            }
            /* Valid session did not throw */
            $this->sesInitTime = $result['init_unixtime'];
            $this->lastReqTime = $result['last_request_unixtime'];
            $this->sesExpireTime = $result['expire_unixtime'];
            $this->numWrites = $result['writes'];
            $out = $result['data'];
        }else{
            /* New session initialized elsewhere - potentially unsafe, but still no collision */
            trigger_error('Potentially unsafe read from uninitialized session: see "session.use_strict_mode"', E_USER_WARNING);
            $this->isNew = true;
            $this->sesInitTime = $this->curReqTime;
            $this->lastReqTime = null;
            $this->sesExpireTime = null;
            $this->numWrites = 0;
            $out = '';
        }

        return $out;
    }

    private function querySession(string $sid) : ?array{
        $sid = $this->link->escape_string($sid);

        $result = $this->link->query('SELECT * FROM sessions WHERE session_id = \'' . $sid . '\';');

        if( !$result ){
            throw new Exception('Failed to import session: query failed');
        }

        return $result->num_rows ? $result->fetch_assoc() : null;
    }

    public function write($sid, $data) : bool{
        /* Determine expire unixtime */
        if( $this->doExpire ){
            $expireTime = 0;
        }elseif( is_int($this->cOptions['idle_timeout']) ){
            $expireTime = $this->curReqTime + $this->cOptions['idle_timeout'];
        }else{
            $expireTime = 'null';
        }

        $sid = $this->link->escape_string($sid);
        $reqSignature = $this->link->escape_string($this->reqSignature);
        $data = $this->link->escape_string($data);

        if( $this->isNew ){
            $this->link->query(
                'INSERT INTO sessions (session_id, init_unixtime, last_request_unixtime, expire_unixtime, request_signature, writes, data) '.
                'VALUES(\'' . $sid . '\', ' . $this->curReqTime . ', init_unixtime, ' . $expireTime . ', \'' . $reqSignature . '\', 1, \'' . $data . '\');');
        }else{
            $this->link->query(
                'UPDATE sessions '.
                'SET last_request_unixtime = ' . $this->curReqTime . ', expire_unixtime = ' . $expireTime . ', request_signature = \'' . $reqSignature . '\', writes = writes + 1, data = \'' . $data . '\' '.
                'WHERE session_id = \'' . $sid . '\';');
        }

        return $this->link->affected_rows > 0;
    }

    public function gc($maxLifetime) : bool{
        return $this->link->query('DELETE FROM sessions WHERE expire_unixtime <= ' . $this->curReqTime . ';');
    }

    public function close() : bool{
        $sesInitTime = null;
        $lastReqTime = null;
        $sesExpireTime = null;
        $numWrites = null;
        $this->isNew = false;
        $this->doExpire = false;
        /* Keep connection open for use - in case new session_start() or in the case of session_regenerate_id() */
        return true;
    }

    public function destroy($sid) : bool{
        $sid = $this->link->escape_string($sid);

        $this->link->query('DELETE FROM sessions WHERE session_id = \'' . $sid . '\';');

        return $this->link->affected_rows > 0;
    }

    public function __destruct(){
        /* This will not be called in the case of Exception - resource handle will persist until PHP GC happens */
        @$this->link->close();
    }
}

?>

And here is how to use with built-in exception handling:

$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Use start method for automatic error handling when authentication errors occur */
$sesHandler->start();
$_SESSION['test'] = 'hello world';
session_write_close();

And here is how to use with manual exception handling:

const REGEN_INTERVAL_SEC = 600;

$sesHandler = new MySQLSessionHandler(new mysqli('hostName', 'user', 'password', 'dbn'));

/* Example using native session_start with manual controls - NOTE: this example does exactly what MySQLSessionHandler::start() does */
try{
    session_start();
    /* Regenerate stagnant session id */
    if( $sesHandler->getInitTime < ($sesHandler->getCurReqTime() - REGEN_INTERVAL_SEC) ){
        /* Preserve session date while marking session expired */
        $sesHandler->setExpire(true);
        session_regenerate_id(false);
    }
}catch(SessionAuthException $e){
    /* Client/request could not be authenticated - start a fresh session */
    session_id(null);
    session_start();
}

$_SESSION['test'] = 'hello world';
session_write_close();

Here is the GIT!

\$\endgroup\$
  • \$\begingroup\$ Do you have a requirement to make this work for a high-traffic site? If not, don't worry about the performance. If yes, provide some more details. \$\endgroup\$ – Rene Saarsoo May 3 '18 at 13:53
  • \$\begingroup\$ @ReneSaarsoo My current project will never see very high traffic... and this handler processed 1000 simulated requests in about 1.5s on my server (decent I think). Still, I will always be concerned with performance as a habit... and esp here since this class will be used in almost every request (even AJAX). If you have any sort of performance improvements (minus switching to a different storage method), I am all ears! Thanks \$\endgroup\$ – Phillip Weber May 3 '18 at 15:29
2
\$\begingroup\$

Without looking too deeply into it, a few things immediately pop out:

  • This class is likely too large with its ~250 lines of code.
  • It has quite a bit of internal state, stored in 10 instance variables.
  • SessionHandlerInterface requires implementing 6 methods, but this class has an entire 17 public methods.

It's likely doing several things that could be extracted to other classes or functions. A good candidate for extraction is the create_sid() function. It seems to me that this has been taken from some 3rd party website or library, as its dash_separated_name doesn't fit in with other camelCasedNames in this class.

1. Replying to your comment:

For me, since everything in this class deals with sessions and how they are handled [--], doesn't it make sense to keep them together [--].

I guess it comes down to an age-old question: which criteria should you use for splitting up your code to modules/classes/functions/etc?

A good guideline to follow, is the single responsibility principle, which states:

A class should have only one reason to change.

I would expect MySQLSessionHandler to change when you need to change something in the relationship of how you're storing the session data to MySQL database.

However I would not expect changing how session ID is generated to require a change in MySQLSessionHandler. The SID does not have much to do with MySQL - you might use the same ID generator for FileSessionHandler or PostgresSessionHandler.

2. Replying to another comment:

[--] decided to add getters in the case that a user would want some sort of external control.

Follow the You ain't gonna need it principle:

Don't add methods/properties to a class just-in-case. Have a clear purpose and clear plan how you'd expect your class to be used. Unless you're really sure, it's better to leave it out. Less is more.

Design first for your own specific use case. Leave out everything you don't actually need. Then try to use it in another project - see if it works as is, or do you need to improve it somehow. Only after having found it useful in several places of your own, consider sharing it with a wider audience.

\$\endgroup\$
  • \$\begingroup\$ The internal state is purposeful - I wanted to keep the control mostly contained to the class (safe from overwrite/erroneous changes) but decided to add getters in the case that a user would want some sort of external control. Do you think there is a good reason to externalize these properties? \$\endgroup\$ – Phillip Weber May 3 '18 at 17:22
  • \$\begingroup\$ create_sid was taken from elsewhere... although I've modified it a bit. It is basically an emulation of the native create_sid.. I'm trying to understand a good reason to rewrite it outside of the class method.. esp since it does exactly what create_sid is supposed to do. Another thought is to simple fallback to parent::create_sid which is faster... but will check for native handler collision, which COULD cause errors. What do you think? \$\endgroup\$ – Phillip Weber May 3 '18 at 17:30
  • \$\begingroup\$ I will definitely rewrite create_sid vars to camelCase to make things prettier! I would appreciate your thoughts about the benefits of splitting this class. For me, since everything in this class deals with sessions and how they are handled (further, the class relies on the private properties/functions in order to work), doesn't it make sense to keep them together and avoid additional overhead of another class (include/construct/destruct)? \$\endgroup\$ – Phillip Weber May 3 '18 at 17:40
  • \$\begingroup\$ I wonder how this create_sid is even used. Your class itself does not use it, neither does your example code. But the class does have several methods that accept sid as input. \$\endgroup\$ – Rene Saarsoo May 3 '18 at 17:41
  • \$\begingroup\$ PHP has native functions that have internal callbacks... i.e.: session_start() calls handler::open() -> [conditionally] handler::create_sid() -> [optionally] handler::validateId() -> handler::read(). They are never meant to be called directly by a user but need to remain public. Session Handlers in PHP aren't as simple and straight forward as they seem :X \$\endgroup\$ – Phillip Weber May 3 '18 at 17:50

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.