3
\$\begingroup\$

I want to know if these encryption functions are ok for a live production application. Based on resources here and here and here I have put together these three functions.

Purpose

This code is for a encryption and decryption system that allows a user to encrypt certain data. The encrypted data is stored, while the keys are not stored at all.

The hash function is not used for verification but rather to add 'rounds', computational work to ensure that an attacker has to run many rounds whilst brute forcing through encryption keys. In addition the hash function extends the encryption key to the length of 32 bytes for use in the encryption functions.


I'm looking for advice regarding:

  1. Are the salts ok?
  2. Is this actually proper 256 bit encryption?
  3. Have I messed up any of my hash algo's etc.?
  4. These functions will be using encryption keys from the user side. Is there anything else I need to do that I am not doing?
  5. Are encrypt256, decrypt256, and hashRound suitable for production?

Here are some notes regarding my code:

  • They are salted via the mcrypt_create_iv function.
  • hashRound is meant to slow down any brute force attacks.
  • Each round is actually 4 rounds, I guess the more hash functions the better right? In case one ever gets compromised.
  • This is meant to be 256bit encrypted and this link suggests that you are to use MCRYPT_RIJNDAEL_128 with a 256 bit key to attain that level of encryption, which is what the hash function spits out here.
  • This does confuse me a little bit, as the hash algo ripemd128 actually produces 256bit - 32byte hashes, so why is it labelled 128?

Here is the algorithm:

  • Start with the EncryptionKey and UserData.
  • The user supplies both the EncryptionKey and UserData.
  • To get (1) A 256 Bit key and (2) force a certain amount of computational work, hash the EncryptionKey X rounds through a RoundsHash function. This outputs NewEncryptionKey.
  • Now encrypt the UserData with NewEncryptionKey.
  • Save the UserData into the database.

I could skip the RoundsHash and get the same result, encrypted and decrypted data.

However, as I want to extend the EncryptionKey to precisely 32 bytes and also make any brute force attempt at decrypting the data slowly, I prefer to put the key first through the RoundsHash function.

As I do not store either EncryptionKey or NewEncryptionKey, if the RoundsHash function were to add a salt it would produce a different output (NewEncryptionKey) and I would never be able to decrypt the data.

Remember that I am only using this hash function to extend the key and add computational work, I am not verifying a password hash as I never store anything other than the UserData.


Here is the code listing:

function encrypt256($key, $input){
    $ivSize = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_128, MCRYPT_MODE_CBC);
    $iv = mcrypt_create_iv($ivSize, MCRYPT_DEV_URANDOM);
    $crypt = mcrypt_encrypt(MCRYPT_RIJNDAEL_128, $key, $input, MCRYPT_MODE_CBC, $iv);
    $encrypted = base64_encode($iv . $crypt);
    return $encrypted;
}

function decrypt256($key, $input){
    $ivSize = mcrypt_get_iv_size(MCRYPT_RIJNDAEL_128, MCRYPT_MODE_CBC);
    $input = base64_decode($input);
    $iv = substr($input, 0, $ivSize);
    $crypt = substr($input, $ivSize, strlen($input));
    $decrypted = mcrypt_decrypt(MCRYPT_RIJNDAEL_128, $key, $crypt, MCRYPT_MODE_CBC, $iv);
    return $decrypted;
}

function hashRound($rounds, $input){
    for($x = 0; $x < $rounds; $x++){
        $input = hash('sha512', hash('ripemd320', hash('whirlpool', $input)));
    }
    return hash('ripemd128', $input); // Sets the length of the key to 256 bits (32 bytes)
}

$rounds = 10000;

$testVal = "This is some test data";
$encryptionKey = "this is a Secure encryptionKey 12345 ##### 212312"; // encryptionKeys supplied by the user.
$enc = hashRound($rounds, $encryptionKey);
$mbStrlen = mb_strlen($enc);

$encrypted = encrypt256($pass, $testVal);
$decrypted = decrypt256($pass, $encrypted);

echo "<p>\$testVal: $testVal</p>";
echo "<p>\$encryptionKey: $encryptionKey</p>";
echo "<p>\$mbStrlen: $mbStrlen</p>";
echo "<p>\$encrypted: $encrypted</p>";
echo "<p>\$decrypted: $decrypted</p>";
\$\endgroup\$
11
  • 1
    \$\begingroup\$ Why implement all this just for password encryption? PHP's built-in in password_hash() and password_verify() should be strongly considered for this purpose. They represent probably the most authoritative, secure, peer-reviewed, and therefore trusted implementation for this functionality in PHP. \$\endgroup\$
    – Mike Brant
    Oct 4, 2016 at 13:26
  • \$\begingroup\$ Also, have you researched the suitability of those hashes being applied in combination and recursively? See the accepted answer to this StackOverflow question for thought stackoverflow.com/questions/348109/… \$\endgroup\$
    – Mike Brant
    Oct 4, 2016 at 13:36
  • 1
    \$\begingroup\$ @BCdotWEB No, it's not. \$\endgroup\$ Oct 4, 2016 at 13:57
  • 1
    \$\begingroup\$ @Joseph Your added explanation helps. I am really worried about your security strategy now. If you have only one "key" that you ever, and you have that key actually hard-coded in your code base, then you have a TREMENDOUS security vulnerability. With a good encryption strategy, you should be able to turn over your code to a potential hacker and still not increase their ability to decrypt your data. Also adding to confusion in question is that I can now notice your variable names are wrong in your code example and their usage doesn't seem to match your function signatures. \$\endgroup\$
    – Mike Brant
    Oct 5, 2016 at 13:55
  • 2
    \$\begingroup\$ @Joseph Based on your comments, I am guessing that your question might be closed as is. This site really tries to focus on real=world production code, not stub code, example code, etc. It might help if you provide your true application code here, so as to make this review more meaningful. \$\endgroup\$
    – Mike Brant
    Oct 5, 2016 at 15:08

1 Answer 1

1
\$\begingroup\$

tl;dr - No, they are not suitable.

I can tell you that now from experience and without even looking at the code that the answer to any question that begins "is my crypto..." will always be that it is NOT secure.

Crypto.StackExchange has a lengthy and detailed answer on why shouldn't we roll our own crypto - typically people fall into one of a handful of logical bear traps, such as believing they have some scheme that they managed to come up with something that teams of people who've studied the problems for decades have missed.

Crypto is really, really, hard and unless you know exactly what you're doing then anything security related which you have written yourself will definitely be insufficient. Think about it this way, OpenSSL contained serious flaws for decades before anyone saw them so even if there were no flaws found here in coedreview, you'd need trained professionals to spot the problem areas.

\$\endgroup\$
1
  • \$\begingroup\$ I don't touch php. So I have no idea what it offers you. The problems in this code existed before a single line of code was written \$\endgroup\$ Oct 6, 2016 at 12:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.