1

here is a great php class to make safe 2 way encryption in php. I am using it in a project that requires to make thousands of calls to it's methods,

i'd like to convert it into a full static class with all its methods too for performance boost and avoid new instance at every new method call

/**
 * A class to handle secure encryption and decryption of arbitrary data
 *
 * Note that this is not just straight encryption.  It also has a few other
 *  features in it to make the encrypted data far more secure.  Note that any
 *  other implementations used to decrypt data will have to do the same exact
 *  operations.  
 *
 * Security Benefits:
 *
 * - Uses Key stretching
 * - Hides the Initialization Vector
 * - Does HMAC verification of source data
 *
 */
class Encryption {

    /**
     * @var string $cipher The mcrypt cipher to use for this instance
     */
    protected $cipher = '';

    /**
     * @var int $mode The mcrypt cipher mode to use
     */
    protected $mode = '';

    /**
     * @var int $rounds The number of rounds to feed into PBKDF2 for key generation
     */
    protected $rounds = 100;

    /**
     * Constructor!
     *
     * @param string $cipher The MCRYPT_* cypher to use for this instance
     * @param int    $mode   The MCRYPT_MODE_* mode to use for this instance
     * @param int    $rounds The number of PBKDF2 rounds to do on the key
     */
    public function __construct($cipher, $mode, $rounds = 100) {
        $this->cipher = $cipher;
        $this->mode = $mode;
        $this->rounds = (int) $rounds;
    }

    /**
     * Decrypt the data with the provided key
     *
     * @param string $data The encrypted datat to decrypt
     * @param string $key  The key to use for decryption
     * 
     * @returns string|false The returned string if decryption is successful
     *                           false if it is not
     */
    public function decrypt($data, $key) {
        $salt = substr($data, 0, 128);
        $enc = substr($data, 128, -64);
        $mac = substr($data, -64);

        list ($cipherKey, $macKey, $iv) = $this->getKeys($salt, $key);

        if ($mac !== hash_hmac('sha512', $enc, $macKey, true)) {
             return false;
        }

        $dec = mcrypt_decrypt($this->cipher, $cipherKey, $enc, $this->mode, $iv);

        $data = $this->unpad($dec);

        return $data;
    }

    /**
     * Encrypt the supplied data using the supplied key
     * 
     * @param string $data The data to encrypt
     * @param string $key  The key to encrypt with
     *
     * @returns string The encrypted data
     */
    public function encrypt($data, $key) {
        $salt = mcrypt_create_iv(128, MCRYPT_DEV_URANDOM);
        list ($cipherKey, $macKey, $iv) = $this->getKeys($salt, $key);

        $data = $this->pad($data);

        $enc = mcrypt_encrypt($this->cipher, $cipherKey, $data, $this->mode, $iv);

        $mac = hash_hmac('sha512', $enc, $macKey, true);
        return $salt . $enc . $mac;
    }

    /**
     * Generates a set of keys given a random salt and a master key
     *
     * @param string $salt A random string to change the keys each encryption
     * @param string $key  The supplied key to encrypt with
     *
     * @returns array An array of keys (a cipher key, a mac key, and a IV)
     */
    protected function getKeys($salt, $key) {
        $ivSize = mcrypt_get_iv_size($this->cipher, $this->mode);
        $keySize = mcrypt_get_key_size($this->cipher, $this->mode);
        $length = 2 * $keySize + $ivSize;

        $key = $this->pbkdf2('sha512', $key, $salt, $this->rounds, $length);

        $cipherKey = substr($key, 0, $keySize);
        $macKey = substr($key, $keySize, $keySize);
        $iv = substr($key, 2 * $keySize);
        return array($cipherKey, $macKey, $iv);
    }

    /**
     * Stretch the key using the PBKDF2 algorithm
     *
     * @see http://en.wikipedia.org/wiki/PBKDF2
     *
     * @param string $algo   The algorithm to use
     * @param string $key    The key to stretch
     * @param string $salt   A random salt
     * @param int    $rounds The number of rounds to derive
     * @param int    $length The length of the output key
     *
     * @returns string The derived key.
     */
    protected function pbkdf2($algo, $key, $salt, $rounds, $length) {
        $size   = strlen(hash($algo, '', true));
        $len    = ceil($length / $size);
        $result = '';
        for ($i = 1; $i <= $len; $i++) {
            $tmp = hash_hmac($algo, $salt . pack('N', $i), $key, true);
            $res = $tmp;
            for ($j = 1; $j < $rounds; $j++) {
                 $tmp  = hash_hmac($algo, $tmp, $key, true);
                 $res ^= $tmp;
            }
            $result .= $res;
        }
        return substr($result, 0, $length);
    }

    protected function pad($data) {
        $length = mcrypt_get_block_size($this->cipher, $this->mode);
        $padAmount = $length - strlen($data) % $length;
        if ($padAmount == 0) {
            $padAmount = $length;
        }
        return $data . str_repeat(chr($padAmount), $padAmount);
    }

    protected function unpad($data) {
        $length = mcrypt_get_block_size($this->cipher, $this->mode);
        $last = ord($data[strlen($data) - 1]);
        if ($last > $length) return false;
        if (substr($data, -1 * $last) !== str_repeat(chr($last), $last)) {
            return false;
        }
        return substr($data, 0, -1 * $last);
    }
}

it's usage is

$e = new Encryption(MCRYPT_BLOWFISH, MCRYPT_MODE_CBC);
$encryptedData = $e->encrypt($data, $key);

$e2 = new Encryption(MCRYPT_BLOWFISH, MCRYPT_MODE_CBC);
$data = $e2->decrypt($encryptedData, $key);

and i would like something like

Encryption::encrypt($data, $key);
Encryption::decrypt($encryptedData, $key);

thanks.

Williem
  • 1,131
  • 1
  • 12
  • 19
  • 3
    That's nice. Good luck with the conversion. Did you have a question? – Marc B Apr 09 '15 at 19:12
  • You know you don't need a new instance with every encrypt/decrypt. Also look into dependency injection why it is a bad idea – MKroeders Apr 09 '15 at 19:17
  • 1
    Why? You never said why you want to do it... – ircmaxell Apr 10 '15 at 14:31
  • @ircmaxell it is for performance purpose as i'll make thousands calls to this class. Thus, i would not like to create new instance at each new method call. of course there is no instanciatiation needed for static classes and this should save me some microseconds on each call. – Williem Apr 10 '15 at 16:12
  • 1
    @WilliemBabalola then going static is the wrong solution. The better one would be to instantiate once, and pass that object using [dependency injection](https://www.youtube.com/watch?v=IKD2-MAkXyQ). – ircmaxell Apr 10 '15 at 16:51
  • Include the source of the code when you copy/paste it in this manner. This class was can be found posted by user @ircmaxell, in the answer here: https://stackoverflow.com/questions/5089841/two-way-encryption-i-need-to-store-passwords-that-can-be-retrieved – Goldbug Jul 18 '17 at 14:18
  • @Goldbug are you the owner of the code ? – Williem Jul 20 '17 at 18:42

2 Answers2

6

I am going to present some solutions and ideas to set you in the right direction. However, I have serious concerns about your code and must first ask that you please don't use this code in a live environment.

Unlike many cryptography researchers who will tell you "don't write crypto code" and leave it at that, I take the write crypto code, don't publish or use it path.

The fact that you asked Stack Overflow about converting your class to use static methods tells me you probably aren't yet qualified to be writing cryptography in PHP. Even if you were more familiar with the language, I would strongly caution you away from the direction you are headed.

Please use this PHP library instead. I've already reviewed it. So have others. It hasn't been formally audited yet, but that's more due to the author not having thousands of dollars to throw at a formal audit than a lack of dedication to secure and quality code.

Here are a few things I found at a glance that are dangerous for a cryptography library. This is not meant as an attack; I just want to show you some of the dangers that come in this territory.

No sanity checking in the constructor

How do you know that $this->cipher and $this->mode are valid mcrypt constants?

Wasted entropy

$salt = mcrypt_create_iv(128, MCRYPT_DEV_URANDOM);

128 bytes of entropy is 1024 bits of entropy; far more than you will ever need for AES.

Using substr() is considered harmful for crypto

PHP has this feature called mbstring.func_overload which completely changes the behavior of functions like substr(), strlen(), etc. to operate with multibyte (Unicode) logic rather than binary string logic. The typical consequence of this behavior is that it leads to an incorrect number of bytes in the resulting substring. Which is bad. How bad depends on how creative your attacker is.

In systems with this feature enabled, you have to explicitly call mb_substr($string, '8bit') to get the number of bytes in a raw binary string. However, this function is not available on all systems.

Using substr() to slice up raw binary strings with wild abandon is dangerous.

Timing Attack on the MAC verification

if ($mac !== hash_hmac('sha512', $enc, $macKey, true))

Anthony Ferrara already covered this topic in detail.

In Summary

I hope, sincerely, that this post is received with the intended tone of constructive criticism and you are enlightened about the amount of insane detail that goes into cryptography engineering.

Furthermore, I hope that this does not turn you away from learning more about the field. Please do learn more about it! It's an exciting field and everyone is better off for having more hands on deck! You can probably even one day school me about a sophisticated attack I would never imagine.

But today, right now, it is dangerous to use amateur cryptography in a production system to protect sensitive data. You owe it to yourself (or your client/employer, if applicable) to decide what matters more: experimentation, or keeping their information safe from compromise. I won't make that choice for you.

Scott Arciszewski
  • 33,610
  • 16
  • 89
  • 206
  • thanks for taking your time to give an answer, do think the [library](https://github.com/defuse/php-encryption/) you advised is ready for production environment and what about its performance is it enough good to handle thousands of calls with minimum system resources. – Williem Apr 13 '15 at 12:44
  • The encryption library should not be a bottleneck. Also, it uses openssl instead of libmcrypt, which can leverage AES-NI and perform faster AES encryption. The overhead should be minimal. – Scott Arciszewski Apr 13 '15 at 14:46
  • if `mbstring.func_overload` is enabled, converting `substr()` to `mb_substr()`, what is a good way to slice up binary strings? – Trowski Apr 13 '15 at 16:52
  • That's not quite what the feature does. That feature changes the behavior of `substr()` and forces you to use `mb_substr($string, $start, $length, '8bit')` in its stead. I've updated the answer to clarify this. – Scott Arciszewski Apr 13 '15 at 17:35
  • @Scott the generated key do not seems to be strings, how do i store it for later use when decrypting formerly encrypted data. – Williem Apr 13 '15 at 17:47
  • `bin2hex()` or `base64_encode()`, then `hex2bin()` or `base64_decode()` (respectively) when retrieving from storage. The library generates a raw binary string because it assumes immediate use. – Scott Arciszewski Apr 13 '15 at 17:53
  • So before using `substr()` to slice up binary strings, one should use `extension_loaded('mbstring')` to determine if the extension is loaded, then either use plain `substr($binaryString, $start, $length)` or `mb_substr($binaryString, $start, $length, '8bit')`? – Trowski Apr 13 '15 at 18:03
  • Just realized a better way of doing it is to probably look for the function, then define it if it doesn't exist. `if (!function_exists('mb_substr')) { function mb_substr($string, $start, $length, $encoding) { return substr($string, $start, $length); } }`. Then just use `mb_substr` everywhere. – Trowski Apr 13 '15 at 18:16
  • Or you could do something like this: https://github.com/symfony/symfony/pull/13984/files – Scott Arciszewski Apr 13 '15 at 18:59
  • Interesting points. But please can you explain the timing attack issue with that line. Anthony Ferrara specifically references the hash_* functions as one of the ways to prevent timing attacks. There is no need for a time safe string comparison function if you are comparing two hashes. Plaintext yes, but hashes wont give anything away right? – Phil May 04 '15 at 23:03
  • The reasons vary from "consistent good habits" to "defense in depth". No practical exploits have been developed that exploit that vector for bcrypt. – Scott Arciszewski May 09 '15 at 03:11
0

You're looking for static methods, simply add the static keyword before public or protected in the method declaration.

However, do note you can't call non-static methods from a static method so you will have to declare all utility methods as static as well.

See http://php.net/manual/en/language.oop5.static.php for more information.

thodic
  • 2,229
  • 1
  • 19
  • 35