5
\$\begingroup\$

I have made the decision to move the storing of session data from the filesystem to the database. Our application is growing at pace and we are having issues with the load balancer breaking the sessions (even with sticky sessions on).

Whilst obviously the idea here is to have my code reviewed, I have a few questions/areas I'd really appreciate some feedback on, which I have detailed after the code.

My sessions table:

CREATE TABLE sessions
(
  id VARCHAR(40) NOT NULL,
  data TEXT,
  access TEXT,
  PRIMARY KEY (id),
  INDEX (id)
);

Session class:

<?php
namespace Company\Project;

use \PDO;


class Session
{
    private $dblayer;
    private $user_agent;

    public function __construct(PDO $dblayer)
    {
        $this->dblayer = $dblayer;
        $this->user_agent = $_SERVER['HTTP_USER_AGENT'];


        session_set_save_handler(
            array($this, 'open'),
            array($this, 'close'),
            array($this, 'read'),
            array($this, 'write'),
            array($this, 'destroy'),
            array($this, 'gc')
        );

        if ('LIVE' == DEVELOPMENT_MODE) {
            session_set_cookie_params(0, '/', '', true, true);
        } else {
            session_set_cookie_params(0, '/', '', false, true);
        }

        session_register_shutdown();
        session_start();

    }

    public function checkUserAgent()
    {
        if ($_SERVER['HTTP_USER_AGENT'] === $this->user_agent) {
            session_regenerate_id(true);
            return true;
        }

        return false;
    }

    public function open()
    {
        if ($this->dblayer) {
            return true;
        }

        return false;
    }

    public function close()
    {
        $this->dblayer = null;
        if (!$this->dblayer) {
            return true;
        }

        return false;
    }

    public function read($id)
    {
        try {
            $this->dblayer->beginTransaction();
            $stmt = $this->dblayer->prepare("SELECT data FROM sessions WHERE id = :id LIMIT 1");
            $stmt->bindParam(':id', $id);
            $stmt->execute();

            $this->dblayer->commit();

            if ($data = $stmt->fetch()) {
                return $data['data'];
            }

            return '';

        } catch (\Exception $e) {
            $this->dblayer->rollBack();
            // will use file_put_contents to save error message, file etc to error log
            return false;
        }
    }

    public function write($id, $data)
    {
            $access = time();
            $this->dblayer->beginTransaction();
            $stmt = $this->dblayer->prepare("REPLACE INTO sessions VALUES(:id, :data, :access)");
            $stmt->bindParam(':data', $data);
            $stmt->bindParam(':id', $id);
            $stmt->bindParam(':access', $access);
            $stmt->execute();

            $this->dblayer->commit();

            if ($stmt) {
                return true;
            }
            echo 'error';
            $this->dblayer->rollBack();
            // can i save to error log here?
            return false;

    }

    public function destroy($id)
    {
        try {
            $this->dblayer->beginTransaction();
            $stmt = $this->dblayer->prepare("DELETE FROM sessions WHERE id = :id");
            $stmt->bindParam(':id', $id);
            $stmt->execute();

            $this->dblayer->commit();

        } catch (\PDOException $e) {
            $this->dblayer->rollBack();
            // again, will save error data to log
            echo $e->getMessage();
            return false;
        }
    }

    public function gc($max)
    {
        $to_delete = time() - $max;

        try {
            $this->dblayer->beginTransaction();
            $stmt = $this->dblayer->prepare("DELETE FROM sessions WHERE access < :to_delete");
            $stmt->bindParam(':to_delete', $to_delete);

            $this->dblayer->commit();

            return true;

        } catch (\PDOException $e) {
            $this->dblayer->rollBack();
            // save error data to log;
            return false;
        }
    }

}

Sample usage:

include '../config.php';
$dblayer = dbh_connect();
new \Company\Project\Session($dblayer);

Questions / areas of interest:

  1. This manual page seem to strongly suggest using register_shutdown_function or session_register_shutdown, yet most examples I see online of similar classes, do not. Am I using it correctly (or does it need to go in each script where I call this session class)?

  2. I always use transactions when querying my database - I assume I'm OK to do so here, too?

  3. My close function - the PHP Manual states the function should return true or false, yet I'm not doing anything that could possibly fail, so this seems a bit pointless to me.

  4. Currently, I'm not using my check user agent method. Where could I incorporate it?

  5. Where would I put session_regenerate_id() when using this class?

  6. Are there other steps I can take to increase security? I use a LOT of ajax in this application.

  7. I'm concerned about the sessions table becoming very large with the normal session garbage probability - would a cron job to clear out old sessions be better?

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

I haven't personally implemented a custom session handler, so take everything below with a grain of salt.

3) My close function - the PHP Manual states the function should return true or false, yet I'm not doing anything that could possibly fail, so this seems a bit pointless to me.

If there is nothing that can fail - which I would agree is the case here - just always return true instead of making the code unnecessarily complex.

4) Currently, I'm not using my check user agent method, where could I incorporate it?

First of all, I would remove the call to session_regenerate_id. There is really no reason to regenerate the session id each time you check the useragent.

If you want to bind a session to a user agent*, the best place would probably be read (and then you could destroy the session and return the empty string if it doesn't match).

But then it's important to not use the stored user agent from the Session class - because this will always match - but to store the useragent in the database when first creating the session, and to then compare the current useragent against that stored value.

* this would somewhat increase security as it makes it more difficult for an attacker to use leaked sessions. Of course, if the attacker has eg XSS, they can simply read out the user agent (or use CSRF to perform any action the victim can perform); still, it's a good idea as defense in depth.

5) Where would I put session_regenerate_id() when using this class?

From a security standpoint, you want to regenerate the session id when the session state changes, to prevent session fixation. So you definitely want to do that when the session is created, and on subsequent writes.

Additionally, you could regenerate the id in intervals of X minutes, but a regeneration on each request is not needed and causes unnecessary overhead (and may actually cause problems if requests overlap).

Misc

  • is there a reason that you don't use the SessionHandlerInterface? It seems that it would simplify your code a bit.
  • the documentation for read says that it must return the empty string when nothing was read, so I would also return that if an exception occurred.
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for this - very helpful. Would you mind taking another look? I've modified to fit most of your suggestions, but got a little confused with the user agent. Also - how would using SessionHandlerInterface simplify my code? Thanks again \$\endgroup\$ – C Ivemy Jan 12 '16 at 22:56
  • \$\begingroup\$ @CIvemy I don't really know enough about custom session handlers to really help here, my area is really more security related. But: by putting session_regenerate_id in the constructor, you still regenerate the id on every request (it seems that all the methods are called on each request though, so I'm unsure of where to put it instead, except outside of this class, which may be the best solution) \$\endgroup\$ – tim Jan 12 '16 at 23:18
  • \$\begingroup\$ regarding user agent: this should work, I think, but I would test it thoroughly (I don't really like that the class now exits though, because the calling code can't handle this case and it violates the interface; an alternative may be to move this outside of the class as well, to the same place where you move the id regeneration). regarding SessionHandlerInterface: you wouldn't have to pass all the array arguments to session_set_save_handler, but just the session object. \$\endgroup\$ – tim Jan 12 '16 at 23:18
  • \$\begingroup\$ I also rolled back your edit, see What you may and may not do after receiving answers. I hope you get some other answers, hopefully from people with more experience with custom session handlers :) \$\endgroup\$ – tim Jan 12 '16 at 23:18

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.