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

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? 1 Answer 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.

• Thank you @luiscubal for your remarks. It's all little changes I can do without class skeleton modifications and, most important, without break compatibility for already-used cases (that means, ok, salts and IVs will be better than previous from now, but old stuff will continue to work with updated class too). I will try to use mt_rand() with no args (that means, 0 to 2^31 on 32 bits systems) instead of rand(0, 1023) and microtime() instead of time. I think that will be sufficient. – lorenzo-s Sep 26 '12 at 22:30
• @lorenzo-s Please consider using SSL random pseudo bytes, if available, or reading from /dev/urandom. Rely on the other options (mt_rand and microtime) only if really necessary. Better safe than sorry. – luiscubal Mar 30 '14 at 15:54