Am I using PHP Mcrypt the right way?

Question

I have written a small wrapper class for using PHP Mcrypt for encrypting and decrypting text data. This class generates a salt for hashing the key and encrypts the given data with the generated key hash. The encrypted data get stored with the initialization vector and the key salt into a container class. The container class can be decrypted when the given key is valid.

class Mcrypt
{
    /**
     * @var array
     */
    protected static $validCiphers = [
        MCRYPT_BLOWFISH,
        MCRYPT_TWOFISH,
        MCRYPT_RIJNDAEL_128,
        MCRYPT_RIJNDAEL_192,
        MCRYPT_RIJNDAEL_256,
        MCRYPT_SERPENT
    ];

    /**
     * @var string
     */
    protected $cipher = MCRYPT_TWOFISH;

    /**
     * @var string
     */
    protected $mode = MCRYPT_MODE_CBC;

    /**
     * @var string
     */
    protected $keyHashRounds = '11';

    /**
     * Encrypts the data with the given key.
     *
     * @param string $data
     * @param string $key
     * @return McryptContainer
     */
    public function encrypt($data, $key)
    {
        $data      = trim($data);
        $container = new McryptContainer;
        $container->setInitializationVector($this->getInitializationVector());
        $container->setPasswordSalt($this->generateSalt());
        $container->setCipher($this->cipher);

        $container->setData(mcrypt_encrypt(
            $this->cipher,
            $this->getKeyHash($key, $container->getPasswordSalt()),
            sha1($data) . $data,
            $this->mode,
            $container->getInitializationVector()
        ));

        return $container;
    }

    /**
     * Decrypts the container data with the given key
     * or returns false if the key is not valid.
     *
     * @param McryptContainer $container
     * @param string $key
     * @return bool|string
     */
    public function decrypt(McryptContainer $container, $key)
    {
        $data = trim(mcrypt_decrypt(
            $container->getCipher(),
            $this->getKeyHash($key, $container->getPasswordSalt()),
            $container->getData(),
            $this->mode,
            $container->getInitializationVector()
        ));

        $checkSum = substr($data, 0, 40);
        $data     = substr($data, 40);

        if (sha1($data) != $checkSum) {
            return false;
        }

        return $data;
    }

    /**
     * Generates a hash for the given key.
     *
     * @param string $key
     * @return string
     */
    protected function getKeyHash($key, $salt)
    {
        $length = mcrypt_enc_get_key_size(mcrypt_module_open($this->cipher, '', $this->mode, ''));
        $hash   = crypt($key, sprintf('$2a$%s$%s$', $this->keyHashRounds, $salt));

        return substr($hash, $length * -1);
    }

    /**
     * Generates a random salt.
     *
     * @return string
     */
    protected function generateSalt()
    {
        $length     = mcrypt_enc_get_key_size(mcrypt_module_open($this->cipher, '', $this->mode, ''));
        $validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
        $salt       = '';
        $count      = strlen($validChars) - 1;

        while ($length--) {
            $salt .= $validChars[mt_rand(0, $count)];
        }

        return $salt;
    }

    /**
     * Generates a new mcrypt initialization vector.
     *
     * @return string
     */
    protected function getInitializationVector()
    {
        return mcrypt_create_iv(mcrypt_get_iv_size($this->cipher, $this->mode), MCRYPT_DEV_URANDOM);
    }

    /**
     * Sets the cipher.
     *
     * @param string $cipher
     */
    public function setCipher($cipher)
    {
        if (!in_array($cipher, static::$validCiphers)) {
            $msg = 'Given cipher is not supported, supported ciphers are: ' . implode(', ', static::$validCiphers);
            throw new \InvalidArgumentException($msg);
        }

        $this->cipher = $cipher;
    }

    /**
     * Sets the rounds used for hashing the key.
     *
     * @param string $keyHashRounds
     */
    public function setKeyHashRounds($keyHashRounds)
    {
        $this->keyHashRounds = $keyHashRounds;
    }
}

Are there any security issues with my usage of Mcrypt?

Here is a Gist containing both classes.

Answer 1

I have been wanting to answer this for a while now, and I think I can give an answer today! So...

Am I using PHP Mcrypt the right way?

Well, what is right? Hows about, what is wrong? There's a blog I know of, which is written by a pretty well respected man on StackOverflow, and he published a post back in late 2012. It covers the basics of the most common mistakes when using Bcrypt. Let's move through each one and see how you did.

Using A Non-Random Salt

I'm looking at you generateSalt()! Unfortunately, your salt generation does a poor job. But why? Well the biggest mistake I see is your use of mt_rand().

As noted by this question regarding the state of PHP's Mersenne Twister function, it's not such a secure generator:

the chief vulnerability is that if an attacker is given a large enough sample of Mersenne Twister output, he can then predict future (and past) outputs. This is a gross violation of the properties that a cryptographically secure random number generator is supposed to have (where you're supposed to not even be able to tell if the random bit string could have been produced by the RNG in question).

So, we need to stop using mt_rand() (and don't roll back to plain ol' rand()!). What should we use instead? For now, openssl_random_pseudo_bytes() is your best bet.

From the ircmaxell's post though, it is noted that:

you should use strong randomness while not using [Cryptographically Secure randomness for salts (Such as /dev/random)]. A perfect source would be /dev/urandom. Other sources would be mcrypt_create_iv when paired with MCRYPT_DEV_URANDOM.

Talking about the source of your randomness also knocks out his topic of Using An Incorrect Random Source for Salt Generation.

How's about his third point: Using Too Weak Of A Cost Parameter? This one highly depends on your hardware. You seem to have chosen 11, which if it works for you, is fine. There's a lot to be said about choosing the right cost, and just like many things security related, it's a security vs. UX trade-off.

So onto his fourth point: Using The Wrong PHP Version. The only things you really need to know is: keep your PHP updated. Updates are meant to help you, and who doesn't like help?

The fifth point bring sup an interesting debate: Using The Wrong Prefix. First off, scratch $2a and use $2y. From the PHP docs:

developers targeting only PHP 5.3.7 and later should use "$2y$" in preference to "$2a$"

His seventh point could possibly be a game changer here: Not Using A Library. You're rolling out your own system. For what benefit? I know, I know, it's always nice to build something and know exactly how things work and you just get a sense of pride! But some things are better left for the experts. Sure it's just wrapper, it's best to just rely on someone else's trusted code.

His eighth point: Not Using A Timing Safe Comparison. This one is interesting. Basically, an attacker can determine patterns based on the time it takes to execute an algorithm. These are called timing attacks. I'm not an expert though, and I don't feel i have the knowledge to critique this aspect of your code.

And then I suggest you take a look at his ninth point, as it is pretty valuable. After you get rid of your salt generation code, this will come in handy before you add that dangerous substr function!

On top of all of hsi points, there are a few things I'd like to mention.

1. I would avoid limiting the user to only a specific list of ciphers. This little snippet may inspire you:

   if (!in_array($cipher, mcrypt_list_algorithms())) {
       throw new InvalidArgumentException('Algorithm not supported.');
   }
   

2. The same principle could be applied to the mode. Just use in_array($mode, mcrypt_list_modes()).

3. Having the default as MCRYPT_TWOFISH really isn't the best default. I would suggest MCRYPT_RIJNDAEL_256.
4. I see no benefit from sha1($data) . $data. It seems weak to sha the data like that.
5. You may want a way to store the ciphertext as a string or such.

My attempt was made at doing something similar based off your question. Only for learning purposes though. I received a well written response regarding the security. I highly you read it.