Demonstrate the bug with AES CFB 128 encryption

Demonstrate a bug with AES CFB 128 for certain key sizes introduced
with the Erlang 19.0 release. The code in the block_crypt_nif function
in the crypto.c source file incorrectly calls aes_cfb_8_crypt when the
specified cipher is aes_cfb8 or aes_cfb128 and the key size is 24 or
32. The aes_cfb_8_crypt function calls the AES_cfb8_encrypt function
from the openssl interface, but this is incorrect when the cipher is
aes_cfb128.

Unfortunately the test cases in the crypto test suite are insufficient
to detect an issue like this because it exercises the encryption and
decryption roundtrip using the same incorrect underlying function. The
problem was observed when trying to update an application to Erlang 19
that attempted to decrypt data that was encrypted using aes_cfb128 by
another source. In this commit I altered the crypto test suite to
provide a demonstration of this problem.

3 files changed, 42 insertions, 1 deletions

diff --git a/lib/crypto/c_src/crypto.c b/lib/crypto/c_src/crypto.c
index 2c8fb445dd..cd375e6d50 100644
--- a/lib/crypto/c_src/crypto.c
+++ b/lib/crypto/c_src/crypto.c
@@ -231,6 +231,7 @@ static ERL_NIF_TERM hmac_update_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM
 static ERL_NIF_TERM hmac_final_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]);
 static ERL_NIF_TERM block_crypt_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]);
 static ERL_NIF_TERM aes_cfb_8_crypt(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]);
+static ERL_NIF_TERM aes_cfb_128_crypt_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]);
 static ERL_NIF_TERM aes_ige_crypt_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]);
 static ERL_NIF_TERM aes_ctr_encrypt(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]);
 static ERL_NIF_TERM aes_ctr_stream_init(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]);
@@ -301,6 +302,7 @@ static ErlNifFunc nif_funcs[] = {
     {"hmac_final_nif", 2, hmac_final_nif},
     {"block_crypt_nif", 5, block_crypt_nif},
     {"block_crypt_nif", 4, block_crypt_nif},
+    {"aes_cfb_128_crypt_nif", 4, aes_cfb_128_crypt_nif},
     {"aes_ige_crypt_nif", 4, aes_ige_crypt_nif},
 
     {"aes_ctr_encrypt", 3, aes_ctr_encrypt},
@@ -1483,6 +1485,31 @@ static ERL_NIF_TERM aes_cfb_8_crypt(ErlNifEnv* env, int argc, const ERL_NIF_TERM
      return ret;
 }
 
+static ERL_NIF_TERM aes_cfb_128_crypt_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
+{/* (Key, IVec, Data, IsEncrypt) */
+    ErlNifBinary key, ivec, text;
+    AES_KEY aes_key;
+    unsigned char ivec_clone[16]; /* writable copy */
+    int new_ivlen = 0;
+    ERL_NIF_TERM ret;
+
+    if (!enif_inspect_iolist_as_binary(env, argv[0], &key)
+        || !(key.size == 16 || key.size == 24 || key.size == 32)
+        || !enif_inspect_binary(env, argv[1], &ivec) || ivec.size != 16
+        || !enif_inspect_iolist_as_binary(env, argv[2], &text)) {
+        return enif_make_badarg(env);
+    }
+
+    memcpy(ivec_clone, ivec.data, 16);
+    AES_set_encrypt_key(key.data, key.size * 8, &aes_key);
+    AES_cfb128_encrypt((unsigned char *) text.data,
+                       enif_make_new_binary(env, text.size, &ret),
+                       text.size, &aes_key, ivec_clone, &new_ivlen,
+                       (argv[3] != atom_true));
+    CONSUME_REDS(env,text);
+    return ret;
+}
+
 static ERL_NIF_TERM aes_ige_crypt_nif(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
 {/* (Key, IVec, Data, IsEncrypt) */
 #ifdef HAVE_AES_IGE
diff --git a/lib/crypto/src/crypto.erl b/lib/crypto/src/crypto.erl
index 696929ba4e..60e0affda0 100644
--- a/lib/crypto/src/crypto.erl
+++ b/lib/crypto/src/crypto.erl
@@ -822,6 +822,8 @@ sha_mac_96(Key, Data) -> hmac(sha, Key, Data, 12).
 block_crypt_nif(_Type, _Key, _Ivec, _Text, _IsEncrypt) -> ?nif_stub.
 block_crypt_nif(_Type, _Key, _Text, _IsEncrypt) -> ?nif_stub.
 
+aes_cfb_128_crypt_nif(_Key, _Ivec, _Text, _IsEncrypt) -> ?nif_stub.
+
 check_des3_key(Key) ->
     case lists:map(fun erlang:iolist_to_binary/1, Key) of
         ValidKey = [B1, B2, B3] when byte_size(B1) =:= 8,
@@ -915,7 +917,9 @@ blowfish_ofb64_encrypt(Key, IVec, Data) ->
 -spec aes_cfb_128_decrypt(iodata(), binary(), iodata()) -> binary().
 
 aes_cfb_128_encrypt(Key, IVec, Data) ->
-    block_encrypt(aes_cfb128, Key, IVec, Data).
+    %% block_encrypt(aes_cfb128, Key, IVec, Data).
+    aes_cfb_128_crypt_nif(Key, IVec, Data, true).
+
 
 aes_cfb_128_decrypt(Key, IVec, Data) ->
     block_decrypt(aes_cfb128, Key, IVec, Data).
diff --git a/lib/crypto/test/crypto_SUITE.erl b/lib/crypto/test/crypto_SUITE.erl
index 7b07cef33f..dbd335c693 100644
--- a/lib/crypto/test/crypto_SUITE.erl
+++ b/lib/crypto/test/crypto_SUITE.erl
@@ -358,6 +358,16 @@ block_cipher({Type, Key,  PlainText}) ->
 	    ct:fail({{crypto, block_decrypt, [Type, Key, CipherText]}, {expected, Plain}, {got, Other}})
     end;
 
+block_cipher({aes_cfb128, Key,  IV, PlainText}) ->
+    Plain = iolist_to_binary(PlainText),
+    CipherText = crypto:aes_cfb_128_encrypt(Key, IV, PlainText),
+    case crypto:block_decrypt(aes_cfb128, Key, IV, CipherText) of
+        Plain ->
+            ok;
+        Other ->
+            ct:fail({{crypto, block_decrypt, [aes_cfb128, Key, IV, CipherText]}, {expected, Plain}, {got, Other}})
+    end;
+
 block_cipher({Type, Key,  IV, PlainText}) ->
     Plain = iolist_to_binary(PlainText),
     CipherText = crypto:block_encrypt(Type, Key, IV, PlainText),