4
\$\begingroup\$

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?

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – lorenzo-s Sep 26 '12 at 22:30
  • \$\begingroup\$ @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. \$\endgroup\$ – luiscubal Mar 30 '14 at 15:54

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.