aes: Add fault injection checks when writing key to hardware

Vulnerability reported by LimitedResults under Espressif Bug Bounty Program.

diff --git a/components/esp32/hwcrypto/aes.c b/components/esp32/hwcrypto/aes.c
index a3fc95910ff11558d84c601b0214657134fe0072..3bf90a9bc41c73c99b46f900782a61f293c65a9a 100644 (file)
--- a/components/esp32/hwcrypto/aes.c
+++ b/components/esp32/hwcrypto/aes.c
@@ -50,6 +50,11 @@
 */
 static portMUX_TYPE aes_spinlock = portMUX_INITIALIZER_UNLOCKED;
 
+static inline bool valid_key_length(const esp_aes_context *ctx)
+{
+    return ctx->key_bytes == 128/8 || ctx->key_bytes == 192/8 || ctx->key_bytes == 256/8;
+}
+
 void esp_aes_acquire_hardware( void )
 {
     portENTER_CRITICAL(&aes_spinlock);
@@ -94,6 +99,7 @@ int esp_aes_setkey( esp_aes_context *ctx, const unsigned char *key,
     }
     ctx->key_bytes = keybits / 8;
     memcpy(ctx->key, key, ctx->key_bytes);
+    ctx->key_in_hardware = 0;
     return 0;
 }
 
@@ -103,28 +109,47 @@ int esp_aes_setkey( esp_aes_context *ctx, const unsigned char *key,
  *
  * Call only while holding esp_aes_acquire_hardware().
  */
-static inline void esp_aes_setkey_hardware( esp_aes_context *ctx, int mode)
+static void esp_aes_setkey_hardware(esp_aes_context *ctx, int mode)
 {
     const uint32_t MODE_DECRYPT_BIT = 4;
     unsigned mode_reg_base = (mode == ESP_AES_ENCRYPT) ? 0 : MODE_DECRYPT_BIT;
 
+    ctx->key_in_hardware = 0;
+
     for (int i = 0; i < ctx->key_bytes/4; ++i) {
         DPORT_REG_WRITE(AES_KEY_BASE + i * 4, *(((uint32_t *)ctx->key) + i));
+        ctx->key_in_hardware += 4;
     }
 
     DPORT_REG_WRITE(AES_MODE_REG, mode_reg_base + ((ctx->key_bytes / 8) - 2));
+
+    /* Fault injection check: all words of key data should have been written to hardware */
+    if (ctx->key_in_hardware < 16
+        || ctx->key_in_hardware != ctx->key_bytes) {
+        abort();
+    }
 }
 
 /* Run a single 16 byte block of AES, using the hardware engine.
  *
  * Call only while holding esp_aes_acquire_hardware().
  */
-static void esp_aes_block(const void *input, void *output)
+static int esp_aes_block(esp_aes_context *ctx, const void *input, void *output)
 {
     const uint32_t *input_words = (const uint32_t *)input;
     uint32_t i0, i1, i2, i3;
     uint32_t *output_words = (uint32_t *)output;
 
+    /* If no key is written to hardware yet, either the user hasn't called
+       mbedtls_aes_setkey_enc/mbedtls_aes_setkey_dec - meaning we also don't
+       know which mode to use - or a fault skipped the
+       key write to hardware. Treat this as a fatal error and zero the output block.
+    */
+    if (ctx->key_in_hardware != ctx->key_bytes) {
+        bzero(output, 16);
+        return MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH;
+    }
+
     /* Storing i0,i1,i2,i3 in registers not an array
        helps a lot with optimisations at -Os level */
     i0 = input_words[0];
@@ -153,11 +178,14 @@ static void esp_aes_block(const void *input, void *output)
        Bypassing this check requires at least one additional fault.
     */
     if(i0 == output_words[0] && i1 == output_words[1] && i2 == output_words[2] && i3 == output_words[3]) {
-        // calling two zeroing functions to narrow the
-        // window for a double-fault here
+        // calling zeroing functions to narrow the
+        // window for a double-fault of the abort step, here
         memset(output, 0, 16);
         mbedtls_platform_zeroize(output, 16);
+        abort();
     }
+
+    return 0;
 }
 
 /*
@@ -167,11 +195,18 @@ int esp_internal_aes_encrypt( esp_aes_context *ctx,
                       const unsigned char input[16],
                       unsigned char output[16] )
 {
+    int r;
+
+    if (!valid_key_length(ctx)) {
+        return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
+    }
+
     esp_aes_acquire_hardware();
+    ctx->key_in_hardware = 0;
     esp_aes_setkey_hardware(ctx, ESP_AES_ENCRYPT);
-    esp_aes_block(input, output);
+    r = esp_aes_block(ctx, input, output);
     esp_aes_release_hardware();
-    return 0;
+    return r;
 }
 
 void esp_aes_encrypt( esp_aes_context *ctx,
@@ -189,11 +224,18 @@ int esp_internal_aes_decrypt( esp_aes_context *ctx,
                       const unsigned char input[16],
                       unsigned char output[16] )
 {
+    int r;
+
+    if (!valid_key_length(ctx)) {
+        return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
+    }
+
     esp_aes_acquire_hardware();
+    ctx->key_in_hardware = 0;
     esp_aes_setkey_hardware(ctx, ESP_AES_DECRYPT);
-    esp_aes_block(input, output);
+    r = esp_aes_block(ctx, input, output);
     esp_aes_release_hardware();
-    return 0;
+    return r;
 }
 
 void esp_aes_decrypt( esp_aes_context *ctx,
@@ -203,7 +245,6 @@ void esp_aes_decrypt( esp_aes_context *ctx,
     esp_internal_aes_decrypt(ctx, input, output);
 }
 
-
 /*
  * AES-ECB block encryption/decryption
  */
@@ -212,12 +253,19 @@ int esp_aes_crypt_ecb( esp_aes_context *ctx,
                        const unsigned char input[16],
                        unsigned char output[16] )
 {
+    int r;
+
+    if (!valid_key_length(ctx)) {
+        return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
+    }
+
     esp_aes_acquire_hardware();
+    ctx->key_in_hardware = 0;
     esp_aes_setkey_hardware(ctx, mode);
-    esp_aes_block(input, output);
+    r = esp_aes_block(ctx, input, output);
     esp_aes_release_hardware();
 
-    return 0;
+    return r;
 }
 
 
@@ -241,14 +289,19 @@ int esp_aes_crypt_cbc( esp_aes_context *ctx,
         return ( ERR_ESP_AES_INVALID_INPUT_LENGTH );
     }
 
+    if (!valid_key_length(ctx)) {
+        return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
+    }
+
     esp_aes_acquire_hardware();
+    ctx->key_in_hardware = 0;
 
     esp_aes_setkey_hardware(ctx, mode);
 
     if ( mode == ESP_AES_DECRYPT ) {
         while ( length > 0 ) {
             memcpy(temp, input_words, 16);
-            esp_aes_block(input_words, output_words);
+            esp_aes_block(ctx, input_words, output_words);
 
             for ( i = 0; i < 4; i++ ) {
                 output_words[i] = output_words[i] ^ iv_words[i];
@@ -267,7 +320,7 @@ int esp_aes_crypt_cbc( esp_aes_context *ctx,
                 output_words[i] = input_words[i] ^ iv_words[i];
             }
 
-            esp_aes_block(output_words, output_words);
+            esp_aes_block(ctx, output_words, output_words);
             memcpy( iv_words, output_words, 16 );
 
             input_words  += 4;
@@ -295,14 +348,19 @@ int esp_aes_crypt_cfb128( esp_aes_context *ctx,
     int c;
     size_t n = *iv_off;
 
+    if (!valid_key_length(ctx)) {
+        return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
+    }
+
     esp_aes_acquire_hardware();
+    ctx->key_in_hardware = 0;
 
     esp_aes_setkey_hardware(ctx, ESP_AES_ENCRYPT);
 
     if ( mode == ESP_AES_DECRYPT ) {
         while ( length-- ) {
             if ( n == 0 ) {
-                esp_aes_block(iv, iv );
+                esp_aes_block(ctx, iv, iv );
             }
 
             c = *input++;
@@ -314,7 +372,7 @@ int esp_aes_crypt_cfb128( esp_aes_context *ctx,
     } else {
         while ( length-- ) {
             if ( n == 0 ) {
-                esp_aes_block(iv, iv );
+                esp_aes_block(ctx, iv, iv );
             }
 
             iv[n] = *output++ = (unsigned char)( iv[n] ^ *input++ );
@@ -343,13 +401,18 @@ int esp_aes_crypt_cfb8( esp_aes_context *ctx,
     unsigned char c;
     unsigned char ov[17];
 
+    if (!valid_key_length(ctx)) {
+        return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
+    }
+
     esp_aes_acquire_hardware();
+    ctx->key_in_hardware = 0;
 
     esp_aes_setkey_hardware(ctx, ESP_AES_ENCRYPT);
 
     while ( length-- ) {
         memcpy( ov, iv, 16 );
-        esp_aes_block(iv, iv);
+        esp_aes_block(ctx, iv, iv);
 
         if ( mode == ESP_AES_DECRYPT ) {
             ov[16] = *input;
@@ -383,13 +446,18 @@ int esp_aes_crypt_ctr( esp_aes_context *ctx,
     int c, i;
     size_t n = *nc_off;
 
+    if (!valid_key_length(ctx)) {
+        return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH;
+    }
+
     esp_aes_acquire_hardware();
+    ctx->key_in_hardware = 0;
 
     esp_aes_setkey_hardware(ctx, ESP_AES_ENCRYPT);
 
     while ( length-- ) {
         if ( n == 0 ) {
-            esp_aes_block(nonce_counter, stream_block);
+            esp_aes_block(ctx, nonce_counter, stream_block);
 
             for ( i = 16; i > 0; i-- )
                 if ( ++nonce_counter[i - 1] != 0 ) {

diff --git a/components/esp32/include/hwcrypto/aes.h b/components/esp32/include/hwcrypto/aes.h
index 1dd5291f07c06f37cd21ccc7f23b8520bff23997..5a3fd24fe038918ea271bfefad7ee961229c42a9 100644 (file)
--- a/components/esp32/include/hwcrypto/aes.h
+++ b/components/esp32/include/hwcrypto/aes.h
@@ -41,17 +41,13 @@ extern "C" {
 /**
  * \brief          AES context structure
  *
- * \note           buf is able to hold 32 extra bytes, which can be used:
- *                 - for alignment purposes if VIA padlock is used, and/or
- *                 - to simplify key expansion in the 256-bit case by
- *                 generating an extra round key
  */
 typedef struct {
     uint8_t key_bytes;
+    volatile uint8_t key_in_hardware; /* This variable is used for fault injection checks, so marked volatile to avoid optimisation */
     uint8_t key[32];
 } esp_aes_context;
 
-
 /**
  * \brief The AES XTS context-type definition.
  */