4
\$\begingroup\$

I have tried to write a small light weighted php session handling class that use PHP's session_set_save_handler() function to overwrite the default session handling functionalities and usage Database to store the sessions data instead of default files system. It checks for possible session hijacking attempts and renews session periodically. I want to know how feasible my class is and improvements that can be incorporated to make it more robust and secure.

sessionmanager.lib.php:

<?php

trait Singleton
{
    private static $_instance;

    public static function getInstance($config = array())
    {
        if (!(self::$_instance instanceof self)) {
            self::$_instance = new self($config);
        }

        return self::$_instance;
    }
}

/**
 * @category    Sessionmanager
 * @version     1.0
 * @author      Anirban Nath <[email protected]>
* */


class sessionmanager
{
    use Singleton;

    /**
    * [$_db PDO object holder]
    * @var [object]
    */
    private $_db;

    /**
     * [$_https Cookie secure flag holder]
     * @var [boolean]
    */

    private $_https;

    /**
     * [$_user_agent User Agent holder]
     * @var [string]
     */

    private $_user_agent;

    /**
     * [$_ip_address Client Machien IP address]
     * @var [string]
    */

    private $_ip_address;

    /**
     * [$_expiry Session LIfetime, default 2 Hours ]
     * @var integer
    */

    private $_expiry                =  7200;

    /**
     * [$_session_cookie_ttl Session Cookie Lifetime , default (0:Clear the session cookies on browser close) ]
     * @var integer
    */

    private $_session_cookie_ttl    =  0;

    /**
     * [$_refresh_interval Refresh Interval toi regenerate Session Id, default 10 minutes]
     * @var integer
    */

    private $_refresh_interval      =  600;

    /**
    * [$_table_name Tbale name for storing Session information]
    * @var string
    */

    private $_table_name            = "sessions";

    /**
    * [$_session_id Holder for session_id]
    * @var [string]
    */
    private $_session_id;

    /**
    * Sescure session Salt
    * @ClassConstant
    */
    const SECURE_SESSION            = '--$ecure$ess10n--';


    /**
     * [__construct ,Pass configuration values to __setconfig, register session handlers and starts the sesssion]
     * @param array $config [array of configuartion params]
    */

    public function __construct(array $config)
    {
        session_set_save_handler(
            array($this, 'open'),
            array($this, 'close'),
            array($this, 'read'),
            array($this, 'write'),
            array($this, 'destroy'),
            array($this, 'gc')
        );

        $this->_setConfig($config);
        session_start();
    }

    /**
     * [_setConfig Sets up the configurations values passed in by __contsruct function and creates a session storage MySql table]
     * @param [Array] $config [Configs params holder]
    */

    private function _setConfig($config)
    {
        $this->_db                  = $config['dbconnector'];
        $this->_expiry              = (isset($config['expiry']))? $config['expiry'] : $this->_expiry ;
        $this->_session_cookie_ttl  = (isset($config['session_cookie_ttl']))? $config['session_cookie_ttl'] : $this->_session_cookie_ttl ;
        $this->_https               = (isset($_SERVER['HTTPS'])) ? TRUE: FALSE;
        $this->_refresh_interval    = (isset($config['refresh_interval'])) ? $config['refresh_interval']: $this->_refresh_interval;
        $this->_user_agent          = isset($config['user_agent']) ? $config['user_agent'] : $_SERVER['HTTP_USER_AGENT'];
        $this->_ip_address          = $this->_getRealIpAddr();

        ini_set('session.cookie_lifetime', $this->_session_cookie_ttl);
        ini_set('session.gc_maxlifetime',  $this->_expiry);
        ini_set('session.cookie_httponly', 1);
        ini_set('session.entropy_file', '/dev/urandom');
        ini_set('session.hash_function', 'whirlpool');
        ini_set('session.use_only_cookies', 1);
        ini_set('session.cookie_secure', $this->_https);
        ini_set('session.entropy_length' ,512);
        ini_set('session.use_trans_sid', false);

        $stmt_create = "CREATE TABLE IF NOT EXISTS {$this->_table_name} (
                        `session_id` varchar(255) NOT NULL,
                        `data` text,
                        `user_agent` varchar(255) NOT NULL,
                        `ip_address` varbinary(16) NOT NULL,
                        `last_updated` int(11) NOT NULL,
                        `fingerprint` varchar(255) NOT NULL,
                        PRIMARY KEY (`session_id`)
                        ) ENGINE=InnoDB DEFAULT CHARSET=utf8";

        $this->_db->exec($stmt_create);
    }

    /**
     * [_getRealIpAddr Get the IP address of the user]
     * @return [strings] [IP address of the client]
    */

    private function _getRealIpAddr()
    {
      if (!empty($_SERVER['HTTP_CLIENT_IP']))
      {
        /*check ip from share internet*/
        $ip     =   $_SERVER['HTTP_CLIENT_IP'];
      }
      elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR']))
      {
        /*to check ip is pass from proxy*/
        $ip     =   $_SERVER['HTTP_X_FORWARDED_FOR'];
      }
      else
      {
        $ip     =   $_SERVER['REMOTE_ADDR'];
      }

      return $ip;
    }

    /**
     * [open The open callback works like a constructor in classes and is executed when the session is being opened.
     * It is the first callback function executed when the session is started automatically or manually with session_start().
     * Return value is TRUE for success, FALSE for failure. ]
     * @param  [string] $path [Path for saving session file]
     * @param  [string] $name  [Session Name]
     * @return [boolean]
     */

    public function open($path, $name)
    {
        return true;
    }

    /**
     * [close The close callback works like a destructor in classes and is executed after the session write callback has been called.
     * It is also invoked when session_write_close() is called. Return value should be TRUE for success, FALSE for failure. ]
     * @return [boolean]
     */
    public function close()
    {
        /*calling explicitly method gc(),that will clear all expired sessions*/
        $this->gc();
        return true;
    }

    /**
    * [_refresh Whenever new session id is required we can call this method, sets new session id]
    */
    private function _refresh()
    {
        session_regenerate_id(true);
        $this->_session_id = session_id();
    }


    /**
     * [_needRenewal method for checking if the session needs Renewal from $_refresh_interval]
     * @param  [int] $id [session_id]
     * @return [boolean]
     */
    private function _needRenewal($id)
    {

        $stmt    = $this->_db->prepare("SELECT last_updated FROM {$this->_table_name} WHERE session_id = ?");
        $stmt->execute(array($id));
        $record  = current($stmt->fetchAll());

        if ($record !== FALSE && count($record) > 0)
        {
            /*Checks if the session ID has exceeded it's permitted lifespan.*/
            if((time() - $this->_refresh_interval) > $record['last_updated'])
            {
                /*Regenerates a new session ID*/
                $this->_refresh();

                $sql = "UPDATE {$this->_table_name} SET session_id =:session_id, last_updated =:last_updated WHERE session_id = '$id'";

                $stmt = $this->_db->prepare($sql);
                $stmt->bindParam(':last_updated', $id , PDO::PARAM_INT);
                $stmt->bindParam(':session_id', $this->_session_id , PDO::PARAM_STR); //this is what will be returned by Refresh
                $stmt->execute();
                return true;
            }
        }

        return false;
    }

    /**
    * [_isExpired method for checking if the current session is expired]
    * @param  [array]  $record [session info array passed in by _read()]
    * @return boolean
    */
    private function _isExpired($record)
    {
       $ses_life = time() - $this->_expiry;
       $stmt     = $this->_db->prepare("SELECT session_id FROM {$this->_table_name} WHERE last_updated < ? AND session_id = ?");
       $stmt->execute(array($ses_life, $record['session_id']));
       $record   = current($stmt->fetchAll());

       if($record)
         return true;
       else
         return false;
    }

    /**
     * [read  The read callback must always return a session encoded (serialized) string,
     * or an empty string if there is no data to read.
     * This callback is called internally by PHP when the session starts or when session_start() is called.
     * Before this callback is invoked PHP will invoke the open callback. ]
     * @param  [string] $id [session_id]
     */
    public function read($id)
    {
        try
        {

            $stmt = $this->_db->prepare("SELECT session_id, fingerprint, data, user_agent, INET6_NTOA(ip_address), last_updated  FROM {$this->_table_name} WHERE  session_id = ?");
            $stmt->execute(array($id));
            $record = current($stmt->fetchAll());
            if(empty($record['session_id']))
            {
                $this->_refresh();
                return '';
            }
            else
            {
                if($this->_isSuspicious($record['fingerprint']) || $this->_isExpired($record))
                {
                    $this->destroy($id);
                    throw new Exception('Possible Session Hijack attempt/Session expired/Some mismatch.');
                }
                else
                {
                    /*Need a renewal ?*/
                    if($this->_needRenewal($id))
                    {
                        /*recursive call*/
                        $this->read($this->_session_id);
                    }

                    return $record['data'];
                }

            }

        }
        catch(PDOException $e)
        {
            echo $e->getMessage();
            $this->_refresh();
            return '';
        }
        catch(Exception $e)
        {
            echo $e->getMessage();
            $this->_refresh();
            return '';
        }
    }

    /**
    * [_getFingerPrint generates session fingerprints md5(USER AGENT + SECURE SESSION + IP ADDRESS)]
    * @return [strings] [encryted session info fingerprint]
    */
    private function _getFingerPrint()
    {
        return md5($this->_user_agent.self::SECURE_SESSION . $this->_ip_address);
    }

    /**
    * [_isSuspicious check for possible session hijack attempt, by comaparing encrypted user system specific values against exoisting records ]
    * @param  [string]  $fp [session fingerprint]
    * @return boolean
    */
    private function _isSuspicious($fp)
    {
        return ($fp != $this->_getFingerPrint()) ? True : False;
    }

    /**
    * [write The write callback is called when the session needs to be saved and closed.
    * This callback receives the current session ID a serialized version the $_SESSION superglobal.
    * The serialization method used internally by PHP is specified in the session.serialize_handler ini setting. 
    * Here we are storing/updating the session data against the session id]
    * @param  [string] $id   [session id]
    * @param  [serilized data] $data [The serialized session data passed to this callback should be stored against the passed session ID]
    * @return [boolean]
    */
    public function write($id, $data)
    {

        try
        {

            $sql = "INSERT INTO {$this->_table_name} (session_id, user_agent, ip_address, last_updated, data, fingerprint)
                        VALUES (:session_id, :user_agent, INET6_ATON(:ip_address), :last_updated, :data,:fingerprint)
                        ON DUPLICATE KEY UPDATE data =VALUES(data), last_updated=VALUES(last_updated)";

            $time   = time();
            $fp     = $this->_getFingerPrint();

            $stmt = $this->_db->prepare($sql);
            $stmt->bindParam(':session_id', $id , PDO::PARAM_STR);
            $stmt->bindParam(':user_agent', $this->_user_agent, PDO::PARAM_STR);
            $stmt->bindParam(':ip_address' , $this->_ip_address , PDO::PARAM_STR);
            $stmt->bindParam(':last_updated', $time , PDO::PARAM_INT);
            $stmt->bindParam(':data', $data , PDO::PARAM_STR);
            $stmt->bindParam(':fingerprint', $fp, PDO::PARAM_STR);
            $stmt->execute();
            return true;

        }
        catch(PDOException $e)
        {
            echo $e->getMessage();
            return false;
        }
        catch(Exception $e)
        {
            echo $e->getMessage();
            return false;
        }
    }

    /**
    * [destroy deletes the current session id from the database]
    * @param  [string] $id [session_id]
    * @return [boolean]
    */
    public function destroy($id)
    {
        /**/
        $stmt           = $this->_db->prepare("DELETE FROM {$this->_table_name} WHERE  session_id =  ?");
        $session_res    = $stmt->execute(array($id));

        if (!$session_res)
            return false;
        else
            return true;

    }

    /**
    * [gc The garbage collector callback is invoked internally by PHP periodically in order to purge old session data.
    * The frequency is controlled by session.gc_probability and session.gc_divisor.
    * The value of lifetime which is passed to this callback can be set in session.gc_maxlifetime.
    * here we are calling this via _close(), to delete all expired sessions from DB
    * Return value should be TRUE for success, FALSE for failure. ]
    * @return [boolean]
    */
    public function gc()
    {
        $ses_life       = time() - $this->_expiry;
        $stmt           = $this->_db->prepare("DELETE FROM {$this->_table_name} WHERE  last_updated < ?");
        $session_res    = $stmt->execute(array($ses_life));

        return true;
    }

    /**
     * [__destruct register_shutdown_function() the following prevents unexpected effects when using objects as save handlers
     * Session data is usually stored after script terminated without the need to call session_write_close(),
     * but as session data is locked to prevent concurrent writes only one script may operate on a session at any time.
     * When using framesets together with sessions you will experience the frames loading one by one due to this locking.
     * You can reduce the time needed to load all the frames by ending the session as soon as all changes to session variables are done. ]
     */
    public function __destruct()
    {
        register_shutdown_function('session_write_close');
    }

}



try
{

    $pdo = new PDO('mysql:host=localhost;dbname=test', '', '');
    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $config['dbconnector'] = $pdo;
}
catch(PDOException $e)
{
    echo $e->getMessage();
    die;
}


$s = sessionmanager::getInstance($config);

Usage example: example.php:

    <?php
      error_reporting(E_ALL);
      ini_set('display_errors', 1);
      error_reporting(-1);

      include('sessionmanager.lib.php');

      $_SESSION['Motto'] = 'Lets Do it !';
      var_dump($_SESSION);

      exit;
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

When looking over your code I find several things I would change. It should be said that I am no expert in security, so I will forward you to places where you can get better guidance than I can about the specific issue. So without further ado, let's go!

The first thing which pops into my mind when reading your code is the singleton. The singleton pattern is an anti-pattern. And it doesn't make much sense in this case either. There should only exist one instance of a session handler at any time. Since you register the handler once and then move on the singleton is obsolete. Your class provides no interface for use outside of the handler (which is good; separating concerns!), so there is not reason for a global accessor, which the singleton getInstance() method is.

Next I would make your class implement the native interface SessionHandlerInterface. This interface was created with the idea of userland code overwriting the native session handler implementation, which is exactly what you are doing. This also means you can simplify the handler registration inside the constructor to:

session_set_save_handler($this);

Then I would also rename your class. SessionManager is very broad and you may in the future make a handler implementation using a memory-based storage engine like Memcached or Redis. Therefore I would rename your class into PDOSessionHandler as this clearly shows the storage implementation used.

I would also urge you to avoid using destructors when dealing with critical classes such as a session handlers. There are edge cases were destructors never will be executed. An example is a fatal error. You are relying on a __destruct() method to register a shutdown function, which is good, but could be moved into the constructor. This will ensure that when your class was successfully instantiated the shutdown function is also registered.

Before moving on to some general things, I would like to talk about changing INI settings inside classes. You are changing almost every setting regarding sessions inside the class. As far as I know the values you are using are good, security related. But you are also hard-coding several settings. These settings will always overwrite any settings declared inside the php.ini file. This may be confusing and can lead to bugs. I think settings like session.entrophy_file, session.entrophy_length, session.hash_function, session.use_only_cookies and session.cookie_secure should only be changed from a php.ini file. The hashing algorithm should be changeable without modifying your class and the same goes for the previously mentioned setings. There are situations where the php.ini isn't under your control. If this is the case I would argue that these settings should be set in an application wide configuration file.

Now to some code changes. There are several places where you use try-catch blocks. They are good for checking for errors, but can be misused. You are catching the base Exception class. This can lead to bugs where exceptions, which are unrelated to PDO, are thrown, then caught and silently disposed. Also echoing exception messages is bad practice. Consider an exception is thrown and the message contains sensitive information such as your database type, host or username/password. Any visitor of your site when this exception occurs will see this critical information too. A malicious user would love this.

You are calling garbage collection (on sessions, not the PHP runtime) each time a session is closed. This may not seem like a big deal when there are a small amount of sessions stored in your database. But imagine that at some point 100 sessions are stored. Each time a request finishes your database has to loop through each of the 100 sessions to check if any of them are stale. This is unnecessary and creates a slower response time from your application as well as halt any reads from other database connections while this is done. PHP defines two INI settings which are related to garbage collection: session.gc_probalility and session.gc_divisor. By default the divisor is set to 100 and the probability is set to 1. This means that there is a 1% chance of invoking garbage collection. If you increase the probability to 5 there is a 5% chance and so on. You can tweak these settings to the values required for your application. These settings should also be changeable outside your class preferably in the INI file or a application wide configurations.

Instead of checking for renewal/expiration each time you read the session I would argue you should add a timestamp check to your SQL query. An example could be:

SELECT session_data FROM sessions WHERE session_id = :id AND updated < :expire

This would effectively only fetch valid sessions. If a session has exceeded its max lifetime is should be considered invalid. The read() method would then return an empty set of data and the old session would linger around in the database until garbage collection cleans it up. The empty session data set would then signify that no session was previously active. An example of a read() method could be:

public function read($id) 
{
    $sth = $this->pdo->prepare('SELECT session_data FROM session WHERE session_id = :id AND updated < :expire');

    $expire = time() - (int) $this->lifetime;

    $sth->bindParam(':id',     $id,     \PDO::PARAM_STR);
    $sth->bindParam(':expire', $expire, \PDO::PARAM_INT);

    if(!$sth->execute()) {
        throw new \RuntimeException('Could not execute session read query.');
    }

    if($sth->numCount() > 0) {
        $row = $sth->fetch(\PDO::FETCH_ASSOC);

        return $row['session_data'];
    }

    return '';
}

This has NOT been tested!

By the way, your current _needRenewal() method has a bug inside the SQL query where a placeholder is named $id. Nothing is interpolated since it's inside single quotes and the bindParam() method looks for a placeholder named :session_id

In the above example I also checked if $sth->execute() actually returns true. If false is returned the SQL query could not be executed. Remember to check the return values of queries and throw appropriate exceptions when necessary. This can help you find bugs easier.

In your _refresh() method you call session_regenerate_id() and reassign the session id. Internally session_regenerate_id() will reassign the session id. There is no reason to store the current session id inside the class since PHP stores this for you. You can fetch the id using session_id() with no parameters. This therefore makes your _refresh() method obsolete.

Now to the last thing I will talk about. As said in the start I am no security expert, but I feel like your fingerprint solution is fishy. Imagine two different people from the same company using your site. The company uses a load balancer, which makes their public IP addresses the same. If all the company's browsers/clients are the same your fingerprint for these people would be identical. Even though you are checking the HTTP_X_FORWARDED_FOR header you cannot rely on load balancers to send the header. I cannot tell you how to properly detect sessions using the fingerprint idea. You can ask on https://security.stackexchange.com/ for help. The best advices I can give you is:

  1. Ensure session cookies aren't accessible through javascript using the INI settings session.cookie_httponly and session.use_only_cookies
  2. Only send cookie over encrypted connections, such as TLS. The can be ensured using the INI setting session.cookie_secure.
  3. Ensure a sufficient source of entrophy is used.

This was quite a mouthful. I hope you are still with me :) If you have any questions feel free to ask.

Happy coding!

\$\endgroup\$
1
  • \$\begingroup\$ thanks much for the wonderful review! The points you highlighted are correct totally , Ill improve my class . \$\endgroup\$ Aug 12, 2015 at 4:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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