Am I using crypt and mcrypt well in this simple encryption class?

Question

These days I read a lot here on SO about password hashing and data encryption. It's a real mess, I mean, deciding what the best practice is. I need a very simple class that can be reused anywhere and that provide a decent-but-not-paranoic security level for my PHP applications (I do not handle bank data). Additionally, I want to rely as much as possible on PHP standard libs. I came up with this:

class Security {

    public static function hashPassword($plain) {
        $salt = md5(rand(0, 1023) . '@' . time()); // Random salt
        return crypt($plain, '$2a$07$' . $salt); // '$2a$07$' is the Blowfish trigger
    }

    public static function checkPassword($plain, $hash) {
        return (crypt($plain, $hash) === $hash);
    }

    public static function generateIv() {
        $iv_size = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_CBC); // It's 32
        return mcrypt_create_iv($iv_size, MCRYPT_RAND);
    }

    public static function encrypt($key, $data, $iv = null, $base64 = true) {
        if (is_null($iv)) $iv = md5($key);
        $ret = mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $key, $data, MCRYPT_MODE_CBC, $iv);
        return ($base64 ? base64_encode($ret) : $ret);
    }

    public static function decrypt($key, $data, $iv = null, $base64 = true) {
        if (is_null($iv)) $iv = md5($key);
        return rtrim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $key, $base64 ? base64_decode($data) : $data, MCRYPT_MODE_CBC, $iv), "\0");
    }

}

As you can see, I choose to hash passwords with crypt() using Blowfish hashing algorithm. The return value of hashPassword() is the salt + hash that then I store in the DB. I made this choice because crypt() is available on every server, provides a confortable way to check hash regardless of algorithm used (it's based on salt prefix) and, I read, bcrypt is a decent hashing method.

Then, for data encryption I used mcrypt() Rijndael 256 algorithm with CBC mode. As you can see, I can use encryption methods in two way. I can pass a IV (and generateIv() helps me to create one) that I will store in the DB along crypted data, or, if I don't, a basic IV is derived from key in both crypt and decrypt process.

What do you think about it? Am I missing something? Can I be finally relaxed about hashing and encryption in my PHP applications?

Answer 1

Your salt generation mechanism does not seem sufficiently random. You will end up with a very small set of salts.

First of all, time() is not random at all. In second place, rand(0, 1023) is not only a relatively weak random number generator, but also generates too few bits to be enough.

All this - combined with the use of the relatively unsafe md5 algorithm - means that there's a higher-than-ideal risk of salt collision.

Will all of this still cause trouble? Frankly, I don't know. This is definitely better than storing passwords in plain text, or using unsalted hashes, but it's not perfect. Generally, I'm nervous about using not-exactly-random data for security algorithms that are meant to use random data.

For a starting point, try to look at this stackoverflow question. This is what the author does for salt generation:

1. Use OpenSSL random pseudo bytes, if available. Using a security library to get random data for use in another security function makes me far less nervous than ordinary rand().
2. Try a *NIX-specific method
3. If not possible, then use a mix of md5, microtime(better than time since it changes more often) and even the PID.

In particular, note than PHP's md5 normally returns a hexadecimal string with a 32-byte length, but bcrypt expects a string with a length of 22, so that might cause troubles (namely, the salt will not be as secure as it could be)

Also, in your case this won't be a problem, but $2a is ambiguous (but sadly, the correct, unambiguous way might be incompatible with older PHP versions). If you can, consider using $2y instead and see if that works for you.

The PHP documentation recommends using mcrypt_enc_get_iv_size instead of mcrypt_get_iv_size and using that in combination with mcrypt_module_open (although strangely, the documentation for mcrypt_encrypt does use mcrypt_get_iv_size)

Again, you are using not-really-random data for a function that seems to expect random data (specifically, the IV when the supplied IV is "null"). This makes me nervous.