Cookie encryption library

Question

Recently I needed to save session state in cookies, instead of server side. I looked around and didn't see anything similar, so I decided to write something to handle the encryption, decryption, and validation of the cookies. I wanted some opinions before/if I release it on packagist.

Any feedback would be great, thanks

https://github.com/Rosio/Encrypted-Cookies

Below are the important files, the exceptions are just child classes of Exception.

EncryptedCookie.php

<?php
namespace Rosio\EncryptedCookie;

use Rosio\EncryptedCookie\CookieStorage;
use Rosio\EncryptedCookie\CryptoSystem\iCryptoSystem;
use Rosio\EncryptedCookie\Exception\InputTooLargeException;
use Rosio\EncryptedCookie\Exception\InputTamperedException;
use Rosio\EncryptedCookie\Exception\InputExpiredException;
use Rosio\EncryptedCookie\Exception\TIDMismatchException;

class EncryptedCookie
{
    protected $name;

    protected $data;
    protected $domain;
    protected $path;
    protected $expiration;
    protected $isSecure;
    protected $isHttpOnly;

    /**
     * The system which encryptes and decryptes cookies.
     */
    protected $cryptoSystem;

    /**
     * The system which stores and retrieves cookies.
     */
    protected $cookieStorage;

    function __construct ($name)
    {
        $this->name = $name;

        $this->domain = '';
        $this->path = '/';
        $this->expiration = 0;
        $this->isSecure = false;
        $this->isHttpOnly = true;
    }

    /**
     * Load a cookie's data if available.
     *
     * @throws InputTamperedException If The returned cookie was modified locally.
     */
    function load ()
    {
        if (!$this->getCookieStorage()->has($this->name))
            return false;

        $data = $this->getCookieStorage()->get($this->name);

        try {
            $data = $this->getCryptoSystem()->decrypt($data);
        }
        catch (\Exception $e) {
            return false;
        }

        $this->data = $data;

        return $this;
    }

    /**
     * Save a cookie's data.
     *
     * @throws InputTooLargeException If the encrypted cookie is larger than 4kb (the max size a cookie can be).
     */
    function save ()
    {
        $this->getCookieStorage()->set($this);

        return $this;
    }

    /**
     * Get the data for the cookie in its encrypted form.
     * @return string
     *
     * @throws InputTooLargeException If the encrypted cookie's data will be truncated by the browser.
     */
    function getEncryptedData ()
    {
        $edata = $this->getCryptoSystem()->encrypt($this->data, $this->expiration);

        if (strlen($edata) >= 4096)
            throw new InputTooLargeException('Total encrypted data must be less than 4kb, or it will be truncated on the client.');

        return $edata;
    }

    /* =============================================================================
       Setters
       ========================================================================== */

    /**
     * Set the cryptographic system used to encrypt/decrypt the cookie.
     * @param iCryptoSystem $cryptoSystem
     */
    function setCryptoSystem (iCryptoSystem $cryptoSystem)
    {
        $this->cryptoSystem = $cryptoSystem;

        return $this;
    }

    /**
     * Set the system used to store and retrieve cookies.
     * This has been abstracted mainly for testing purposes.
     * @param CookieStorage $cookieStorage
     */
    function setCookieStorage (CookieStorage $cookieStorage)
    {
        $this->cookieStorage = $cookieStorage;

        return $this;
    }

    /**
     * Set the data to be stored in the cookie.
     * Max is about 2.8kb (encrypting adds size).
     * @param mixed $data Data to be stored in the cookie.  If it isn't a string it will be serialized.
     */
    function setData ($data)
    {
        $this->data = serialize($data);

        return $this;
    }

    /**
     * Set the expiration of the cookie.  This also corresponds to the date the cookie's data is considered invalid.
     * @param int $expiration A Unix timestamp.
     *
     * @throws InvalidArgumentException If the given expiration isn't a valid unix timestamp.
     */
    function setExpiration ($expiration)
    {
        if (!is_numeric($expiration) || $expiration < 0)
            throw new \InvalidArgumentException('Expiration must be a unix timestamp.');

        $this->expiration = $expiration;

        return $this;
    }

    /**
     * The domain the cookie is valid for.
     * @param string $domain
     */
    function setDomain ($domain)
    {
        $this->domain = $domain;

        return $this;
    }

    /**
     * The path the cookie is valid for.
     * @param string $path
     */
    function setPath ($path)
    {
        $this->path = $path;

        return $this;
    }

    /**
     * Should this cookie only be sent to the server if there is a secure connection?
     * @param boolean $isSecure
     */
    function setSecure ($isSecure)
    {
        $this->isSecure = $isSecure;

        return $this;
    }

    /**
     * Should this cookie only be available to the server (and not client-side scripts)?
     * @param boolean $isHttpOnly
     */
    function setHttpOnly ($isHttpOnly)
    {
        $this->isHttpOnly = $isHttpOnly;

        return $this;
    }

    /* =============================================================================
       Getters
       ========================================================================== */
    function getData ()
    {
        return unserialize($this->data);
    }

    function getExpiration ()
    {
        return $this->expiration;
    }

    function getPath ()
    {
        return $this->path;
    }

    function getDomain ()
    {
        return $this->domain;
    }

    function isSecure ()
    {
        return $this->isSecure;
    }

    function isHttpOnly ()
    {
        return $this->isHttpOnly;
    }

    /**
     * Retrieve the cookie storage in use by this cookie.
     * @return CookieStorage
     */
    protected function getCookieStorage ()
    {
        // Lazy load a default if one wasn't set
        if ($this->cookieStorage === null)
            $this->cookieStorage = new CookieStorage();

        return $this->cookieStorage;
    }

    /**
     * Retrieve the crypto system in use by this encrypted cookie.
     * @return iCryptoSystem
     *
     * @throws BadMethodCallException If the crypto system has not been set.
     */
    protected function getCryptoSystem ()
    {
        if ($this->cryptoSystem === null)
            throw new \BadMethodCallException('Crypto system must be set before a cookie can be encrypted/decrypted.');

        return $this->cryptoSystem;
    }
}

CookieStorage.php

<?php
namespace Rosio\EncryptedCookie;

use Rosio\EncryptedCookie\EncryptedCookie;

class CookieStorage
{
    protected $group;

    public function __construct ($group = null)
    {
        $this->group = $group;
    }

    public function has ($name)
    {
        return isset($_COOKIE[$this->getCookieName($name)]);
    }

    public function get ($name)
    {
        return $_COOKIE[$this->getCookieName($name)];
    }

    public function set (EncryptedCookie $cookie)
    {
        setcookie($cookie->getName(), $cookie->getEncryptedData(), $cookie->getExpiration(), $cookie->getPath(), $cookie->getDomain(), $cookie->isSecure(), $cookie->isHttpOnly());
    }

    protected function getCookieName ($name)
    {
        return $this->group === null ? $name : $this->group . '_' . $name;
    }
}

CookieFactory.php

<?php

namespace Rosio\EncryptedCookie;

use Rosio\EncryptedCookie\CryptoSystem\iCryptoSystem;
use Rosio\EncryptedCookie\CookieStorage;

class CookieFactory
{
    private $cryptoSystem;
    private $cookieStorage;

    function __construct (iCryptoSystem $cryptoSystem, CookieStorage $cookieStorage = null)
    {
        $this->cryptoSystem = $cryptoSystem;

        if ($cookieStorage === null)
            $this->cookieStorage = new CookieStorage;
        else
            $this->cookieStorage = $cookieStorage;
    }

    function create ($name)
    {
        $cookie = new EncryptedCookie ($name);
        $cookie->setCryptoSystem($this->cryptoSystem);
        $cookie->setCookieStorage($this->cookieStorage);

        return $cookie;
    }

    function get ($name)
    {
        $cookie = new EncryptedCookie ($name);
        $cookie->setCryptoSystem($this->cryptoSystem);
        $cookie->setCookieStorage($this->cookieStorage);

        $cookie->load();

        return $cookie;
    }
}

CryptoSystem/iCryptoSystem.php

<?php

namespace Rosio\EncryptedCookie\CryptoSystem;

interface iCryptoSystem
{
    public function encrypt ($data, $expiration);
    public function decrypt ($data);
}

CryptoSystem/AES_SHA1.php

<?php
namespace Rosio\EncryptedCookie\CryptoSystem;

use Rosio\EncryptedCookie\Exception\RNGUnavailableException;
use Rosio\EncryptedCookie\Exception\InputTamperedException;
use Rosio\EncryptedCookie\Exception\InputExpiredException;
use Rosio\EncryptedCookie\Exception\TIDMismatchException;

class AES_SHA1 implements iCryptoSystem
{
    const IV_SIZE = 32;

    private $symmetricKey;
    private $HMACKey;

    public function __construct($symmetricKey, $HMACKey)
    {
        $this->symmetricKey = $symmetricKey;
        $this->HMACKey      = $HMACKey;
    }

    public function encrypt ($data, $expiration)
    {
        $iv = $this->getRandom(self::IV_SIZE);
        $atime = time();
        $tid = $this->getTID();

        $encData = mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $this->symmetricKey, $data, MCRYPT_MODE_CBC, $iv);

        $hmac = $this->getHMAC($encData, $atime, $expiration, $tid, $iv);

        return base64_encode($encData) . '|' . base64_encode($atime) . '|' . base64_encode($expiration) . '|' . base64_encode($tid) . '|' . base64_encode($iv) . '|' . base64_encode($hmac);
    }

    public function decrypt ($data)
    {
        list($encData, $atime, $expiration, $tid, $iv, $hmac) = array_map('base64_decode', explode('|', $data));

        if ($tid !== $this->getTID())
            throw new TIDMismatchException('The data TID no longer matches the crypto system TID.');

        $generatedHMAC = $this->getHMAC($encData, $atime, $expiration, $tid, $iv);

        if ($hmac !== $generatedHMAC)
            throw new InputTamperedException('The data HMAC no longer matches.');

        if ($expiration > 0 && $atime + $expiration < time())
            throw new InputExpiredException('The expiration time on the data has been reached.');

        $data = trim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $this->symmetricKey, $encData, MCRYPT_MODE_CBC, $iv), chr(0));

        return $data;
    }

    protected function getRandom ($length)
    {
        $wasCryptoSecure = false;
        $random = openssl_random_pseudo_bytes($length, $wasCryptoSecure);

        if ($wasCryptoSecure !== true)
            throw new RNGUnavailableException('The RNG was unable to provide truely random numbers.');

        return $random;
    }

    protected function getHMAC ($encryptedData, $aTime, $expiration, $tid, $iv)
    {
        return hash_hmac('sha1', base64_encode($encryptedData) . base64_encode($aTime) . base64_encode($expiration) . base64_encode($tid) . base64_encode($iv), $this->HMACKey, true);
    }

    function setIVSize ($size)
    {
        $this->IVSize = $size;
    }

    /**
     * Get a string which uniquely represents the algorithms and keys used to encrypt the data.
     * @return string
     */
    function getTID ()
    {
        return substr(md5(md5($this->symmetricKey) . 'AES_SHA1' . md5($this->HMACKey)), 0, 8);
    }
}

Please embed the code you'd like reviewed. It shouldn't just be behind a link. — Jamal♦, Feb 24 at 21:35
@Jamal, it's 10+ files, I'll start embedding it, but I'm not sure how practical it'd be. — Ben, Feb 24 at 21:36
You may split it up amongst different questions if necessary. — Jamal♦, Feb 24 at 21:37

Fge · Accepted Answer · 2014-02-26 14:42:45Z

Overall your design looks good for me. Nothing really bad here. Some suggestions on your architecture though:

Your cookie implementation is dependent on the CryptoSystem and the CookieStorage. But then a Cookie doesn't actually care about how it is stored or if its content is encrypted. This makes your cookie class very long though with three different responsibilities: representation of the cookie itself, storing & encryption. Furthermore, storing and encryption just delegate.

I'd consider a cookie as a simple value object which can be created everywhere (without a factory). The storage on the other hand is responsible for how the information is stored. One possibility of this how seems to be encrypted for me.

My two recommendations regarding the architecture are:

Remove storage and encryption from the cookie class
Make a cookie-storage that encrypts / decrypts on save/load

This puts each responsibility in exactly one class and slightly less coupling. Furthermore you the factory is not required any more :) .The result would end up like:

class Cookie
{
    protected $name;
    protected $data;

    protected $domain;
    protected $path;
    protected $expiration;
    protected $isSecure;
    protected $isHttpOnly;

    // getters/setters here
}

class EncryptedCookieStorage {
    public function __construct(iCryptoSystem $crypto, $group = null) {};
}

// usage
$cookie = new Cookie();
$storage->save($cookie);

Some other remarks not related to this suggestion:

I usually prefer constructor injection of required dependencies and setter injection for optional dependencies. If your class requires more then two dependencies it most likely has too many responsibilities.
I use protected only if I want subclasses to modify variables. This is almost never the case though. Someone sublcassing my class might forget some integrity checks or whatever. Subclasses should use getters/setters for this in my opinion.
I hope all your exceptions inherit from a common abstraction specific for this component.
Not sure if cookie expiration is really an exception
Do type-validation on your scalars (or where you expect scalars): CookieFactory::get(new \Exception) is probably not what you expect.

Thanks a ton for the write up. I knew the code itself wasn't bad, but I'm not particularly strong when it comes to architecture. I'll look at reworking the design before 1.0. — Ben, Mar 3 at 21:22

CodesInChaos · Answer 2 · 2014-02-28 13:54:33Z

I only looked at the crypto, not the rest of the code. I found some minor issues, but it's pretty rare to see crypto code with so few flaws.

You're using a variable time comparison to validate the MAC. So the time taken to execute the comparison depends on the number of characters that match the correct MAC. It might be possible to exploit that timing side-channel to forge a MAC.

Constant time comparisons typically look something like this:
```
$differentBits = 0;
for ($i = 0; $i < length; $i++) {
    $differentBits |= (ord($s1[$i]) ^ ord($s2[$i]));
}
return $differentBits === 0;
```
You claim to use AES-256 which has 128 bit blocks and a 256 key, but you're actually using Rijndael with 256 bit block. Either use AES (Rijndael 128), or update the documentation.
Consider using HMAC-SHA-256 (possibly with truncated output). While HMAC-SHA-1 isn't broken, I recommend going with the stronger choice.

One question I had specifically was if a truncated md5 of the secret keys was a bad idea for the TID? It feels like I shouldn't expose any part of the secret key, but I couldn't think of another way to generate a unique TID — Ben, Mar 3 at 22:13
Assuming you use proper high entropy keys, publishing their hash should be no problem if the hash is first pre-image resistant. I'd rather choose truncated SHA-2 over MD5, but even with MD5 I'm not aware of any practical attacks. — CodesInChaos, Mar 4 at 8:18

asked	4 months ago
viewed	65 times
active	4 months ago

current community

your communities

more stack exchange communities

Cookie encryption library

2 Answers

Your Answer

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

Hot Network Questions

current community

your communities

more stack exchange communities

Cookie encryption library

2 Answers

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions