Encryption functions based on mcrypt

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.