20
\$\begingroup\$

I learned here that it is unsafe to design encryption algorithms from scratch. Given that advice, I made a pair of encryption functions based on mcrypt:

function aes128ctr_en($data,$key,$hmac = false) {
    $key = ($hmac===false) ? hash('sha256',$key,true) : hash_hmac('sha256',$key,$hmac,true);
    $data .= hash('md5',$data,true);
    $iv = mcrypt_create_iv(16,MCRYPT_DEV_URANDOM);
    return mcrypt_encrypt('rijndael-128',$key,$data,'ctr',$iv).$iv;
}

function aes128ctr_de($data,$key,$hmac = false) {
    $key = ($hmac===false) ? hash('sha256',$key,true) : hash_hmac('sha256',$key,$hmac,true);
    $iv = substr($data,-16);
    $data = substr($data,0,strlen($data)-16);
    $data = mcrypt_decrypt('rijndael-128',$key,$data,'ctr',$iv);
    $md5 = substr($data,-16);
    $data = substr($data,0,strlen($data)-16);
    return (hash('md5',$data,true)===$md5) ? $data : false;
}

$encrypted = aes128ctr_en('secret text','password');
echo aes128ctr_de($encrypted,'password');
  1. Are these safe?
  2. What about the IV? Is it ok to just add it to the end of the encrypted string?
  3. Would it be better/faster to make all this by module, that is by using mcrypt_module_open?
\$\endgroup\$

migrated from stackoverflow.com Nov 7 '11 at 22:53

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ are you encrypting something to be placed into a database? \$\endgroup\$ – dqhendricks Jan 3 '11 at 20:05
  • 5
    \$\begingroup\$ Wait. Someone advised you not do it, and you went right ahead and did anyway? - However it certainly looks complex, though I'm not sure what the point of wrapping mcrypt again is. If this is for a banking site, consult a consultant. \$\endgroup\$ – mario Jan 3 '11 at 20:10
  • 7
    \$\begingroup\$ Well, he's not designing his own crypto - he's using mcrypt_encrypt(). All he's doing is wrapping it up to make it easier to use and is asking if that wrapper leads to things being less secure. I think it's reasonable to ask if this particular treatment of IVs, hashes, and key creation is done without unintentionally opening a vulnerability. \$\endgroup\$ – Michael Burr Jan 3 '11 at 20:21
  • 2
    \$\begingroup\$ @mario: He was advised not to create an encryption algorithm by his own. This he didn't do. He just used an exisiting one. \$\endgroup\$ – Sani Singh Huttunen Jan 3 '11 at 20:21
  • 1
    \$\begingroup\$ Someone advised me not to make my own ecryption alg, here I use AES and CTR mode which I read is safer against some kind of attacks. \$\endgroup\$ – rsk82 Jan 3 '11 at 20:22
16
\$\begingroup\$

No it is not secure. For simplicity assume that your data D is exactly one block long. Then the ciphertext contains 3 16-byte blocks as follows.

C0 || C1 || C2

= D xor AES(IV) || MD5(D) xor AES(IV+1) || IV

(The IV is normally prepended to the message, but that is rather irrelevant here).

If an attacker can guess D then the attacker can derive AES(IV) and AES(IV+1) and subsequently generate another valid ciphertext by computing

D' xor AES(IV) || MD5(D') xor AES(IV+1) || IV

for whatever message D' he likes.

An unkeyed hash just does not give the integrity that you would like. As usual one should use a HMAC or something similar instead of the MD5. Hence the biggest weakness in your protocol is not the primitives that you use, but the fact that you are not using them properly. And just to answer another question posed here: People on SO are far from being competent enough to find every flaw in a new protocol. So you'd better look for an existing standard.

Let me also add, that protocols similar to the one here have been proposed in the 80s. E.g., Kerberos had a similar flaw. Better message authentications such as HMAc were designed exactly to prevent this kind of ciphertext manipulations.

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

Things that may or may not be an issue:

  1. You are not salting the passwords. If you are storing passwords for more than one user and multiple users have the same password, you will be able to look at the database and detect at a glance which users share the same password. Ideally, this should be avoided.

  2. You are not using key-strengthening. This makes it so that brute-forcing the password will be easier than brute-forcing the key itself, which means that most of your encryption algorithm is de facto weaker than claimed . I recommend bcrypt for this; I believe it is included with php 5.3+.

Note that neither of these is necessarily relevant. You have not defined why you are using encryption (i.e., what you wish to be safe from), so it is not possible to fully answer your question.

\$\endgroup\$
  • \$\begingroup\$ in php it is called simple "crypt": php.net/manual/en/function.crypt.php \$\endgroup\$ – rsk82 Jan 3 '11 at 21:34
  • \$\begingroup\$ @user393087: My main point about bcrypt was that your current hash might run too fast. \$\endgroup\$ – Brian Jan 3 '11 at 22:26
  • 3
    \$\begingroup\$ He doens't say if he's storing passwords, but if he is, he shouldn't even be encrypting them, just hashing them. \$\endgroup\$ – Chochos Jan 3 '11 at 23:52
  • \$\begingroup\$ @Chochos: That is what crypt does. \$\endgroup\$ – Brian Jan 4 '11 at 2:14
3
\$\begingroup\$

If by safe you mean that you are not deviating from standardized encryption procedures which are thought to be secure, then yes you are safe.

If by safe you mean that no one can ever crack this, then the answer is no. Given enough time and advances in computing power or cryptanalysis will eventually lead to to the cipher being broken.

It is not a requirement to keep the IV secret and it can be transferred in the clear, so yes, you can append the IV to the ciphertext.

\$\endgroup\$

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.