search

perl-openssl / perl-Crypt-OpenSSL-AES

https://metacpan.org/pod/Crypt::OpenSSL::AES
Other
2 stars 2 forks source link

undocumented backwards compatibility break since 0.02 #9

Closed maaarghk closed 1 year ago

maaarghk commented 1 year ago

hey,

in the POD you state:

The original version supports only AES 256 ECB (electronic codebook mode encryption).

this is not true. the "original" version supported 128, 192 and 256 based on the provided key length:

https://github.com/perl-openssl/perl-Crypt-OpenSSL-AES/blob/324ae34172cdf64d8616ef520643220a299bbb37/AES.xs#L45-L52

(note keysize*8 is passed)

i think for that reason it is wrong to set the cipher to AES-256-ECB when no options hash is provided. in get_cipher, in the case where name == NULL, keysize should be checked to determine the cipher.

here is an updated version of the original test file which passes on 0.02 but fails on latest:

# Before `make install' is performed this script should be runnable with
# `make test'. After `make install' it should work as `perl Crypt-OpenSSL-AES.t'

#########################

# change 'tests => 1' to 'tests => last_test_to_print';

use Test::More tests => 10;
BEGIN { use_ok('Crypt::OpenSSL::AES') };

my $key;
my $plaintext;
my $expected_enc;
my $c;

#
# test AES-256-ECB
#
$key = pack("C*",0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32
,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33);
$plaintext = pack("C*",0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44);
$expected_enc = pack("C*", 0x9b, 0xc3, 0x7f, 0x1b, 0x92, 0x93, 0xcc, 0xf9, 0x6b, 0x64, 0x00, 0xae, 0xa3, 0xc8, 0x85, 0xbb);
$c = new Crypt::OpenSSL::AES($key);

ok(($encrypted = $c->encrypt($plaintext)) eq $expected_enc, "encrypt with key length 32 (AES-256-ECB)");
ok($c->decrypt($encrypted) eq $plaintext, "decrypt with key length 32 (AES-256-ECB)");
ok($c->decrypt($c->encrypt("Hello World. 123")) eq "Hello World. 123", "Simple String Encrypted/Decrypted Successfully with key length 32 AES-256-ECB");

# echo -n "ABCDABCDABCDABCD"| openssl enc -nopad -e -aes-256-ecb -K '3031323330313233303132333031323330313233303132333031323330313233' | xxd -i
# echo -n "ABCDABCDABCDABCD"| openssl enc -nopad -e -aes-192-ecb -K '303132333031323330313233303132333031323330313233'
# echo -n "ABCDABCDABCDABCD"| openssl enc -nopad -e -aes-128-ecb -K '30313233303132333031323330313233' | xxd -i

#
# test AES-192-ECB
#
$key = pack("C*",0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33);
$plaintext = pack("C*",0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44);
$expected_enc = pack("C*", 0xd3, 0xb8, 0xa7, 0xf0, 0xaa, 0x8f, 0x62, 0xc7, 0xb8, 0x78, 0xb7, 0xb3, 0xee, 0x47, 0x6a, 0x9f);
$c = new Crypt::OpenSSL::AES($key);

ok(($encrypted = $c->encrypt($plaintext)) eq $expected_enc, "encrypt with key length 24 (AES-192-ECB)");
ok($c->decrypt($encrypted) eq $plaintext, "decrypt with key length 24 (AES-192-ECB)");
ok($c->decrypt($c->encrypt("Hello World. 123")) eq "Hello World. 123", "Simple String Encrypted/Decrypted Successfully with key length 24 AES-192-ECB");

#
# test AES-128-ECB
#
$key = pack("C*",0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33,0x30,0x31,0x32,0x33);
$plaintext = pack("C*",0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44,0x41,0x42,0x43,0x44);
$expected_enc = pack("C*", 0x6a, 0xf2, 0xaa, 0x85, 0x28, 0xfc, 0x73, 0x3c, 0xda, 0x30, 0xd4, 0x9f, 0x43, 0x5c, 0x30, 0x4e);
$c = new Crypt::OpenSSL::AES($key);

ok(($encrypted = $c->encrypt($plaintext)) eq $expected_enc, "encrypt with key length 16 (AES-128-ECB)");
ok($c->decrypt($encrypted) eq $plaintext, "decrypt with key length 16 (AES-128-ECB)");
ok($c->decrypt($c->encrypt("Hello World. 123")) eq "Hello World. 123", "Simple String Encrypted/Decrypted Successfully with key length 16 AES-128-ECB");

cheers.

M

timlegge commented 1 year ago

You make a great point. I will fix it some time this week. Thanks for the test. One of the problems updating this module was that the tests were a little sparse so it was difficult to be sure I was not breaking something.

timlegge commented 1 year ago

This is a first stab at it. Once I looked at it I noticed that I was also assuming that the user would match the cipher to the key size. I may change it again to have the perl code remove the size from the cipher being passed in.

I noticed that I had also used a test with a mismatched cipher and a key that I had to change.

Tim

diff --git a/AES.xs b/AES.xs
index 2769d06..3db13f5 100644
--- a/AES.xs
+++ b/AES.xs
@@ -61,38 +61,42 @@ char * get_option_svalue (pTHX_ HV * options, char * name) {

 #if OPENSSL_VERSION_NUMBER >= 0x00908000L
 #ifdef LIBRESSL_VERSION_NUMBER
-const EVP_CIPHER * get_cipher(pTHX_ HV * options) {
+const EVP_CIPHER * get_cipher(pTHX_ HV * options, STRLEN keysize) {
 #else
-EVP_CIPHER * get_cipher(pTHX_ HV * options) {
+EVP_CIPHER * get_cipher(pTHX_ HV * options, STRLEN keysize) {
 #endif
     char * name = get_option_svalue(aTHX_ options, "cipher");

-    if (name == NULL)
+    if (name == NULL && keysize == 16)
+        return (EVP_CIPHER * ) EVP_aes_128_ecb();
+    else if (name == NULL && keysize == 24)
+        return (EVP_CIPHER * ) EVP_aes_192_ecb();
+    else if (name == NULL && keysize == 32)
         return (EVP_CIPHER * ) EVP_aes_256_ecb();
-    else if (strcmp(name, "AES-128-ECB") == 0)
+    else if (strcmp(name, "AES-128-ECB") == 0 && keysize == 16)
         return (EVP_CIPHER * ) EVP_aes_128_ecb();
-    else if (strcmp(name, "AES-192-ECB") == 0)
+    else if (strcmp(name, "AES-192-ECB") == 0 && keysize == 24)
         return (EVP_CIPHER * ) EVP_aes_192_ecb();
-    else if (strcmp(name, "AES-256-ECB") == 0)
+    else if (strcmp(name, "AES-256-ECB") == 0 && keysize == 32)
         return (EVP_CIPHER * ) EVP_aes_256_ecb();
-    else if (strcmp(name, "AES-128-CBC") == 0)
+    else if (strcmp(name, "AES-128-CBC") == 0 && keysize == 16)
         return (EVP_CIPHER * ) EVP_aes_128_cbc();
-    else if (strcmp(name, "AES-192-CBC") == 0)
+    else if (strcmp(name, "AES-192-CBC") == 0 && keysize == 24)
         return (EVP_CIPHER * ) EVP_aes_192_cbc();
-    else if (strcmp(name, "AES-256-CBC") == 0)
+    else if (strcmp(name, "AES-256-CBC") == 0 && keysize == 32)
         return (EVP_CIPHER * ) EVP_aes_256_cbc();
-    else if (strcmp(name, "AES-128-CFB") == 0)
+    else if (strcmp(name, "AES-128-CFB") == 0 && keysize == 16)
         return (EVP_CIPHER * ) EVP_aes_128_cfb();
-    else if (strcmp(name, "AES-192-CFB") == 0)
+    else if (strcmp(name, "AES-192-CFB") == 0 && keysize == 24)
         return (EVP_CIPHER * ) EVP_aes_192_cfb();
-    else if (strcmp(name, "AES-256-CFB") == 0)
+    else if (strcmp(name, "AES-256-CFB") == 0 && keysize == 32)
         return (EVP_CIPHER * ) EVP_aes_256_cfb();
 #if OPENSSL_VERSION_NUMBER >=  0x10001000L
-    else if (strcmp(name, "AES-128-CTR") == 0)
+    else if (strcmp(name, "AES-128-CTR") == 0 && keysize == 16)
         return (EVP_CIPHER * ) EVP_aes_128_ctr();
-    else if (strcmp(name, "AES-192-CTR") == 0)
+    else if (strcmp(name, "AES-192-CTR") == 0 && keysize == 24)
         return (EVP_CIPHER * ) EVP_aes_192_ctr();
-    else if (strcmp(name, "AES-256-CTR") == 0)
+    else if (strcmp(name, "AES-256-CTR") == 0 && keysize == 32)
         return (EVP_CIPHER * ) EVP_aes_256_ctr();
 #else
     else if (
@@ -101,21 +105,28 @@ EVP_CIPHER * get_cipher(pTHX_ HV * options) {
         (strcmp(name, "AES-256-CTR") == 0))
         croak ("CTR ciphers not supported on this version of OpenSSL");
 #endif
-    else if (strcmp(name, "AES-128-OFB") == 0)
+    else if (strcmp(name, "AES-128-OFB") == 0 && keysize == 16)
         return (EVP_CIPHER * ) EVP_aes_128_ofb();
-    else if (strcmp(name, "AES-192-OFB") == 0)
+    else if (strcmp(name, "AES-192-OFB") == 0 && keysize == 24)
         return (EVP_CIPHER * ) EVP_aes_192_ofb();
-    else if (strcmp(name, "AES-256-OFB") == 0)
+    else if (strcmp(name, "AES-256-OFB") == 0 && keysize == 32)
         return (EVP_CIPHER * ) EVP_aes_256_ofb();
     else
         croak ("You specified an unsupported cipher");
 }
 #endif

-char * get_cipher_name (pTHX_ HV * options) {
+char * get_cipher_name (pTHX_ HV * options, long long keysize) {
     char * value = get_option_svalue(aTHX_ options, "cipher");
     if (value == NULL)
-        return "AES-256-ECB";
+        if (keysize == 16)
+            return "AES-128-ECB";
+        else if (keysize == 24)
+            return "AES-192-ECB";
+        else if (keysize == 32)
+            return "AES-256-ECB";
+        else
+            croak ("get_cipher_name - Unsupported Key Size");

     return value;
 }
@@ -182,9 +193,9 @@ CODE:
         Newz(0, RETVAL, 1, struct state);
         RETVAL->padding = get_padding(aTHX_ options);
 #if OPENSSL_VERSION_NUMBER >= 0x00908000L
-        cipher = get_cipher(aTHX_ options);
+        cipher = get_cipher(aTHX_ options, keysize);
         iv = get_iv(aTHX_ options);
-        cipher_name = get_cipher_name(aTHX_ options);
+        cipher_name = get_cipher_name(aTHX_ options, keysize);
         if ((strcmp(cipher_name, "AES-128-ECB") == 0 ||
             strcmp(cipher_name, "AES-192-ECB") == 0 ||
             strcmp(cipher_name, "AES-256-ECB") == 0)
diff --git a/t/02-algorithms.t b/t/02-algorithms.t
index aec72ca..1b2c3b0 100644
--- a/t/02-algorithms.t
+++ b/t/02-algorithms.t
@@ -65,9 +65,9 @@ unlike ($@, qr/AES: Data size must be multiple of blocksize/, "Padding and data
 {
     eval {
         $c = Crypt::OpenSSL::AES->new(pack("H*", $key),
-            { cipher => "AES-192-ECB", iv => pack("H*", substr($iv, 0, 32)), });
+            { cipher => "AES-256-ECB", iv => pack("H*", substr($iv, 0, 32)), });
     };
-    like ($@, qr/AES-192-ECB does not use IV/, "AES-192-ECB does not use IV");
+    like ($@, qr/AES-256-ECB does not use IV/, "AES-256-ECB does not use IV");
 }

 {