My Session Handler

Question

I'm yet to add garbage cleanup, ID regeneration and the ability to unset sessions, but this is what I have so far.

• Does this help prevent session hijacking/fixation?
• Can you see any vulnerabilities?

index.php

require 'Session.class.php';

// I know this isn't done very well
$pdo = new PDO('mysql:host=localhost;dbname=session', 'root', 'root');

$session = new Session($pdo);

$session->set(array(
    'username'      =>  'hello'
));

echo $session->get('username');

Session.class.php

<?php

class Session {
    private $_db = null;
    private $_expiry = 7200;
    private $_id;
    private $_data = array();

    public function __construct(PDO $db) {
        $this->_db = $db;
        $this->_run();

        if (mt_rand(0, 10) <= 8) $this->_garbageCleanup();
    }

    public function get($key) {
        return isset($this->_data[$key]) ? $this->_data[$key] : FALSE;
    }

    public function set($data = array()) {
        $this->_data = array_merge($data, $this->_data);

        $query = $this->_db->prepare("UPDATE `sessions` SET `session_data` = ? WHERE `session_id` = ?");
        $query->bindValue(1, serialize($this->_data));
        $query->bindValue(2, $this->_id);
        $query->execute();
    }

    private function _run() {
        if (isset($_COOKIE['session'])) {
            $this->_id = $_COOKIE['session'];

            $query = $this->_db->prepare("SELECT * FROM `sessions` WHERE `session_id` = ? AND `session_ip` = ? AND `session_user_agent` = ?");
            $query->bindValue(1, $this->_id);
            $query->bindValue(2, $_SERVER['REMOTE_ADDR']);
            $query->bindValue(3, $_SERVER['HTTP_USER_AGENT']);
            $query->execute();
            $result = $query->fetch();

            $this->_data = unserialize($result['session_data']);
        } else {
            $this->_createSession();
        }
    }

    private function _createSession() {
        $this->_id = md5(microtime() . mt_rand(0, 100));

        $query = $this->_db->prepare("INSERT INTO `sessions` VALUES (?, ?, ?, ?)");
        $query->bindValue(1, $this->_id);
        $query->bindValue(2, serialize($this->_data));
        $query->bindValue(3, $_SERVER['REMOTE_ADDR']);
        $query->bindValue(4, $_SERVER['HTTP_USER_AGENT']);
        $query->execute();

        setcookie('session', $this->_id, time() + $this->_expiry);
    }

    private function garbageCleanup() {
        // Does nothing yet, but will later!
    }
}

Answer 1

At this moment $_SESSION would be a lot safer then storing it in the database.

What you are doing is the same as session, but instead of storing everything in a file in the filesystem (like $_SESSION does) you are storing it in the database.

$_SESSION works with a cookie (just like your code) that sets an id. The only validation is that 'id'. Once you have guessed that one, you have succesfully hijacked the session.

The only difference here is you are adding

1. HTTP_USER_AGENT (pointless, this is simply data comming from the client so it can't be trusted)
2. REMOTE_ADDR (also comming from the client and it won't protect vs man in the middle atacks).

So what your left is, is an id. Just like $_SESSION. And just like with $_SESSION, security stands and falls with the generation of that ID.

• You use md5 as hashing algorithm. This is bad, very bad: http://en.wikipedia.org/wiki/MD5#Security (just read the first sentnce).
• Apart from md5, you are also generating it using a time-based algorithm, never, just never do that. If i know the time a session started, I only need 100 trys... (mt_rand(1,100)). Oh, and mt_rand isn't really random. It is (yes you guessed it, time based pseudo random).

What i recommend is that you use a much stronger algorithm for creating that ID. e.g. pbkdf2: http://php.net/manual/en/function.hash-pbkdf2.php or similar.

the code itself looks clean! but i have some performance problems:

Everytime you set a var, you do an update to the database. This is inperformant. Simply use a __destruct() function where the actual database record gets updated.

Answer 2

The code has much duplication that should be eliminated.

Every database call, technically, is duplicated functionality. The call to $this->_db->prepare occurs many times, but need only occur once. Consider the following idea:

<?php
use PDO;
use PDOException;

/**
 * Used for interacting with the database. Usage:
 * <pre>
 * $db = Database::get();
 * $db->call( ... );
 * </pre>
 */
class Database {
  private static $instance;
  private $dataStore;

  /**
   * Sets the connection that this class uses for database transactions.
   */
  public function __construct() {
    global $dbhost;
    global $dbname;
    global $dbuser;
    global $dbpass;

    try {
      $this->setDataStore(
        new PDO( "pgsql:dbname=$dbname;host=$dbhost", $dbuser, $dbpass ) );
    }
    catch( PDOException $ex ) {
      $this->log( $ex->getMessage() );
    }
  }

  /**
   * Returns the singleton database instance.
   */
  public function get() {
    if( self::$instance === null ) {
      self::$instance = new Database();
    }

    return self::$instance;
  }

  /**
   * Call a database function and return the results. If there are
   * multiple columns to return, then the value for $params must contain
   * a comma; otherwise, without a comma, the value for $params is used
   * as the return column name. For example:
   *
   *- SELECT $params FROM $proc( ?, ? ); -- with comma
   *- SELECT $proc( ?, ? ) AS $params; -- without comma
   *- SELECT $proc( ?, ? ); -- empty
   *
   * @param $proc Name of the function or stored procedure to call.
   * @param $params Name of parameters to use as return columns.
   */
  public function call( $proc, $params = "" ) {
    $args = array();
    $count = 0;
    $placeholders = "";

    // Key is zero-based (e.g., $proc = 0, $params = 1).
    foreach( func_get_args() as $key => $parameter ) {
      // Skip the $proc and $params arguments to this method.
      if( $key < 2 ) continue;

      $count++;
      $placeholders = empty( $placeholders ) ? "?" : "$placeholders,?";
      array_push( $args, $parameter );
    }

    $sql = "";

    if( empty( $params ) ) {
      // If there are no parameters, then just make a call.
      $sql = "SELECT $proc( $placeholders )";
    }
    else if( strpos( $params, "," ) !== false ) {
      // If there is a comma, select the column names.
      $sql = "SELECT $params FROM $proc( $placeholders )";
    }
    else {
      // Otherwise, select the result into the given column name.
      $sql = "SELECT $proc( $placeholders ) AS $params";
    }

    $statement = $this->getDataStore()->prepare( $sql );

    //$this->log( "SQL: $sql" );

    for( $i = 1; $i <= $count; $i++ ) {
      //$this->log( "Bind " . $i . " to " . $args[$i - 1] );
      $statement->bindParam( $i, $args[$i - 1] );
    }

    $statement->execute();
    $result = $statement->fetchAll();
    $this->decodeArray( $result );

    return $result;
  }

  /**
   * Converts an array of numbers into an array suitable for usage with
   * PostgreSQL.
   *
   * @param $array An array of integers.
   */
  public function arrayToString( $array ) {
    return "{" . implode( ",", $array ) . "}";
  }

  /**
   * Recursive method to decode a UTF8-encoded array.
   *
   * @param $array - The array to decode.
   * @param $key - Name of the function to call.
   */
  private function decodeArray( &$array ) {
    if( is_array( $array ) ) {
      array_map( array( $this, "decodeArray" ), $array );
    }
    else {
      $array = utf8_decode( $array );
    }
  }

  private function getDataStore() {
    return $this->dataStore;
  }

  private function setDataStore( $dataStore ) {
    $this->dataStore = $dataStore;
  }
}
?>

For some example usages, see: Generic method for database calls