Fix AES-CTR leftover data issue.

A customer discovered a problem with our AES-CTR implementation where partial
block data from a prior AES-CTR operation was being mixed in with a subsequent
operation's input data, even when the IV was being changed between. This is a
known problem with wc_AesSetIV, where it doesn't clear the `left` field. This
commit makes sure we clear `left` on init calls, among some other changes:

- Add always call init flag.
- In init, call wc_AesSetIV when IV isn't NULL. When setting key, ensure that we
don't wipe out a previously set IV with wc_AesSetKey. This change allows us to
remove the wc_AesSetIV call in we_aes_ctr_cipher.
- we_aes_ctr_cipher should return 0 when in == NULL.

Author: haydenroche5

src/we_aes_ctr.c

@@ -32,8 +32,8 @@
  */
 typedef struct we_AesCtr
 {
-    /** The wolfSSL AES data object. */
-    Aes            aes;
+    /* The wolfSSL AES data object. */
+    Aes aes;
 } we_AesCtr;
 
 
@@ -52,6 +52,7 @@ static int we_aes_ctr_init(EVP_CIPHER_CTX *ctx, const unsigned char *key,
     int ret = 1;
     int rc;
     we_AesCtr *aes;
+    const unsigned char* tmpIv = iv;
 
     WOLFENGINE_ENTER(WE_LOG_CIPHER, "we_aes_ctr_init");
     WOLFENGINE_MSG_VERBOSE(WE_LOG_CIPHER, "ARGS [ctx = %p, key = %p, iv = %p, "
@@ -66,23 +67,40 @@ static int we_aes_ctr_init(EVP_CIPHER_CTX *ctx, const unsigned char *key,
         ret = 0;
     }
 
-    if ((ret == 1) && (key != NULL)) {
+    if (ret == 1) {
         rc = wc_AesInit(&aes->aes, NULL, INVALID_DEVID);
         if (rc != 0) {
             WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesInit", rc);
             ret = 0;
         }
     }
-
     if ((ret == 1) && (key != NULL)) {
-        /* No decryption for CTR. */
-        rc = wc_AesSetKey(&aes->aes, key, EVP_CIPHER_CTX_key_length(ctx), iv,
+        if (tmpIv == NULL) {
+            /* If no IV given, attempt to use previously set ctx IV. */
+            tmpIv = EVP_CIPHER_CTX_iv_noconst(ctx);
+        }
+        /* No decryption for CTR (hence AES_ENCRYPTION). */
+        rc = wc_AesSetKey(&aes->aes, key, EVP_CIPHER_CTX_key_length(ctx), tmpIv,
                           AES_ENCRYPTION);
         if (rc != 0) {
             WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesSetKey", rc);
             ret = 0;
         }
     }
+    if ((ret == 1) && (iv != NULL)) {
+        rc = wc_AesSetIV(&aes->aes, iv);
+        if (rc != 0) {
+            WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesSetIV", rc);
+            ret = -1;
+        }
+        else {
+            /* 
+             * wc_AesSetIV should clear this field, but it doesn't in some
+             * wolfSSL versions.
+             */
+            aes->aes.left = 0;
+        }
+    }
 
     WOLFENGINE_LEAVE(WE_LOG_CIPHER, "we_aes_ctr_init", ret);
 
@@ -99,7 +117,7 @@ static int we_aes_ctr_init(EVP_CIPHER_CTX *ctx, const unsigned char *key,
  * @param  in   [in]      Data to encrypt/decrypt.
  * @param  len  [in]      Length of data to encrypt/decrypt.
  * @return  -1 on failure.
- * @return  1 on success.
+ * @return  Length of output data on success.
  */
 static int we_aes_ctr_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
                              const unsigned char *in, size_t len)
@@ -121,26 +139,27 @@ static int we_aes_ctr_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
     }
 
     if (ret == 1) {
-        rc = wc_AesSetIV(&aes->aes, EVP_CIPHER_CTX_iv_noconst(ctx));
-        if (rc != 0) {
-            WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesSetIV", rc);
-            ret = -1;
+        if (in != NULL && len > 0) {
+            rc = wc_AesCtrEncrypt(&aes->aes, out, in, (word32)len);
+            if (rc != 0) {
+                WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesCtrEncrypt", rc);
+                ret = -1;
+            }
+            else {
+                unsigned int num = EVP_CIPHER_CTX_num(ctx);
+                num = (num + len) % AES_128_KEY_SIZE;
+                EVP_CIPHER_CTX_set_num(ctx, num);
+
+                XMEMCPY(EVP_CIPHER_CTX_iv_noconst(ctx), aes->aes.reg,
+                        AES_BLOCK_SIZE);
+
+                ret = (int)len;
+            }
         }
-    }
-    if (ret == 1) {
-        rc = wc_AesCtrEncrypt(&aes->aes, out, in, (word32)len);
-        if (rc != 0) {
-            WOLFENGINE_ERROR_FUNC(WE_LOG_CIPHER, "wc_AesCtrEncrypt", rc);
-            ret = -1;
+        else if (in == NULL)  {
+            ret = 0;
         }
     }
-    if (ret == 1) {
-        unsigned int num = EVP_CIPHER_CTX_num(ctx);
-        num = (num + len) % AES_128_KEY_SIZE;
-        EVP_CIPHER_CTX_set_num(ctx, num);
-
-        XMEMCPY(EVP_CIPHER_CTX_iv_noconst(ctx), aes->aes.reg, AES_BLOCK_SIZE);
-    }
 
     WOLFENGINE_LEAVE(WE_LOG_CIPHER, "we_aes_ctr_cipher", ret);
 
@@ -196,7 +215,9 @@ static int we_aes_ctr_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, void *ptr)
 
 /** Flags for AES-CTR method. */
 #define AES_CTR_FLAGS              \
-    (EVP_CIPH_FLAG_DEFAULT_ASN1  | \
+    (EVP_CIPH_ALWAYS_CALL_INIT   | \
+     EVP_CIPH_FLAG_CUSTOM_CIPHER | \
+     EVP_CIPH_FLAG_DEFAULT_ASN1  | \
      EVP_CIPH_CTR_MODE)
 
 /** AES128-CTR EVP cipher method. */

test/test_cipher.c

@@ -514,11 +514,134 @@ int test_aes256_ctr_stream(ENGINE *e, void *data)
     return err;
 }
 
-/* OpenSSL allows the user to call EVP_CipherInit with NULL key or IV. In the
+/*
+ * This test exercises a bug in some versions of wolfSSL where partial block
+ * data (i.e. data that didn't align to the AES block size of 16 bytes) from a
+ * previous AES-CTR operation would be mixed in with the next operation's input
+ * data, even when the IV was changed in between. When the key or IV changes,
+ * any partial block data should not be used.
+ */
+int test_aes_ctr_leftover_data_regression(ENGINE *e, void *data)
+{
+    enum {
+        NUM_PACKETS = 2,
+        PACKET_SIZE = AES_BLOCK_SIZE + 1
+    };
+    int err = 0;
+    const unsigned char key[AES_128_KEY_SIZE] = {
+        0x37, 0xe8, 0x46, 0x48, 0xf4, 0xd6, 0xa7, 0x28, 0xc6, 0xd5, 0x2e, 0x3b,
+        0xf4, 0xc4, 0x46, 0x66
+    };
+    const unsigned char ivs[NUM_PACKETS][AES_BLOCK_SIZE] = {
+        {
+            0x88, 0x93, 0x6f, 0xfd, 0x7d, 0x94, 0x21, 0xcc, 0x40, 0x64, 0xde,
+            0x8a, 0xb9, 0xaf, 0xdd, 0xe4
+        },
+        {
+            0x0f, 0x10, 0x9c, 0xc9, 0x25, 0x9a, 0x53, 0xf0, 0xd3, 0x92, 0xdf,
+            0x35, 0xb2, 0x35, 0xa6, 0xd8
+        }
+    };
+    const unsigned char packets[NUM_PACKETS][PACKET_SIZE] = {
+        {
+            0x3c, 0x89, 0x18, 0x76, 0xfc, 0xae, 0xdc, 0xee, 0xab, 0xf2, 0xf7,
+            0x56, 0x47, 0x1f, 0xe9, 0x20, 0x67
+        },
+        {
+            0xb5, 0x3b, 0x8d, 0xa1, 0xe7, 0x0a, 0x46, 0x56, 0xf6, 0xfd, 0xeb,
+            0x85, 0x61, 0xd8, 0xaf, 0xac, 0xfb
+        }
+    };
+    EVP_CIPHER_CTX* encCtx = NULL;
+    EVP_CIPHER_CTX* decCtx = NULL;
+    unsigned char encText[PACKET_SIZE];
+    unsigned char decText[PACKET_SIZE];
+    int i;
+
+    (void)data;
+
+    if (err == 0) {
+        err = (encCtx = EVP_CIPHER_CTX_new()) == NULL;
+    }
+    if (err == 0) {
+        err = (decCtx = EVP_CIPHER_CTX_new()) == NULL;
+    }
+
+    /* Set key. */
+    if (err == 0) {
+        err = EVP_CipherInit_ex(encCtx, EVP_aes_128_ctr(), NULL, key,
+                                NULL, -1) != 1;
+    }
+    if (err == 0) {
+        err = EVP_CipherInit_ex(decCtx, EVP_aes_128_ctr(), e, key,
+                                NULL, -1) != 1;
+    }
+
+    for (i = 0; err == 0 && i < NUM_PACKETS; ++ i) {
+        /* Set IV. */
+        if (err == 0) {
+            err = EVP_CipherInit_ex(encCtx, NULL, NULL, NULL, ivs[i], 1) != 1;
+        }
+        if (err == 0) {
+            err = EVP_CipherInit_ex(decCtx, NULL, e, NULL, ivs[i], 0) != 1;
+        }
+        /* Encrypt. */
+        if (err == 0) {
+            err = EVP_Cipher(encCtx, encText, packets[i], PACKET_SIZE) < 0;
+        }
+        /* Decrypt. */
+        if (err == 0) {
+            err = EVP_Cipher(decCtx, decText, encText, PACKET_SIZE) < 0;
+        }
+        /* Ensure decrypted and plaintext match. */
+        if (err == 0) {
+            err = memcmp(decText, packets[i], PACKET_SIZE) != 0;
+        }
+    }
+
+    /* Try the other way, now. Encrypt with wolfEngine, decrypt with wolfSSL. */
+    if (err == 0) {
+        err = EVP_CipherInit_ex(encCtx, EVP_aes_128_ctr(), e, key,
+                                NULL, -1) != 1;
+    }
+    if (err == 0) {
+        err = EVP_CipherInit_ex(decCtx, EVP_aes_128_ctr(), NULL, key,
+                                NULL, -1) != 1;
+    }
+
+    for (i = 0; err == 0 && i < NUM_PACKETS; ++ i) {
+        if (err == 0) {
+            err = EVP_CipherInit_ex(encCtx, NULL, e, NULL, ivs[i], 1) != 1;
+        }
+        if (err == 0) {
+            err = EVP_CipherInit_ex(decCtx, NULL, NULL, NULL, ivs[i], 0) != 1;
+        }
+        if (err == 0) {
+            err = EVP_Cipher(encCtx, encText, packets[i], PACKET_SIZE) < 0;
+        }
+        if (err == 0) {
+            err = EVP_Cipher(decCtx, decText, encText, PACKET_SIZE) < 0;
+        }
+        if (err == 0) {
+            err = memcmp(decText, packets[i], PACKET_SIZE) != 0;
+        }
+    }
+
+    if (encCtx != NULL)
+        EVP_CIPHER_CTX_free(encCtx);
+    if (decCtx != NULL)
+        EVP_CIPHER_CTX_free(decCtx);
+
+    return err;
+}
+
+/*
+ * OpenSSL allows the user to call EVP_CipherInit with NULL key or IV. In the
  * past, setting the IV first (with key NULL) with wolfEngine and then setting
  * the key (with IV NULL) would result in the IV getting set to 0s on the call
  * to set the key. This was discovered in testing with OpenSSH. This is a
- * regression test to ensure we preserve the IV in this scenario. */
+ * regression test to ensure we preserve the IV in this scenario.
+ */
 int test_aes_ctr_iv_init_regression(ENGINE *e, void *data)
 {
     int err = 0;

test/unit.c

@@ -117,6 +117,7 @@ TEST_CASE test_case[] = {
     TEST_DECL(test_aes128_ctr_stream, NULL),
     TEST_DECL(test_aes192_ctr_stream, NULL),
     TEST_DECL(test_aes256_ctr_stream, NULL),
+    TEST_DECL(test_aes_ctr_leftover_data_regression, NULL),
     TEST_DECL(test_aes_ctr_iv_init_regression, NULL),
 #endif
 #ifdef WE_HAVE_AESGCM

test/unit.h

@@ -157,7 +157,8 @@ int test_aes128_ctr_stream(ENGINE *e, void *data);
 int test_aes192_ctr_stream(ENGINE *e, void *data);
 int test_aes256_ctr_stream(ENGINE *e, void *data);
 
-int test_aes_ctr_iv_init_regression(ENGINE *, void *);
+int test_aes_ctr_leftover_data_regression(ENGINE *e, void *data);
+int test_aes_ctr_iv_init_regression(ENGINE *e, void *data);
 
 #endif