Please critique my use of mcrypt

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
ThePianoGuy
Forum Newbie
Posts: 5
Joined: Tue Feb 07, 2012 5:29 pm

Please critique my use of mcrypt

Post by ThePianoGuy »

Hi everyone,

I've been sifting through this forum trying to learn as much as I can about how to correctly use mcrypt. I'd appreciate comments on what I've got so far:

Code: Select all

function encrypt($decrypted, $password) { 
 $key = md5($password);
 $decrypted = rtrim($decrypted, "\0\4");
 $hash = md5($decrypted);
 $iv = mcrypt_create_iv(mcrypt_get_iv_size(MCRYPT_RIJNDAEL_256, MCRYPT_MODE_CBC), MCRYPT_RAND);
 $encrypted = trim(base64_encode(mcrypt_encrypt(MCRYPT_RIJNDAEL_256, $key, $decrypted, MCRYPT_MODE_CBC, $iv)));
 return $hash . "\n----------\n" . base64_encode($iv) . "\n----------\n" . $encrypted;
 } 

function decrypt($encrypted, $password) {
 $key = md5($password);
 list($hash, $iv, $encrypted) = explode("\n----------\n", $encrypted, 3);
 $decrypted = rtrim(mcrypt_decrypt(MCRYPT_RIJNDAEL_256, $key, base64_decode($encrypted), MCRYPT_MODE_CBC, base64_decode($iv)), "\0\4");
 if (md5($decrypted) != $hash) return false;
 return $decrypted;
 }
Questions:

1) I've read that using the password as the key is bad. Is an md5 of the password sufficient?

2) The script and the data are stored in the filesystem of the server. The password is to be memorized and entered by the operator. Should I salt the key, and if so, how and why?

3) Is there any issue with storing the iv or md5 of the data in clear text?

Thanks to anyone who can answer any of my questions or offer advice about my code to make it more secure.
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my use of mcrypt

Post by social_experiment »

ThePianoGuy wrote:1) I've read that using the password as the key is bad. Is an md5 of the password sufficient?
Not really; you are still using a value which in the event of an attack will probably be tested first rendering the hashed value useless
ThePianoGuy wrote:2) The script and the data are stored in the filesystem of the server. The password is to be memorized and entered by the operator. Should I salt the key, and if so, how and why?
Yes, but don't simply add a value to the password en then pass it to the function; Make the key a combination of the salt + password, then hash that and use it as the key. You would then store a hash which is made up of two parts; (this should probably be smaller than the keysize of the encryption algorigthm)
ThePianoGuy wrote:3) Is there any issue with storing the iv or md5 of the data in clear text?
Yeah many but the main one is that it's easier to 'read' than a hashed value.

md5 is not a safe hash anymore, if you want to hash, use atleast sha256 or higher.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
ThePianoGuy
Forum Newbie
Posts: 5
Joined: Tue Feb 07, 2012 5:29 pm

Re: Please critique my use of mcrypt

Post by ThePianoGuy »

Thanks for your reply. Pardon my n00bness. If my data were attacked, the attacker would also have access to the script, and thus the salt. So how would the hash of salt+password be more secure?

So you're saying I should replace the key with hash('sha256', 'asdf324!.#qQ' . $password)?
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my use of mcrypt

Post by social_experiment »

ThePianoGuy wrote:would also have access to the script, and thus the salt.
The hashed key is a combination of the password and a salt; if an attacker gets the salt the password is still missing; for them to retrieve the password they would have to break your hashed value or do a dictionary attack against the password
ThePianoGuy wrote:So you're saying I should replace the key with hash('sha256', 'asdf324!.#qQ' . $password)?
Yes, but make the 'asdf324!.#qQ a random salt value; you could probably make this update after each successful "login"
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
ThePianoGuy
Forum Newbie
Posts: 5
Joined: Tue Feb 07, 2012 5:29 pm

Re: Please critique my use of mcrypt

Post by ThePianoGuy »

Ok, but if I don't use a salt in preparing the hashed key, the attacker would still have to break the hashed value or do a dictionary attack against the password...no?
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my use of mcrypt

Post by social_experiment »

Yes, the salt is merely extra protection against this; storing the password hash makes it vulnerable to rainbow tables;
search.php?keywords=password+hashing
Have a look at some of these urls in the form
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
ThePianoGuy
Forum Newbie
Posts: 5
Joined: Tue Feb 07, 2012 5:29 pm

Re: Please critique my use of mcrypt

Post by ThePianoGuy »

But I'm not storing the password hash.
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: Please critique my use of mcrypt

Post by social_experiment »

:) my bad; i meant the key

Edit
Thinking about this again you probably don't want to store the key outright but store a salt value; this salt value + the password makes up the key. I'm not sure if this is what i conveyed earlier because i re-read the original post and found that i might have explained poorly
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Re: Please critique my use of mcrypt

Post by Mordred »

A few quick notes:
- Salting is basically for adding entropy to weak passwords. If you're not going to allow users to key this encryption, you can just pick a strong password and not deal with salts at all.
- There areblock cipher modes that provide encryption + authentication; I'm not sure if PHP's mcrypt supports them though
- If not, you're better off with encrypting the plaintext, then keeping a HMAC of the encrypted text, then you can check for validity of the data without decrypting it.
- Why are you doing those trims? Seems like a bug to me, they will damage the data.

On your questions:
1) In all human-password-keyed systems you'd use the password as a master key to generate other keys. What you do seems fine, provided that the password is strong enough (i.e. it's entirely under your control)
2) See above about salting. May be useful, may be not.
3) No, there is no need for the IV or the data integrity hash to be secret, but see above for hashing plaintext vs hashing the encrypted text.
ThePianoGuy
Forum Newbie
Posts: 5
Joined: Tue Feb 07, 2012 5:29 pm

Re: Please critique my use of mcrypt

Post by ThePianoGuy »

Thank you for your reply :)
Post Reply