From 7ca54ff43aa01276295d16badd4291afa65a2f03 Mon Sep 17 00:00:00 2001 From: Ashish Sharma Date: Mon, 6 Jul 2026 17:24:57 +0800 Subject: [PATCH] fix(mbedtls): validate crypto input lengths (TEE OOB, auth-bypass, overflows) --- .../mbedtls/esp_crt_bundle/esp_crt_bundle.c | 22 ++++++++++++++-- components/mbedtls/port/aes/block/esp_aes.c | 23 ++++++++++++++++- components/mbedtls/port/aes/dma/esp_aes.c | 19 ++++++++++++++ components/mbedtls/port/aes/esp_aes_gcm.c | 24 +++++++++++++++++- components/mbedtls/port/ecc/esp_ecc.c | 25 ++++++++++++++++++- 5 files changed, 108 insertions(+), 5 deletions(-) diff --git a/components/mbedtls/esp_crt_bundle/esp_crt_bundle.c b/components/mbedtls/esp_crt_bundle/esp_crt_bundle.c index de6b7f71073..a29b0226b4a 100644 --- a/components/mbedtls/esp_crt_bundle/esp_crt_bundle.c +++ b/components/mbedtls/esp_crt_bundle/esp_crt_bundle.c @@ -99,9 +99,11 @@ static const uint8_t* esp_crt_get_key(const cert_t cert) return esp_crt_get_name(cert) + esp_crt_get_name_len(cert); } -static uint16_t esp_crt_get_len(const cert_t cert) +static uint32_t esp_crt_get_len(const cert_t cert) { - return CRT_HEADER_SIZE + esp_crt_get_name_len(cert) + esp_crt_get_key_len(cert); + /* Widened to uint32_t: name_len and key_len are each uint16_t, so their sum plus the + * header can exceed UINT16_MAX and would otherwise wrap, under-reporting the cert size. */ + return (uint32_t)CRT_HEADER_SIZE + (uint32_t)esp_crt_get_name_len(cert) + (uint32_t)esp_crt_get_key_len(cert); } static uint32_t esp_crt_get_cert_offset(const bundle_t bundle, const uint32_t index) @@ -305,6 +307,11 @@ static bool esp_crt_check_bundle(const uint8_t* const x509_bundle, const size_t return false; } + if (unlikely(num_certs == 0)) { + // No certificates: the loops below compute num_certs - 1, which would underflow. + return false; + } + // Check all offsets for consistency with certificate data for (uint32_t i = 0; i < num_certs - 1; ++i) { const uint32_t off = esp_crt_get_cert_offset(x509_bundle, i); @@ -318,6 +325,17 @@ static bool esp_crt_check_bundle(const uint8_t* const x509_bundle, const size_t } } + // The loop above stops at num_certs - 1, so the final certificate's extent is never + // validated; check it explicitly so its key data cannot run past the bundle (CWE-125). + const uint32_t last_off = esp_crt_get_cert_offset(x509_bundle, num_certs - 1); + if (unlikely(last_off >= bundle_size)) { + return false; + } + const uint32_t last_len = esp_crt_get_len(x509_bundle + last_off); + if (unlikely((uint64_t)last_off + last_len > bundle_size)) { + return false; + } + // All checks passed. return true; } diff --git a/components/mbedtls/port/aes/block/esp_aes.c b/components/mbedtls/port/aes/block/esp_aes.c index 00cd9584381..4de7ff666ba 100644 --- a/components/mbedtls/port/aes/block/esp_aes.c +++ b/components/mbedtls/port/aes/block/esp_aes.c @@ -358,6 +358,16 @@ int esp_aes_crypt_cfb128(esp_aes_context *ctx, int c; size_t n = *iv_off; + + /* iv[] is a fixed AES_BLOCK_BYTES buffer and n indexes it directly (before the modulo update), + * so a caller-supplied *iv_off >= AES_BLOCK_BYTES -- attacker-controlled via the TEE secure + * service -- is an out-of-bounds read/write of iv[] in TEE context (CWE-787 / CWE-125). Bound + * it here (before acquiring the AES hardware) matching esp_aes_crypt_ofb(). */ + if (n >= AES_BLOCK_BYTES) { + ESP_LOGE(TAG, "IV offset out of bounds"); + return MBEDTLS_ERR_AES_BAD_INPUT_DATA; + } + esp_aes_acquire_hardware(); ctx->key_in_hardware = 0; ctx->key_in_hardware = aes_hal_setkey(ctx->key, ctx->key_bytes, ESP_AES_ENCRYPT); @@ -491,6 +501,17 @@ int esp_aes_crypt_ctr(esp_aes_context *ctx, } size_t n = *nc_off; + + /* stream_block[] is a fixed AES_BLOCK_BYTES buffer and n indexes it directly (before the + * modulo update), so a caller-supplied *nc_off >= AES_BLOCK_BYTES -- attacker-controlled via + * the TEE secure service -- is an out-of-bounds read of stream_block[] in TEE context + * (CWE-125), leaking adjacent memory into the output. Bound it here, matching + * esp_aes_crypt_cfb128() and esp_aes_crypt_ofb(). */ + if (n >= AES_BLOCK_BYTES) { + ESP_LOGE(TAG, "Nonce offset out of bounds"); + return MBEDTLS_ERR_AES_BAD_INPUT_DATA; + } + if (!valid_key_length(ctx)) { return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH; } @@ -556,7 +577,7 @@ int esp_aes_crypt_ofb(esp_aes_context *ctx, n = *iv_off; - if (n > 15) { + if (n >= AES_BLOCK_BYTES) { return (MBEDTLS_ERR_AES_BAD_INPUT_DATA); } diff --git a/components/mbedtls/port/aes/dma/esp_aes.c b/components/mbedtls/port/aes/dma/esp_aes.c index 696168a9c35..fcd4ed8a203 100644 --- a/components/mbedtls/port/aes/dma/esp_aes.c +++ b/components/mbedtls/port/aes/dma/esp_aes.c @@ -347,6 +347,15 @@ int esp_aes_crypt_cfb128(esp_aes_context *ctx, n = *iv_off; + /* iv[] is a fixed AES_BLOCK_BYTES buffer and n indexes it directly (before the modulo update), + * so a caller-supplied *iv_off >= AES_BLOCK_BYTES -- attacker-controlled via the TEE secure + * service -- is an out-of-bounds read/write of iv[] in TEE context (CWE-787 / CWE-125). Bound + * it here. */ + if (n >= AES_BLOCK_BYTES) { + ESP_LOGE(TAG, "IV offset out of bounds"); + return MBEDTLS_ERR_AES_BAD_INPUT_DATA; + } + /* First process the *iv_off bytes * which are pending from the previous call to this API */ @@ -492,6 +501,16 @@ int esp_aes_crypt_ctr(esp_aes_context *ctx, n = *nc_off; + /* stream_block[] is a fixed AES_BLOCK_BYTES buffer and n indexes it directly (before the + * modulo update), so a caller-supplied *nc_off >= AES_BLOCK_BYTES -- attacker-controlled via + * the TEE secure service -- is an out-of-bounds read of stream_block[] in TEE context + * (CWE-125), leaking adjacent memory into the output. Bound it here, matching + * esp_aes_crypt_cfb128() and esp_aes_crypt_ofb(). */ + if (n >= AES_BLOCK_BYTES) { + ESP_LOGE(TAG, "Nonce offset out of bounds"); + return MBEDTLS_ERR_AES_BAD_INPUT_DATA; + } + if (!valid_key_length(ctx)) { return MBEDTLS_ERR_AES_INVALID_KEY_LENGTH; } diff --git a/components/mbedtls/port/aes/esp_aes_gcm.c b/components/mbedtls/port/aes/esp_aes_gcm.c index 839fb71a1fb..9e5ac70aa04 100644 --- a/components/mbedtls/port/aes/esp_aes_gcm.c +++ b/components/mbedtls/port/aes/esp_aes_gcm.c @@ -518,6 +518,14 @@ int esp_aes_gcm_update( esp_gcm_context *ctx, return MBEDTLS_ERR_GCM_BAD_INPUT; } + /* Honor the documented contract: the output buffer must hold input_length bytes, which are + * written unconditionally below; without this check an undersized buffer overflows (CWE-20 + * -> CWE-787). MBEDTLS_ERR_GCM_BAD_INPUT is the bad-input error code used throughout this file. */ + if ( output_size < input_length ) { + ESP_LOGE(TAG, "Output buffer too small"); + return MBEDTLS_ERR_GCM_BAD_INPUT; + } + if ( output > input && (size_t) ( output - input ) < input_length ) { return ( MBEDTLS_ERR_GCM_BAD_INPUT ); } @@ -638,7 +646,7 @@ static int esp_aes_gcm_crypt_and_tag_partial_hw( esp_gcm_context *ctx, return ( ret ); } - if ( ( ret = esp_aes_gcm_update( ctx, input, length, output, 0, &olen ) ) != 0 ) { + if ( ( ret = esp_aes_gcm_update( ctx, input, length, output, length, &olen ) ) != 0 ) { return ( ret ); } @@ -671,6 +679,12 @@ int esp_aes_gcm_crypt_and_tag( esp_gcm_context *ctx, return mbedtls_gcm_crypt_and_tag_soft(ctx->ctx_soft, mode, length, iv, iv_len, aad, aad_len, input, output, tag_len, tag); } #endif + /* GCM tags are 4..16 bytes. Validate here so the hardware path also rejects an invalid + * tag_len (the software path enforces this in esp_aes_gcm_finish()); otherwise the HAL tag + * read would be driven with an out-of-range length (CWE-125 / CWE-787). */ + if ( tag_len < 4 || tag_len > 16 ) { + return MBEDTLS_ERR_GCM_BAD_INPUT; + } #if CONFIG_MBEDTLS_HARDWARE_GCM int ret; size_t remainder_bit; @@ -771,6 +785,14 @@ int esp_aes_gcm_auth_decrypt( esp_gcm_context *ctx, size_t i; int diff; + /* Validate tag_len before use: a zero tag_len makes the constant-time comparison loop + * below run zero iterations, so diff stays 0 and any forged ciphertext is accepted as + * authentic (CWE-347); an oversized tag_len also over-reads the 16-byte check_tag + * (CWE-125). Enforce the same 4..16 range as esp_aes_gcm_finish(). */ + if ( tag_len > 16 || tag_len < 4 ) { + return MBEDTLS_ERR_GCM_BAD_INPUT; + } + if ( ( ret = esp_aes_gcm_crypt_and_tag( ctx, ESP_AES_DECRYPT, length, iv, iv_len, aad, aad_len, input, output, tag_len, check_tag ) ) != 0 ) { diff --git a/components/mbedtls/port/ecc/esp_ecc.c b/components/mbedtls/port/ecc/esp_ecc.c index 43060022fe5..59a558fd2e7 100644 --- a/components/mbedtls/port/ecc/esp_ecc.c +++ b/components/mbedtls/port/ecc/esp_ecc.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2021-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2021-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -33,6 +33,17 @@ int esp_ecc_point_multiply(const ecc_point_t *point, const uint8_t *scalar, ecc_ uint16_t len = point->len; ecc_mode_t work_mode = verify_first ? ECC_MODE_VERIFY_THEN_POINT_MUL : ECC_MODE_POINT_MUL; + /* len is used as the HW read/write byte count for the fixed-size ecc_point_t buffers; + * reject any value that is not a supported curve length before touching hardware. On the + * TEE secure-service path this field is attacker-controlled (CWE-20 -> OOB read/write). */ + if (len != P192_LEN && len != P256_LEN +#if SOC_ECC_SUPPORT_CURVE_P384 + && len != P384_LEN +#endif + ) { + return -1; + } + esp_ecc_acquire_hardware(); ecc_hal_write_mul_param(scalar, point->x, point->y, len); @@ -65,6 +76,18 @@ int esp_ecc_point_verify(const ecc_point_t *point) { int result; + /* point->len drives a fixed-stride MMIO write loop in the HAL; an unvalidated oversized + * value (attacker-controlled via the TEE secure service) walks past the ECC register block + * and can reach other peripheral registers (CWE-787). Reject non-curve lengths up front and + * return 0 (point not verified) -- the fail-safe value for this routine. */ + if (point->len != P192_LEN && point->len != P256_LEN +#if SOC_ECC_SUPPORT_CURVE_P384 + && point->len != P384_LEN +#endif + ) { + return 0; + } + esp_ecc_acquire_hardware(); ecc_hal_write_verify_param(point->x, point->y, point->len); ecc_hal_set_mode(ECC_MODE_VERIFY);