patternphpModerate
Encryption functions based on mcrypt
Viewed 0 times
encryptionfunctionsmcryptbased
Problem
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');- Are these safe?
- What about the IV? Is it ok to just add it to the end of the encrypted string?
- Would it be better/faster to make all this by module, that is by using
mcrypt_module_open?
Solution
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.
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.
Context
StackExchange Code Review Q#5865, answer score: 16
Revisions (0)
No revisions yet.