current community

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

I have to protect really sensitive information and I have to do it both ways: encryption and decryption. I'll be using this PHP code:

function encrypt($mprhase) {
     $MASTERKEY = "KEY PHRASE!";
     $td = mcrypt_module_open('tripledes', '', 'ecb', '');
     $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($td), MCRYPT_RAND);
     mcrypt_generic_init($td, $MASTERKEY, $iv);
     $crypted_value = mcrypt_generic($td, $mprhase);
     mcrypt_generic_deinit($td);
     mcrypt_module_close($td);
     return base64_encode($crypted_value);
} 

function decrypt($mprhase) {
     $MASTERKEY = "KEY PHRASE!";
     $td = mcrypt_module_open('tripledes', '', 'ecb', '');
     $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($td), MCRYPT_RAND);
     mcrypt_generic_init($td, $MASTERKEY, $iv);
     $decrypted_value = mdecrypt_generic($td, base64_decode($mprhase));
     mcrypt_generic_deinit($td);
     mcrypt_module_close($td);
     return $decrypted_value;
}

How safe is it?

share|improve this question
1  
What kind of data are you encrypting? User passwords? Top secret documents? Your e-mail address? –  Simon André Forsberg Dec 20 '13 at 18:24
1  
Both ECB and TripleDES have known problems, you might want to look into other algorithms. –  grasGendarme Dec 20 '13 at 18:36
1  
I am sorry you might want to try the new Beta site Cryptography –  Malachi Dec 20 '13 at 19:43
1  
@AliAbdulkarimSalem: Just a side-note: MCRYPT_DEV_URANDOM would be considered safer than using MCRYPT_RAND, as the latter relies on the system's RNG. They're not always as random as they claim to be. I know it's a tad paranoid, but seeing as this is for the CS division, you never know... –  Elias Van Ootegem Dec 21 '13 at 16:41
1  
Do you really need to write code to encrypt your files? Couldn't you just use a utility like PGP/GnuPG? –  200_success Dec 23 '13 at 8:25
show 2 more comments

2 Answers

up vote 2 down vote accepted

In all reality the safety of the Data can be affected by any part of the transportation process, So just seeing how you encrypt and decrypt the information doesn't really help us help you in determining the safety of the data.

Nothing to do with the Security of the Functions

what you should do is create one function that will accept a parameter of encrypt or decrypt most of the information that you are using in both functions is duplicated.

function encryptOrDecrypt($mprhase, $crypt) {
     $MASTERKEY = "KEY PHRASE!";
     $td = mcrypt_module_open('tripledes', '', 'ecb', '');
     $iv = mcrypt_create_iv(mcrypt_enc_get_iv_size($td), MCRYPT_RAND);
     mcrypt_generic_init($td, $MASTERKEY, $iv);
     if ($crypt == 'encrypt')
     {
         $return_value = base64_encode(mcrypt_generic($td, $mprhase));
     }
     else
     {
         $return_value = mdecrypt_generic($td, base64_decode($mprhase));
     }
     mcrypt_generic_deinit($td);
     mcrypt_module_close($td);
     return $return_value;
}

obviously some of the variables have been changed to protect the innocent, (in other words you should change the variable $return_value to something more meaningful)

(another note: you might want to fiddle around with the input parameter that I added as well.)

Additional Important Information

As Corbin Pointed out, this should be a class/object that has methods for encrypting and deciphering and the whole works that go along with a class/object, unfortunately I don't know PHP syntax well enough to write it out.

a Function can do this, but it is sloppy coding, and I should have said this from the get go, I apologize.

I will look into the syntax when I have time or you can Visit this Fun page Or you can use ol' reliable

share|improve this answer
1  
Something about this just doesn't feel quite right to me. Having a flag change behavior is typically a major red flag that the function should be split into more than one function. I think I would prefer slightly duplicated code than a non-single-responsibility function. I also think it's dangerous to use a string flag and not check for a valid input to it. That's just waiting for some kind of misuse to fall through the cracks. –  Corbin Dec 23 '13 at 8:54
 
@Corbin, you are completely right, this should be a class/object that has methods for encrypting and deciphering and the whole works that go along with a class/object, unfortunately I don't know PHP syntax well enough to write it out. –  Malachi Dec 23 '13 at 14:39
add comment
up vote 2 down vote

As @grasGendarme says, neither the Triple-DES cipher nor the ECB cipher mode is considered to offer good security.

ECB mode is fast and easy to use, but is easy to cryptanalyze and generally not recommended. ECB mode encrypts each block independently, so if a block appears twice in the plaintext, then it will be encrypted into two identical blocks in the ciphertext. It may be acceptable to use ECB if your plaintext is short and random — for example, if you are encrypting a crypto key — and it has no predictable elements such as a standard header.

In Applied Cryptography, Second Edition, §9.11, Bruce Schneier recommends:

  • For encrypting short random data, such as other keys, ECB is OK.
  • For normal plaintext, use CBC, CFB, or OFB.
  • CBC is generally best for encrypting files.
  • CFB — specifically 8-bit CFB — is generally the mode of choice for encrypting streams, as in a link between a terminal and a host.
  • OFB/Counter is the mode of choice in an error-prone environment.

My guess is that you want CBC, but you should make that determination for yourself.

Triple-DES is considered an old standard; it has been superseded by AES. While Triple-DES hasn't been cracked, if you had to choose a cipher from scratch, you ought to pick AES.

It's a bad idea to hard-code the key in your code. Store it in a separate file, where it may be better protected by filesystem permissions and encrypted with a passphrase.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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