From cd693c36fd01e2342b3017271451a8f042f4b42a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Jun 2024 19:13:19 +0200 Subject: [PATCH 01/17] MBEDTLS_STATIC_ASSERT: make it work outside of a function At the top level, the macro would have had to be used without a following semicolon (except with permissive compilers that accept spurious semicolons outside of a function), which is confusing to humans and indenters. Fix that. Signed-off-by: Gilles Peskine --- library/common.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/library/common.h b/library/common.h index 3936ffdfe1..7bb2674293 100644 --- a/library/common.h +++ b/library/common.h @@ -352,17 +352,19 @@ static inline void mbedtls_xor_no_simd(unsigned char *r, #endif /* Always provide a static assert macro, so it can be used unconditionally. - * It will expand to nothing on some systems. - * Can be used outside functions (but don't add a trailing ';' in that case: - * the semicolon is included here to avoid triggering -Wextra-semi when - * MBEDTLS_STATIC_ASSERT() expands to nothing). - * Can't use the C11-style `defined(static_assert)` on FreeBSD, since it + * It does nothing on systems where we don't know how to define a static assert. + */ +/* Can't use the C11-style `defined(static_assert)` on FreeBSD, since it * defines static_assert even with -std=c99, but then complains about it. */ #if defined(static_assert) && !defined(__FreeBSD__) -#define MBEDTLS_STATIC_ASSERT(expr, msg) static_assert(expr, msg); +#define MBEDTLS_STATIC_ASSERT(expr, msg) static_assert(expr, msg) #else -#define MBEDTLS_STATIC_ASSERT(expr, msg) +/* Make sure `MBEDTLS_STATIC_ASSERT(expr, msg);` is valid both inside and + * outside a function. We choose a struct declaration, which can be repeated + * any number of times and does not need a matching definition. */ +#define MBEDTLS_STATIC_ASSERT(expr, msg) \ + struct ISO_C_does_not_allow_extra_semicolon_outside_of_a_function #endif #if defined(__has_builtin) From 4804847b1528c00fff132fcde66f35d94e4f55b5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Jun 2024 21:47:31 +0200 Subject: [PATCH 02/17] Make it possible to enable CTR_DRBG/PSA without a PSA AES driver Make it possible, but not officially supported, to switch the CTR_DRBG module to PSA mode even if MBEDTLS_AES_C is defined. This is not really useful in practice, but is convenient to test the PSA mode without setting up drivers. Signed-off-by: Gilles Peskine --- include/mbedtls/ctr_drbg.h | 25 ++++++++++++++++----- library/ctr_drbg.c | 30 ++++++++++++------------- tests/include/test/psa_crypto_helpers.h | 8 ++++--- tests/src/psa_crypto_helpers.c | 9 ++++++-- 4 files changed, 47 insertions(+), 25 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index c00756df1b..c7db4702a9 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -32,9 +32,24 @@ #include "mbedtls/build_info.h" -/* In case AES_C is defined then it is the primary option for backward - * compatibility purposes. If that's not available, PSA is used instead */ -#if defined(MBEDTLS_AES_C) +/* The CTR_DRBG implementation can either directly call the low-level AES + * module (gated by MBEDTLS_AES_C) or call the PSA API to perform AES + * operations. Calling the AES module directly is the default, both for + * maximum backward compatibility and because it's a bit more efficient + * (less glue code). + * + * When MBEDTLS_AES_C is disabled, the CTR_DRBG module calls PSA crypto and + * thus benefits from the PSA AES accelerator driver. + * It is technically possible to enable MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO + * to use PSA even when MBEDTLS_AES_C is disabled, but there is very little + * reason to do so other than testing purposes and this is not officially + * supported. + */ +#if !defined(MBEDTLS_AES_C) +#define MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO +#endif + +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) #include "mbedtls/aes.h" #else #include "psa/crypto.h" @@ -157,7 +172,7 @@ extern "C" { #define MBEDTLS_CTR_DRBG_ENTROPY_NONCE_LEN (MBEDTLS_CTR_DRBG_ENTROPY_LEN + 1) / 2 #endif -#if !defined(MBEDTLS_AES_C) +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) typedef struct mbedtls_ctr_drbg_psa_context { mbedtls_svc_key_id_t key_id; psa_cipher_operation_t operation; @@ -189,7 +204,7 @@ typedef struct mbedtls_ctr_drbg_context { * This is the maximum number of requests * that can be made between reseedings. */ -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) mbedtls_aes_context MBEDTLS_PRIVATE(aes_ctx); /*!< The AES context. */ #else mbedtls_ctr_drbg_psa_context MBEDTLS_PRIVATE(psa_ctx); /*!< The PSA context. */ diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 66d9d28c58..aea62bbfd1 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -26,13 +26,13 @@ #endif /* Using error translation functions from PSA to MbedTLS */ -#if !defined(MBEDTLS_AES_C) +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) #include "psa_util_internal.h" #endif #include "mbedtls/platform.h" -#if !defined(MBEDTLS_AES_C) +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) static psa_status_t ctr_drbg_setup_psa_context(mbedtls_ctr_drbg_psa_context *psa_ctx, unsigned char *key, size_t key_len) { @@ -73,7 +73,7 @@ static void ctr_drbg_destroy_psa_contex(mbedtls_ctr_drbg_psa_context *psa_ctx) void mbedtls_ctr_drbg_init(mbedtls_ctr_drbg_context *ctx) { memset(ctx, 0, sizeof(mbedtls_ctr_drbg_context)); -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) mbedtls_aes_init(&ctx->aes_ctx); #else ctx->psa_ctx.key_id = MBEDTLS_SVC_KEY_ID_INIT; @@ -102,7 +102,7 @@ void mbedtls_ctr_drbg_free(mbedtls_ctr_drbg_context *ctx) mbedtls_mutex_free(&ctx->mutex); } #endif -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) mbedtls_aes_free(&ctx->aes_ctx); #else ctr_drbg_destroy_psa_contex(&ctx->psa_ctx); @@ -168,7 +168,7 @@ static int block_cipher_df(unsigned char *output, unsigned char chain[MBEDTLS_CTR_DRBG_BLOCKSIZE]; unsigned char *p, *iv; int ret = 0; -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) mbedtls_aes_context aes_ctx; #else psa_status_t status; @@ -209,7 +209,7 @@ static int block_cipher_df(unsigned char *output, key[i] = i; } -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) mbedtls_aes_init(&aes_ctx); if ((ret = mbedtls_aes_setkey_enc(&aes_ctx, key, @@ -238,7 +238,7 @@ static int block_cipher_df(unsigned char *output, use_len -= (use_len >= MBEDTLS_CTR_DRBG_BLOCKSIZE) ? MBEDTLS_CTR_DRBG_BLOCKSIZE : use_len; -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) if ((ret = mbedtls_aes_crypt_ecb(&aes_ctx, MBEDTLS_AES_ENCRYPT, chain, chain)) != 0) { goto exit; @@ -264,7 +264,7 @@ static int block_cipher_df(unsigned char *output, /* * Do final encryption with reduced data */ -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) if ((ret = mbedtls_aes_setkey_enc(&aes_ctx, tmp, MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { goto exit; @@ -282,7 +282,7 @@ static int block_cipher_df(unsigned char *output, p = output; for (j = 0; j < MBEDTLS_CTR_DRBG_SEEDLEN; j += MBEDTLS_CTR_DRBG_BLOCKSIZE) { -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) if ((ret = mbedtls_aes_crypt_ecb(&aes_ctx, MBEDTLS_AES_ENCRYPT, iv, iv)) != 0) { goto exit; @@ -299,7 +299,7 @@ static int block_cipher_df(unsigned char *output, p += MBEDTLS_CTR_DRBG_BLOCKSIZE; } exit: -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) mbedtls_aes_free(&aes_ctx); #else ctr_drbg_destroy_psa_contex(&psa_ctx); @@ -336,7 +336,7 @@ static int ctr_drbg_update_internal(mbedtls_ctr_drbg_context *ctx, unsigned char *p = tmp; int j; int ret = 0; -#if !defined(MBEDTLS_AES_C) +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) psa_status_t status; size_t tmp_len; #endif @@ -352,7 +352,7 @@ static int ctr_drbg_update_internal(mbedtls_ctr_drbg_context *ctx, /* * Crypt counter block */ -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) if ((ret = mbedtls_aes_crypt_ecb(&ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, ctx->counter, p)) != 0) { goto exit; @@ -374,7 +374,7 @@ static int ctr_drbg_update_internal(mbedtls_ctr_drbg_context *ctx, /* * Update key and counter */ -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) if ((ret = mbedtls_aes_setkey_enc(&ctx->aes_ctx, tmp, MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { goto exit; @@ -564,7 +564,7 @@ int mbedtls_ctr_drbg_seed(mbedtls_ctr_drbg_context *ctx, good_nonce_len(ctx->entropy_len)); /* Initialize with an empty key. */ -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) if ((ret = mbedtls_aes_setkey_enc(&ctx->aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { return ret; @@ -655,7 +655,7 @@ int mbedtls_ctr_drbg_random_with_add(void *p_rng, /* * Crypt counter block */ -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) if ((ret = mbedtls_aes_crypt_ecb(&ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, ctx->counter, locals.tmp)) != 0) { goto exit; diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 7306d8eb10..39611a2634 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -16,6 +16,8 @@ #include #endif +#include + #if defined(MBEDTLS_PSA_CRYPTO_C) /** Initialize the PSA Crypto subsystem. */ #define PSA_INIT() PSA_ASSERT(psa_crypto_init()) @@ -430,12 +432,12 @@ uint64_t mbedtls_test_parse_binary_string(data_t *bin_string); * This is like #PSA_DONE except it does nothing under the same conditions as * #AES_PSA_INIT. */ -#if defined(MBEDTLS_AES_C) +#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) #define AES_PSA_INIT() ((void) 0) #define AES_PSA_DONE() ((void) 0) -#else /* MBEDTLS_AES_C */ +#else /* MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO */ #define AES_PSA_INIT() PSA_INIT() #define AES_PSA_DONE() PSA_DONE() -#endif /* MBEDTLS_AES_C */ +#endif /* MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO */ #endif /* PSA_CRYPTO_HELPERS_H */ diff --git a/tests/src/psa_crypto_helpers.c b/tests/src/psa_crypto_helpers.c index e1ea2b5c81..1581eecb3b 100644 --- a/tests/src/psa_crypto_helpers.c +++ b/tests/src/psa_crypto_helpers.c @@ -13,6 +13,10 @@ #include #include +#if defined(MBEDTLS_CTR_DRBG_C) +#include +#endif + #if defined(MBEDTLS_PSA_CRYPTO_C) #include @@ -70,8 +74,9 @@ const char *mbedtls_test_helper_is_psa_leaking(void) mbedtls_psa_get_stats(&stats); -#if defined(MBEDTLS_CTR_DRBG_C) && !defined(MBEDTLS_AES_C) && \ - !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) +#if !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) && \ + defined(MBEDTLS_CTR_DRBG_C) && \ + defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) /* When AES_C is not defined and PSA does not have an external RNG, * then CTR_DRBG uses PSA to perform AES-ECB. In this scenario 1 key * slot is used internally from PSA to hold the AES key and it should From 86c603702e74db1490579e6f0a20a8c5a2d8d6dd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Jun 2024 22:08:44 +0200 Subject: [PATCH 03/17] Reorder blocks to avoid double negations Convert `#if !... A #else B #endif` to `#if ... B #else A`. No semantic change. Signed-off-by: Gilles Peskine --- include/mbedtls/ctr_drbg.h | 12 +-- library/ctr_drbg.c | 122 ++++++++++++------------ tests/include/test/psa_crypto_helpers.h | 8 +- 3 files changed, 71 insertions(+), 71 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index c7db4702a9..216169c770 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -49,10 +49,10 @@ #define MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO #endif -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) -#include "mbedtls/aes.h" -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) #include "psa/crypto.h" +#else +#include "mbedtls/aes.h" #endif #include "entropy.h" @@ -204,10 +204,10 @@ typedef struct mbedtls_ctr_drbg_context { * This is the maximum number of requests * that can be made between reseedings. */ -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - mbedtls_aes_context MBEDTLS_PRIVATE(aes_ctx); /*!< The AES context. */ -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) mbedtls_ctr_drbg_psa_context MBEDTLS_PRIVATE(psa_ctx); /*!< The PSA context. */ +#else + mbedtls_aes_context MBEDTLS_PRIVATE(aes_ctx); /*!< The AES context. */ #endif /* diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index aea62bbfd1..b82044eb7d 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -73,11 +73,11 @@ static void ctr_drbg_destroy_psa_contex(mbedtls_ctr_drbg_psa_context *psa_ctx) void mbedtls_ctr_drbg_init(mbedtls_ctr_drbg_context *ctx) { memset(ctx, 0, sizeof(mbedtls_ctr_drbg_context)); -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - mbedtls_aes_init(&ctx->aes_ctx); -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) ctx->psa_ctx.key_id = MBEDTLS_SVC_KEY_ID_INIT; ctx->psa_ctx.operation = psa_cipher_operation_init(); +#else + mbedtls_aes_init(&ctx->aes_ctx); #endif /* Indicate that the entropy nonce length is not set explicitly. * See mbedtls_ctr_drbg_set_nonce_len(). */ @@ -102,10 +102,10 @@ void mbedtls_ctr_drbg_free(mbedtls_ctr_drbg_context *ctx) mbedtls_mutex_free(&ctx->mutex); } #endif -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - mbedtls_aes_free(&ctx->aes_ctx); -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) ctr_drbg_destroy_psa_contex(&ctx->psa_ctx); +#else + mbedtls_aes_free(&ctx->aes_ctx); #endif mbedtls_platform_zeroize(ctx, sizeof(mbedtls_ctr_drbg_context)); ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; @@ -168,15 +168,15 @@ static int block_cipher_df(unsigned char *output, unsigned char chain[MBEDTLS_CTR_DRBG_BLOCKSIZE]; unsigned char *p, *iv; int ret = 0; -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - mbedtls_aes_context aes_ctx; -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) psa_status_t status; size_t tmp_len; mbedtls_ctr_drbg_psa_context psa_ctx; psa_ctx.key_id = MBEDTLS_SVC_KEY_ID_INIT; psa_ctx.operation = psa_cipher_operation_init(); +#else + mbedtls_aes_context aes_ctx; #endif int i, j; @@ -209,19 +209,19 @@ static int block_cipher_df(unsigned char *output, key[i] = i; } -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) + status = ctr_drbg_setup_psa_context(&psa_ctx, key, sizeof(key)); + if (status != PSA_SUCCESS) { + ret = psa_generic_status_to_mbedtls(status); + goto exit; + } +#else mbedtls_aes_init(&aes_ctx); if ((ret = mbedtls_aes_setkey_enc(&aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { goto exit; } -#else - status = ctr_drbg_setup_psa_context(&psa_ctx, key, sizeof(key)); - if (status != PSA_SUCCESS) { - ret = psa_generic_status_to_mbedtls(status); - goto exit; - } #endif /* @@ -238,18 +238,18 @@ static int block_cipher_df(unsigned char *output, use_len -= (use_len >= MBEDTLS_CTR_DRBG_BLOCKSIZE) ? MBEDTLS_CTR_DRBG_BLOCKSIZE : use_len; -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - if ((ret = mbedtls_aes_crypt_ecb(&aes_ctx, MBEDTLS_AES_ENCRYPT, - chain, chain)) != 0) { - goto exit; - } -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) status = psa_cipher_update(&psa_ctx.operation, chain, MBEDTLS_CTR_DRBG_BLOCKSIZE, chain, MBEDTLS_CTR_DRBG_BLOCKSIZE, &tmp_len); if (status != PSA_SUCCESS) { ret = psa_generic_status_to_mbedtls(status); goto exit; } +#else + if ((ret = mbedtls_aes_crypt_ecb(&aes_ctx, MBEDTLS_AES_ENCRYPT, + chain, chain)) != 0) { + goto exit; + } #endif } @@ -264,12 +264,7 @@ static int block_cipher_df(unsigned char *output, /* * Do final encryption with reduced data */ -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - if ((ret = mbedtls_aes_setkey_enc(&aes_ctx, tmp, - MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { - goto exit; - } -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) ctr_drbg_destroy_psa_contex(&psa_ctx); status = ctr_drbg_setup_psa_context(&psa_ctx, tmp, MBEDTLS_CTR_DRBG_KEYSIZE); @@ -277,32 +272,37 @@ static int block_cipher_df(unsigned char *output, ret = psa_generic_status_to_mbedtls(status); goto exit; } +#else + if ((ret = mbedtls_aes_setkey_enc(&aes_ctx, tmp, + MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { + goto exit; + } #endif iv = tmp + MBEDTLS_CTR_DRBG_KEYSIZE; p = output; for (j = 0; j < MBEDTLS_CTR_DRBG_SEEDLEN; j += MBEDTLS_CTR_DRBG_BLOCKSIZE) { -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - if ((ret = mbedtls_aes_crypt_ecb(&aes_ctx, MBEDTLS_AES_ENCRYPT, - iv, iv)) != 0) { - goto exit; - } -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) status = psa_cipher_update(&psa_ctx.operation, iv, MBEDTLS_CTR_DRBG_BLOCKSIZE, iv, MBEDTLS_CTR_DRBG_BLOCKSIZE, &tmp_len); if (status != PSA_SUCCESS) { ret = psa_generic_status_to_mbedtls(status); goto exit; } +#else + if ((ret = mbedtls_aes_crypt_ecb(&aes_ctx, MBEDTLS_AES_ENCRYPT, + iv, iv)) != 0) { + goto exit; + } #endif memcpy(p, iv, MBEDTLS_CTR_DRBG_BLOCKSIZE); p += MBEDTLS_CTR_DRBG_BLOCKSIZE; } exit: -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - mbedtls_aes_free(&aes_ctx); -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) ctr_drbg_destroy_psa_contex(&psa_ctx); +#else + mbedtls_aes_free(&aes_ctx); #endif /* * tidy up the stack @@ -352,18 +352,18 @@ static int ctr_drbg_update_internal(mbedtls_ctr_drbg_context *ctx, /* * Crypt counter block */ -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - if ((ret = mbedtls_aes_crypt_ecb(&ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, - ctx->counter, p)) != 0) { - goto exit; - } -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) status = psa_cipher_update(&ctx->psa_ctx.operation, ctx->counter, sizeof(ctx->counter), p, MBEDTLS_CTR_DRBG_BLOCKSIZE, &tmp_len); if (status != PSA_SUCCESS) { ret = psa_generic_status_to_mbedtls(status); goto exit; } +#else + if ((ret = mbedtls_aes_crypt_ecb(&ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, + ctx->counter, p)) != 0) { + goto exit; + } #endif p += MBEDTLS_CTR_DRBG_BLOCKSIZE; @@ -374,12 +374,7 @@ static int ctr_drbg_update_internal(mbedtls_ctr_drbg_context *ctx, /* * Update key and counter */ -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - if ((ret = mbedtls_aes_setkey_enc(&ctx->aes_ctx, tmp, - MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { - goto exit; - } -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) ctr_drbg_destroy_psa_contex(&ctx->psa_ctx); status = ctr_drbg_setup_psa_context(&ctx->psa_ctx, tmp, MBEDTLS_CTR_DRBG_KEYSIZE); @@ -387,6 +382,11 @@ static int ctr_drbg_update_internal(mbedtls_ctr_drbg_context *ctx, ret = psa_generic_status_to_mbedtls(status); goto exit; } +#else + if ((ret = mbedtls_aes_setkey_enc(&ctx->aes_ctx, tmp, + MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { + goto exit; + } #endif memcpy(ctx->counter, tmp + MBEDTLS_CTR_DRBG_KEYSIZE, MBEDTLS_CTR_DRBG_BLOCKSIZE); @@ -564,12 +564,7 @@ int mbedtls_ctr_drbg_seed(mbedtls_ctr_drbg_context *ctx, good_nonce_len(ctx->entropy_len)); /* Initialize with an empty key. */ -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - if ((ret = mbedtls_aes_setkey_enc(&ctx->aes_ctx, key, - MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { - return ret; - } -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) psa_status_t status; status = ctr_drbg_setup_psa_context(&ctx->psa_ctx, key, MBEDTLS_CTR_DRBG_KEYSIZE); @@ -577,6 +572,11 @@ int mbedtls_ctr_drbg_seed(mbedtls_ctr_drbg_context *ctx, ret = psa_generic_status_to_mbedtls(status); return status; } +#else + if ((ret = mbedtls_aes_setkey_enc(&ctx->aes_ctx, key, + MBEDTLS_CTR_DRBG_KEYBITS)) != 0) { + return ret; + } #endif /* Do the initial seeding. */ @@ -655,12 +655,7 @@ int mbedtls_ctr_drbg_random_with_add(void *p_rng, /* * Crypt counter block */ -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - if ((ret = mbedtls_aes_crypt_ecb(&ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, - ctx->counter, locals.tmp)) != 0) { - goto exit; - } -#else +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) psa_status_t status; size_t tmp_len; @@ -670,6 +665,11 @@ int mbedtls_ctr_drbg_random_with_add(void *p_rng, ret = psa_generic_status_to_mbedtls(status); goto exit; } +#else + if ((ret = mbedtls_aes_crypt_ecb(&ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, + ctx->counter, locals.tmp)) != 0) { + goto exit; + } #endif use_len = (output_len > MBEDTLS_CTR_DRBG_BLOCKSIZE) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index 39611a2634..fae715ca9a 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -432,12 +432,12 @@ uint64_t mbedtls_test_parse_binary_string(data_t *bin_string); * This is like #PSA_DONE except it does nothing under the same conditions as * #AES_PSA_INIT. */ -#if !defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) -#define AES_PSA_INIT() ((void) 0) -#define AES_PSA_DONE() ((void) 0) -#else /* MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO */ +#if defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) #define AES_PSA_INIT() PSA_INIT() #define AES_PSA_DONE() PSA_DONE() +#else /* MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO */ +#define AES_PSA_INIT() ((void) 0) +#define AES_PSA_DONE() ((void) 0) #endif /* MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO */ #endif /* PSA_CRYPTO_HELPERS_H */ From d72ad738bdb3269d31eec06a0a458442decadf2d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 13 Jun 2024 16:06:45 +0200 Subject: [PATCH 04/17] Prevent mbedtls_psa_register_se_key with volatile keys mbedtls_psa_register_se_key() is not usable with volatile keys, since there is no way to return the implementation-chosen key identifier which would be needed to use the key. Document this limitation. Reject an attempt to create such an unusable key. Fixes #9253. Signed-off-by: Gilles Peskine --- ChangeLog.d/mbedtls_psa_register_se_key.txt | 3 +++ include/psa/crypto.h | 3 +++ include/psa/crypto_extra.h | 8 ++++++++ library/psa_crypto.c | 8 ++++++++ tests/suites/test_suite_psa_crypto_se_driver_hal.data | 11 ++++++++++- 5 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/mbedtls_psa_register_se_key.txt diff --git a/ChangeLog.d/mbedtls_psa_register_se_key.txt b/ChangeLog.d/mbedtls_psa_register_se_key.txt new file mode 100644 index 0000000000..2fc2751ac0 --- /dev/null +++ b/ChangeLog.d/mbedtls_psa_register_se_key.txt @@ -0,0 +1,3 @@ +Bugfix + * Document and enforce the limitation of mbedtls_psa_register_se_key() + to persistent keys. Resolves #9253. diff --git a/include/psa/crypto.h b/include/psa/crypto.h index 3525da221f..6d12e43d4f 100644 --- a/include/psa/crypto.h +++ b/include/psa/crypto.h @@ -129,6 +129,9 @@ static psa_key_attributes_t psa_key_attributes_init(void); * * \param[out] attributes The attribute structure to write to. * \param key The persistent identifier for the key. + * This can be any value in the range from + * #PSA_KEY_ID_USER_MIN to #PSA_KEY_ID_USER_MAX + * inclusive. */ static void psa_set_key_id(psa_key_attributes_t *attributes, mbedtls_svc_key_id_t key); diff --git a/include/psa/crypto_extra.h b/include/psa/crypto_extra.h index 6ed1f6c43a..5f413d6aa3 100644 --- a/include/psa/crypto_extra.h +++ b/include/psa/crypto_extra.h @@ -154,6 +154,14 @@ static inline void psa_clear_key_slot_number( * specified in \p attributes. * * \param[in] attributes The attributes of the existing key. + * - The lifetime must be a persistent lifetime + * in a secure element. Volatile lifetimes are + * not currently supported. + * - The key identifier must be in the valid + * range for persistent keys. + * - The key type and size must be specified and + * must be consistent with the key material + * in the secure element. * * \retval #PSA_SUCCESS * The key was successfully registered. diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 8100afc471..28bcd21be2 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2149,6 +2149,14 @@ psa_status_t mbedtls_psa_register_se_key( return PSA_ERROR_NOT_SUPPORTED; } + /* Not usable with volatile keys, even with an appropriate location, + * due to the API design. + * https://github.com/Mbed-TLS/mbedtls/issues/9253 + */ + if (PSA_KEY_LIFETIME_IS_VOLATILE(psa_get_key_lifetime(attributes))) { + return PSA_ERROR_INVALID_ARGUMENT; + } + status = psa_start_key_creation(PSA_KEY_CREATION_REGISTER, attributes, &slot, &driver); if (status != PSA_SUCCESS) { diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.data b/tests/suites/test_suite_psa_crypto_se_driver_hal.data index cc89c0fc20..d028b21821 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.data +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.data @@ -148,7 +148,16 @@ generate_key_smoke:PSA_KEY_TYPE_HMAC:256:PSA_ALG_HMAC( PSA_ALG_SHA_256 ) Key registration: smoke test register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:7:1:1:PSA_SUCCESS -Key registration: invalid lifetime (volatile internal storage) +Key registration: invalid lifetime (volatile, in SE, id=0) +register_key_smoke_test:TEST_SE_VOLATILE_LIFETIME:7:0:0:PSA_ERROR_INVALID_ARGUMENT + +Key registration: invalid lifetime (volatile, in SE, id=1) +register_key_smoke_test:TEST_SE_VOLATILE_LIFETIME:7:1:1:PSA_ERROR_INVALID_ARGUMENT + +Key registration: invalid lifetime (volatile, internal, id=0) +register_key_smoke_test:PSA_KEY_LIFETIME_VOLATILE:7:0:0:PSA_ERROR_INVALID_ARGUMENT + +Key registration: invalid lifetime (volatile, internal, id=1) register_key_smoke_test:PSA_KEY_LIFETIME_VOLATILE:7:1:1:PSA_ERROR_INVALID_ARGUMENT Key registration: invalid lifetime (internal storage) From 543909d894cc7eaa24c1fb4327876ee5b418c3bb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Jun 2024 22:10:08 +0200 Subject: [PATCH 05/17] Add a test for the built-in key range Restricting the built-in key range would be an API break since applications can hard-code a built-in key value and expect that it won't clash with anything else. Make it harder to accidentally break the API. Signed-off-by: Gilles Peskine --- include/psa/crypto_extra.h | 2 +- .../test_suite_psa_crypto_driver_wrappers.data | 3 +++ ...test_suite_psa_crypto_driver_wrappers.function | 15 +++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/include/psa/crypto_extra.h b/include/psa/crypto_extra.h index 5f413d6aa3..0cf42c6055 100644 --- a/include/psa/crypto_extra.h +++ b/include/psa/crypto_extra.h @@ -487,7 +487,7 @@ psa_status_t mbedtls_psa_external_get_random( * #PSA_KEY_ID_VENDOR_MIN and #PSA_KEY_ID_VENDOR_MAX and must not intersect * with any other set of implementation-chosen key identifiers. * - * This value is part of the library's ABI since changing it would invalidate + * This value is part of the library's API since changing it would invalidate * the values of built-in key identifiers in applications. */ #define MBEDTLS_PSA_KEY_ID_BUILTIN_MIN ((psa_key_id_t) 0x7fff0000) diff --git a/tests/suites/test_suite_psa_crypto_driver_wrappers.data b/tests/suites/test_suite_psa_crypto_driver_wrappers.data index 54e0892004..fb2da8c3c2 100644 --- a/tests/suites/test_suite_psa_crypto_driver_wrappers.data +++ b/tests/suites/test_suite_psa_crypto_driver_wrappers.data @@ -1,3 +1,6 @@ +Built-in key range +builtin_key_id_stability: + sign_hash transparent driver: in driver ECDSA SECP256R1 SHA-256 depends_on:PSA_WANT_ALG_DETERMINISTIC_ECDSA:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_BASIC:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_IMPORT:PSA_WANT_KEY_TYPE_ECC_KEY_PAIR_EXPORT:PSA_WANT_ECC_SECP_R1_256:PSA_WANT_ALG_SHA_256 sign_hash:PSA_KEY_TYPE_ECC_KEY_PAIR( PSA_ECC_FAMILY_SECP_R1 ):PSA_ALG_DETERMINISTIC_ECDSA( PSA_ALG_SHA_256 ):PSA_SUCCESS:"ab45435712649cb30bbddac49197eebf2740ffc7f874d9244c3460f54f322d3a":"9ac4335b469bbd791439248504dd0d49c71349a295fee5a1c68507f45a9e1c7b":"6a3399f69421ffe1490377adf2ea1f117d81a63cf5bf22e918d51175eb259151ce95d7c26cc04e25503e2f7a1ec3573e3c2412534bb4a19b3a7811742f49f50f":0:PSA_SUCCESS diff --git a/tests/suites/test_suite_psa_crypto_driver_wrappers.function b/tests/suites/test_suite_psa_crypto_driver_wrappers.function index e7925dd694..84611faddd 100644 --- a/tests/suites/test_suite_psa_crypto_driver_wrappers.function +++ b/tests/suites/test_suite_psa_crypto_driver_wrappers.function @@ -489,6 +489,21 @@ exit: * END_DEPENDENCIES */ +/* BEGIN_CASE */ +void builtin_key_id_stability() +{ + /* If the range of built-in keys is reduced, it's an API break, since + * it breaks user code that hard-codes the key id of built-in keys. + * It's ok to expand this range, but not to shrink it. That is, you + * may make the MIN smaller or the MAX larger at any time, but + * making the MIN larger or the MAX smaller can only be done in + * a new major version of the library. + */ + TEST_EQUAL(MBEDTLS_PSA_KEY_ID_BUILTIN_MIN, 0x7fff0000); + TEST_EQUAL(MBEDTLS_PSA_KEY_ID_BUILTIN_MAX, 0x7fffefff); +} +/* END_CASE */ + /* BEGIN_CASE */ void sign_hash(int key_type_arg, int alg_arg, From b6bf370159aaa01b9906aa7095fdcf48db0722ff Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Jun 2024 22:15:42 +0200 Subject: [PATCH 06/17] Assert that key ID ranges don't overlap Ensure that a key ID can't be in range for more than one of volatile keys, persistent (i.e. user-chosen) keys or built-in keys. Signed-off-by: Gilles Peskine --- library/psa_crypto_slot_management.c | 31 ++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 9986a44969..305ad6ec46 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -27,6 +27,37 @@ #include "mbedtls/threading.h" #endif + + +/* Make sure we have distinct ranges of key identifiers for distinct + * purposes. */ +MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_USER_MIN < PSA_KEY_ID_USER_MAX, + "Empty user key ID range"); +MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VENDOR_MIN < PSA_KEY_ID_VENDOR_MAX, + "Empty vendor key ID range"); +MBEDTLS_STATIC_ASSERT(MBEDTLS_PSA_KEY_ID_BUILTIN_MIN < MBEDTLS_PSA_KEY_ID_BUILTIN_MAX, + "Empty builtin key ID range"); +MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MIN < PSA_KEY_ID_VOLATILE_MAX, + "Empty volatile key ID range"); + +MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_USER_MAX < PSA_KEY_ID_VENDOR_MIN || + PSA_KEY_ID_VENDOR_MAX < PSA_KEY_ID_USER_MIN, + "Overlap between user key IDs and vendor key IDs"); + +MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VENDOR_MIN <= MBEDTLS_PSA_KEY_ID_BUILTIN_MIN && + MBEDTLS_PSA_KEY_ID_BUILTIN_MAX <= PSA_KEY_ID_VENDOR_MAX, + "Builtin key identifiers are not in the vendor range"); + +MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VENDOR_MIN <= PSA_KEY_ID_VOLATILE_MIN && + PSA_KEY_ID_VOLATILE_MAX <= PSA_KEY_ID_VENDOR_MAX, + "Volatile key identifiers are not in the vendor range"); + +MBEDTLS_STATIC_ASSERT(PSA_KEY_ID_VOLATILE_MAX < MBEDTLS_PSA_KEY_ID_BUILTIN_MIN || + MBEDTLS_PSA_KEY_ID_BUILTIN_MAX < PSA_KEY_ID_VOLATILE_MIN, + "Overlap between builtin key IDs and volatile key IDs"); + + + typedef struct { psa_key_slot_t key_slots[MBEDTLS_PSA_KEY_SLOT_COUNT]; uint8_t key_slots_initialized; From 708ec09e30e1a1ccc28faf26b6d2a39249c02323 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Jul 2024 20:02:37 +0200 Subject: [PATCH 07/17] Assert that the key ID range for volatile keys is large enough Signed-off-by: Gilles Peskine --- library/psa_crypto_slot_management.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index 305ad6ec46..c2949a61e7 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -65,6 +65,10 @@ typedef struct { static psa_global_data_t global_data; +MBEDTLS_STATIC_ASSERT(ARRAY_LENGTH(global_data.key_slots) <= + PSA_KEY_ID_VOLATILE_MAX - PSA_KEY_ID_VOLATILE_MIN + 1, + "The volatile key range is larger than the key slot array"); + static uint8_t psa_get_key_slots_initialized(void) { uint8_t initialized; From 7dea096086854e1916e18d173cda3757f803b6a3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Jul 2024 21:24:05 +0200 Subject: [PATCH 08/17] Fix overlap between volatile keys and built-in keys Fix interference between PSA volatile keys and built-in keys when MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS is enabled and MBEDTLS_PSA_KEY_SLOT_COUNT is more than 4096. This overlap used to make it possible that a volatile key would receive the identifier of a built-in key, and is now caught by a static assertion. Signed-off-by: Gilles Peskine --- ChangeLog.d/dynamic-keystore.txt | 4 ++++ library/psa_crypto_slot_management.h | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) create mode 100644 ChangeLog.d/dynamic-keystore.txt diff --git a/ChangeLog.d/dynamic-keystore.txt b/ChangeLog.d/dynamic-keystore.txt new file mode 100644 index 0000000000..d576dcd86f --- /dev/null +++ b/ChangeLog.d/dynamic-keystore.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix interference between PSA volatile keys and built-in keys + when MBEDTLS_PSA_CRYPTO_BUILTIN_KEYS is enabled and + MBEDTLS_PSA_KEY_SLOT_COUNT is more than 4096. diff --git a/library/psa_crypto_slot_management.h b/library/psa_crypto_slot_management.h index a84be7d837..88b7c837cc 100644 --- a/library/psa_crypto_slot_management.h +++ b/library/psa_crypto_slot_management.h @@ -15,7 +15,7 @@ /** Range of volatile key identifiers. * - * The last #MBEDTLS_PSA_KEY_SLOT_COUNT identifiers of the implementation + * The first #MBEDTLS_PSA_KEY_SLOT_COUNT identifiers of the implementation * range of key identifiers are reserved for volatile key identifiers. * A volatile key identifier is equal to #PSA_KEY_ID_VOLATILE_MIN plus the * index of the key slot containing the volatile key definition. @@ -23,12 +23,12 @@ /** The minimum value for a volatile key identifier. */ -#define PSA_KEY_ID_VOLATILE_MIN (PSA_KEY_ID_VENDOR_MAX - \ - MBEDTLS_PSA_KEY_SLOT_COUNT + 1) +#define PSA_KEY_ID_VOLATILE_MIN PSA_KEY_ID_VENDOR_MIN /** The maximum value for a volatile key identifier. */ -#define PSA_KEY_ID_VOLATILE_MAX PSA_KEY_ID_VENDOR_MAX +#define PSA_KEY_ID_VOLATILE_MAX \ + (PSA_KEY_ID_VOLATILE_MIN + MBEDTLS_PSA_KEY_SLOT_COUNT - 1) /** Test whether a key identifier is a volatile key identifier. * From 0d0f4adb41430a0d9b29d29c3d065d8493b1bbcc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Jul 2024 12:34:19 +0200 Subject: [PATCH 09/17] Update invalid key id in a test case PSA_KEY_ID_VOLATILE_MIN-1 is now in the persistent key ID range, so it's no longer an invalid key ID for registration. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto_se_driver_hal.data | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.data b/tests/suites/test_suite_psa_crypto_se_driver_hal.data index d028b21821..ae4ee0c25c 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.data +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.data @@ -178,8 +178,8 @@ register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:7:PSA_KEY_ID_VENDOR_MAX+1:-1 Key registration: key id min vendor register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:7:PSA_KEY_ID_VENDOR_MIN:1:PSA_ERROR_INVALID_ARGUMENT -Key registration: key id max vendor except volatile -register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:7:PSA_KEY_ID_VOLATILE_MIN-1:1:PSA_ERROR_INVALID_ARGUMENT +Key registration: key id max vendor +register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:7:PSA_KEY_ID_VENDOR_MAX:1:PSA_ERROR_INVALID_ARGUMENT Key registration: key id min volatile register_key_smoke_test:TEST_SE_PERSISTENT_LIFETIME:7:PSA_KEY_ID_VOLATILE_MIN:1:PSA_ERROR_INVALID_ARGUMENT From e7624cabfbae8fafddb3cdc42f98eef51d9d78fc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Jul 2024 21:29:13 +0200 Subject: [PATCH 10/17] Improve the documentation of MBEDTLS_PSA_KEY_SLOT_COUNT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The description was misleading: setting the option doesn't “restrict” the number of slots, that restriction exists anyway. Setting the option merely determines the value of the limit. Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 35921412c6..ce9a52478d 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -4025,13 +4025,18 @@ //#define MBEDTLS_PSA_HMAC_DRBG_MD_TYPE MBEDTLS_MD_SHA256 /** \def MBEDTLS_PSA_KEY_SLOT_COUNT - * Restrict the PSA library to supporting a maximum amount of simultaneously - * loaded keys. A loaded key is a key stored by the PSA Crypto core as a - * volatile key, or a persistent key which is loaded temporarily by the - * library as part of a crypto operation in flight. * - * If this option is unset, the library will fall back to a default value of - * 32 keys. + * The maximum amount of PSA keys simultaneously in memory. This counts all + * volatile keys, plus loaded persistent keys. + * + * Currently, persistent keys do not need to be loaded all the time while + * a multipart operation is in progress, only while the operation is being + * set up. This may change in future versions of the library. + * + * Currently, the library traverses of the whole table on each access to a + * persistent key. Therefore large values may cause poor performance. + * + * This option has no effect when #MBEDTLS_PSA_CRYPTO_C is disabled. */ //#define MBEDTLS_PSA_KEY_SLOT_COUNT 32 From 8a13d8297b63120bf70f3d50828cb6eba2bb9cc5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 21 Jun 2024 00:09:07 +0200 Subject: [PATCH 11/17] Improve full-key-store tests Split the "many transient keys" test function in two: one that expects to successfully create many keys, and one that expects to fill the key store. This will make things easier when we add a dynamic key store where filling the key store is not practical unless artificially limited. Signed-off-by: Gilles Peskine --- ...test_suite_psa_crypto_slot_management.data | 19 +++- ..._suite_psa_crypto_slot_management.function | 102 +++++++++++++++++- 2 files changed, 115 insertions(+), 6 deletions(-) diff --git a/tests/suites/test_suite_psa_crypto_slot_management.data b/tests/suites/test_suite_psa_crypto_slot_management.data index 7d364acab6..560350c6ee 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.data +++ b/tests/suites/test_suite_psa_crypto_slot_management.data @@ -214,8 +214,23 @@ invalid_handle:INVALID_HANDLE_CLOSED:PSA_ERROR_INVALID_HANDLE invalid handle: huge invalid_handle:INVALID_HANDLE_HUGE:PSA_ERROR_INVALID_HANDLE -Open many transient keys -many_transient_keys:42 +Key slot count: less than maximum +many_transient_keys:MBEDTLS_PSA_KEY_SLOT_COUNT - 1 + +Key slot count: maximum +many_transient_keys:MBEDTLS_PSA_KEY_SLOT_COUNT + +Key slot count: try to overfill, destroy first +fill_key_store:0 + +Key slot count: try to overfill, destroy second +fill_key_store:1 + +Key slot count: try to overfill, destroy next-to-last +fill_key_store:-2 + +Key slot count: try to overfill, destroy last +fill_key_store:-1 # Eviction from a key slot to be able to import a new persistent key. Key slot eviction to import a new persistent key diff --git a/tests/suites/test_suite_psa_crypto_slot_management.function b/tests/suites/test_suite_psa_crypto_slot_management.function index 94f26f6b42..013945e759 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.function +++ b/tests/suites/test_suite_psa_crypto_slot_management.function @@ -98,6 +98,11 @@ exit: return 0; } +/* Currently, there is always a maximum number of volatile keys that can + * realistically be reached in tests. When we add configurations where this + * is not true, undefine the macro in such configurations. */ +#define MAX_VOLATILE_KEYS MBEDTLS_PSA_KEY_SLOT_COUNT + /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -813,21 +818,19 @@ void many_transient_keys(int max_keys_arg) psa_set_key_type(&attributes, PSA_KEY_TYPE_RAW_DATA); for (i = 0; i < max_keys; i++) { + mbedtls_test_set_step(i); status = psa_import_key(&attributes, (uint8_t *) &i, sizeof(i), &keys[i]); - if (status == PSA_ERROR_INSUFFICIENT_MEMORY) { - break; - } PSA_ASSERT(status); TEST_ASSERT(!mbedtls_svc_key_id_is_null(keys[i])); for (j = 0; j < i; j++) { TEST_ASSERT(!mbedtls_svc_key_id_equal(keys[i], keys[j])); } } - max_keys = i; for (i = 1; i < max_keys; i++) { + mbedtls_test_set_step(i); PSA_ASSERT(psa_close_key(keys[i - 1])); PSA_ASSERT(psa_export_key(keys[i], exported, sizeof(exported), @@ -843,6 +846,97 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MAX_VOLATILE_KEYS */ +void fill_key_store(int key_to_destroy_arg) +{ + mbedtls_svc_key_id_t *keys = NULL; + size_t max_keys = MAX_VOLATILE_KEYS; + size_t i, j; + psa_status_t status; + psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT; + uint8_t exported[sizeof(size_t)]; + size_t exported_length; + + PSA_ASSERT(psa_crypto_init()); + + mbedtls_psa_stats_t stats; + mbedtls_psa_get_stats(&stats); + /* Account for any system-created volatile key, e.g. for the RNG. */ + max_keys -= stats.volatile_slots; + TEST_CALLOC(keys, max_keys + 1); + + psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_EXPORT); + psa_set_key_algorithm(&attributes, 0); + psa_set_key_type(&attributes, PSA_KEY_TYPE_RAW_DATA); + + /* Fill the key store. */ + for (i = 0; i < max_keys; i++) { + mbedtls_test_set_step(i); + status = psa_import_key(&attributes, + (uint8_t *) &i, sizeof(i), + &keys[i]); + PSA_ASSERT(status); + TEST_ASSERT(!mbedtls_svc_key_id_is_null(keys[i])); + for (j = 0; j < i; j++) { + TEST_ASSERT(!mbedtls_svc_key_id_equal(keys[i], keys[j])); + } + } + + /* Attempt to overfill. */ + mbedtls_test_set_step(max_keys); + status = psa_import_key(&attributes, + (uint8_t *) &max_keys, sizeof(max_keys), + &keys[max_keys]); + TEST_EQUAL(status, PSA_ERROR_INSUFFICIENT_MEMORY); + TEST_ASSERT(mbedtls_svc_key_id_is_null(keys[max_keys])); + + /* Check that the keys are not corrupted. */ + for (i = 0; i < max_keys; i++) { + mbedtls_test_set_step(i); + PSA_ASSERT(psa_export_key(keys[i], + exported, sizeof(exported), + &exported_length)); + TEST_MEMORY_COMPARE(exported, exported_length, + (uint8_t *) &i, sizeof(i)); + } + + /* Destroy one key and try again. */ + size_t key_to_destroy = (key_to_destroy_arg >= 0 ? + (size_t) key_to_destroy_arg : + max_keys + key_to_destroy_arg); + mbedtls_svc_key_id_t reused_id = keys[key_to_destroy]; + const uint8_t replacement_value[1] = { 0x64 }; + PSA_ASSERT(psa_destroy_key(keys[key_to_destroy])); + keys[key_to_destroy] = MBEDTLS_SVC_KEY_ID_INIT; + status = psa_import_key(&attributes, + replacement_value, sizeof(replacement_value), + &keys[key_to_destroy]); + PSA_ASSERT(status); + TEST_ASSERT(mbedtls_svc_key_id_equal(reused_id, keys[key_to_destroy])); + + /* Check that the keys are not corrupted and destroy them. */ + for (i = 0; i < max_keys; i++) { + mbedtls_test_set_step(i); + PSA_ASSERT(psa_export_key(keys[i], + exported, sizeof(exported), + &exported_length)); + if (i == key_to_destroy) { + TEST_MEMORY_COMPARE(exported, exported_length, + replacement_value, sizeof(replacement_value)); + } else { + TEST_MEMORY_COMPARE(exported, exported_length, + (uint8_t *) &i, sizeof(i)); + } + PSA_ASSERT(psa_destroy_key(keys[i])); + keys[i] = MBEDTLS_SVC_KEY_ID_INIT; + } + +exit: + PSA_DONE(); + mbedtls_free(keys); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C */ void key_slot_eviction_to_import_new_key(int lifetime_arg) { From f39b2e01907fb3f583a53d1c20cb1d6ca70a9c12 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 13 Jun 2024 20:28:58 +0200 Subject: [PATCH 12/17] Fix spurious test case failure with accelerated AES When the PSA RNG uses AES through a PSA driver, it consumes one volatile key identifier. When MBEDTLS_PSA_KEY_SLOT_DYNAMIC is enabled, that identifier happens to coincide with the key ID value that the test case assumes not to exist. Use a different value that avoids this coincidence. Signed-off-by: Gilles Peskine --- .../test_suite_psa_crypto_slot_management.data | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_psa_crypto_slot_management.data b/tests/suites/test_suite_psa_crypto_slot_management.data index 560350c6ee..742f9b1ace 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.data +++ b/tests/suites/test_suite_psa_crypto_slot_management.data @@ -122,7 +122,18 @@ open_fail:PSA_KEY_ID_VENDOR_MAX + 1:PSA_ERROR_DOES_NOT_EXIST Open failure: invalid identifier (implementation range) depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C -open_fail:PSA_KEY_ID_USER_MAX + 1:PSA_ERROR_DOES_NOT_EXIST +# We need to avoid existing volatile key IDs. Normally there aren't any +# existing volatile keys because the test case doesn't create any, but +# in some configurations, the implementation or a driver creates a +# volatile key during initialization for its own use. At the time of +# writing, this happens in builds where AES uses a PSA driver and the +# PSA RNG uses AES-CTR_DRBG through the PSA AES. +# Pick a key id that's in the middle of the volatile key ID range. +# That works out both when MBEDTLS_PSA_KEY_SLOT_DYNAMIC is enabled and +# volatile key IDs are assigned starting with the lowest value, and when +# MBEDTLS_PSA_KEY_SLOT_DYNAMIC is disabled and volatile key IDs are assigned +# starting with the highest values. +open_fail:(PSA_KEY_ID_VOLATILE_MIN + PSA_KEY_ID_VOLATILE_MAX) / 2:PSA_ERROR_DOES_NOT_EXIST Open failure: non-existent identifier depends_on:MBEDTLS_PSA_CRYPTO_STORAGE_C From d66dc64622aaae8c47500ce081f1d6c103e204e0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 17 Jul 2024 14:00:31 +0200 Subject: [PATCH 13/17] Keep track of PSA keys used interally When PSA uses CTR_DRBG for its random generator and CTR_DRBG uses PSA for AES, as currently implemented, there is one volatile key in permanent use for the CTR_DRBG instance. Account for that in tests that want to know exactly how many volatile keys are in use, or how many volatile keys can be created. Signed-off-by: Gilles Peskine --- tests/include/test/psa_crypto_helpers.h | 20 ++++++++++++++ tests/src/psa_crypto_helpers.c | 12 --------- .../test_suite_psa_crypto_init.function | 26 +++++++++++++++++++ ...test_suite_psa_crypto_slot_management.data | 5 +--- 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/tests/include/test/psa_crypto_helpers.h b/tests/include/test/psa_crypto_helpers.h index fae715ca9a..71ba0fc021 100644 --- a/tests/include/test/psa_crypto_helpers.h +++ b/tests/include/test/psa_crypto_helpers.h @@ -440,4 +440,24 @@ uint64_t mbedtls_test_parse_binary_string(data_t *bin_string); #define AES_PSA_DONE() ((void) 0) #endif /* MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO */ +#if !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) && \ + defined(MBEDTLS_CTR_DRBG_C) && \ + defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) +/* When AES_C is not defined and PSA does not have an external RNG, + * then CTR_DRBG uses PSA to perform AES-ECB. In this scenario 1 key + * slot is used internally from PSA to hold the AES key and it should + * not be taken into account when evaluating remaining open slots. */ +#define MBEDTLS_TEST_PSA_INTERNAL_KEYS_FOR_DRBG 1 +#else +#define MBEDTLS_TEST_PSA_INTERNAL_KEYS_FOR_DRBG 0 +#endif + +/** The number of volatile keys that PSA crypto uses internally. + * + * We expect that many volatile keys to be in use after a successful + * psa_crypto_init(). + */ +#define MBEDTLS_TEST_PSA_INTERNAL_KEYS \ + MBEDTLS_TEST_PSA_INTERNAL_KEYS_FOR_DRBG + #endif /* PSA_CRYPTO_HELPERS_H */ diff --git a/tests/src/psa_crypto_helpers.c b/tests/src/psa_crypto_helpers.c index 1581eecb3b..1069eddfa1 100644 --- a/tests/src/psa_crypto_helpers.c +++ b/tests/src/psa_crypto_helpers.c @@ -74,21 +74,9 @@ const char *mbedtls_test_helper_is_psa_leaking(void) mbedtls_psa_get_stats(&stats); -#if !defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG) && \ - defined(MBEDTLS_CTR_DRBG_C) && \ - defined(MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO) - /* When AES_C is not defined and PSA does not have an external RNG, - * then CTR_DRBG uses PSA to perform AES-ECB. In this scenario 1 key - * slot is used internally from PSA to hold the AES key and it should - * not be taken into account when evaluating remaining open slots. */ if (stats.volatile_slots > 1) { return "A volatile slot has not been closed properly."; } -#else - if (stats.volatile_slots != 0) { - return "A volatile slot has not been closed properly."; - } -#endif if (stats.persistent_slots != 0) { return "A persistent slot has not been closed properly."; } diff --git a/tests/suites/test_suite_psa_crypto_init.function b/tests/suites/test_suite_psa_crypto_init.function index 9ff33a6d84..2fd282ec61 100644 --- a/tests/suites/test_suite_psa_crypto_init.function +++ b/tests/suites/test_suite_psa_crypto_init.function @@ -8,6 +8,23 @@ #include "mbedtls/entropy.h" #include "entropy_poll.h" +static int check_stats(void) +{ + mbedtls_psa_stats_t stats; + mbedtls_psa_get_stats(&stats); + + TEST_EQUAL(stats.volatile_slots, MBEDTLS_TEST_PSA_INTERNAL_KEYS); + TEST_EQUAL(stats.persistent_slots, 0); + TEST_EQUAL(stats.external_slots, 0); + TEST_EQUAL(stats.half_filled_slots, 0); + TEST_EQUAL(stats.locked_slots, 0); + + return 1; + +exit: + return 0; +} + #define ENTROPY_MIN_NV_SEED_SIZE \ MAX(MBEDTLS_ENTROPY_MIN_PLATFORM, MBEDTLS_ENTROPY_BLOCK_SIZE) @@ -187,10 +204,19 @@ void init_deinit(int count) psa_status_t status; int i; for (i = 0; i < count; i++) { + mbedtls_test_set_step(2 * i); status = psa_crypto_init(); PSA_ASSERT(status); + if (!check_stats()) { + goto exit; + } + + mbedtls_test_set_step(2 * i); status = psa_crypto_init(); PSA_ASSERT(status); + if (!check_stats()) { + goto exit; + } PSA_DONE(); } } diff --git a/tests/suites/test_suite_psa_crypto_slot_management.data b/tests/suites/test_suite_psa_crypto_slot_management.data index 742f9b1ace..af3b946754 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.data +++ b/tests/suites/test_suite_psa_crypto_slot_management.data @@ -225,11 +225,8 @@ invalid_handle:INVALID_HANDLE_CLOSED:PSA_ERROR_INVALID_HANDLE invalid handle: huge invalid_handle:INVALID_HANDLE_HUGE:PSA_ERROR_INVALID_HANDLE -Key slot count: less than maximum -many_transient_keys:MBEDTLS_PSA_KEY_SLOT_COUNT - 1 - Key slot count: maximum -many_transient_keys:MBEDTLS_PSA_KEY_SLOT_COUNT +many_transient_keys:MBEDTLS_PSA_KEY_SLOT_COUNT - MBEDTLS_TEST_PSA_INTERNAL_KEYS Key slot count: try to overfill, destroy first fill_key_store:0 From c297c76b5bd8f4f112356f3f464809bf9ad011b6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 18 Jul 2024 19:03:02 +0200 Subject: [PATCH 14/17] Fix copypasta Signed-off-by: Gilles Peskine --- include/mbedtls/ctr_drbg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 216169c770..0b7cce1923 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -41,7 +41,7 @@ * When MBEDTLS_AES_C is disabled, the CTR_DRBG module calls PSA crypto and * thus benefits from the PSA AES accelerator driver. * It is technically possible to enable MBEDTLS_CTR_DRBG_USE_PSA_CRYPTO - * to use PSA even when MBEDTLS_AES_C is disabled, but there is very little + * to use PSA even when MBEDTLS_AES_C is enabled, but there is very little * reason to do so other than testing purposes and this is not officially * supported. */ From 5eca4029c2ef0026354e0be13e4af44f502ed782 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Aug 2024 20:08:23 +0200 Subject: [PATCH 15/17] Fix inverted assertion message Signed-off-by: Gilles Peskine --- library/psa_crypto_slot_management.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index c2949a61e7..9b297c9c08 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -67,7 +67,7 @@ static psa_global_data_t global_data; MBEDTLS_STATIC_ASSERT(ARRAY_LENGTH(global_data.key_slots) <= PSA_KEY_ID_VOLATILE_MAX - PSA_KEY_ID_VOLATILE_MIN + 1, - "The volatile key range is larger than the key slot array"); + "The key slot array is larger than the volatile key ID range"); static uint8_t psa_get_key_slots_initialized(void) { From a9083b752c76af901012e7ddad41fdfdaa21a618 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Aug 2024 20:09:08 +0200 Subject: [PATCH 16/17] PSA_DONE: account for MBEDTLS_TEST_PSA_INTERNAL_KEYS Replace the hard-coded 1 by the proper constant now that the proper constant exists. Signed-off-by: Gilles Peskine --- tests/src/psa_crypto_helpers.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/src/psa_crypto_helpers.c b/tests/src/psa_crypto_helpers.c index 1069eddfa1..197fd41980 100644 --- a/tests/src/psa_crypto_helpers.c +++ b/tests/src/psa_crypto_helpers.c @@ -74,7 +74,12 @@ const char *mbedtls_test_helper_is_psa_leaking(void) mbedtls_psa_get_stats(&stats); - if (stats.volatile_slots > 1) { + /* Some volatile slots may be used for internal purposes. Generally + * we'll have exactly MBEDTLS_TEST_PSA_INTERNAL_KEYS at this point, + * but in some cases we might have less, e.g. if a code path calls + * PSA_DONE more than once, or if there has only been a partial or + * failed initialization. */ + if (stats.volatile_slots > MBEDTLS_TEST_PSA_INTERNAL_KEYS) { return "A volatile slot has not been closed properly."; } if (stats.persistent_slots != 0) { From aaa96721d11510633299c569a88812feed5bb3bd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Aug 2024 20:09:49 +0200 Subject: [PATCH 17/17] Improve documentation in some tests Signed-off-by: Gilles Peskine --- tests/suites/test_suite_psa_crypto_init.function | 2 ++ ...test_suite_psa_crypto_slot_management.function | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/tests/suites/test_suite_psa_crypto_init.function b/tests/suites/test_suite_psa_crypto_init.function index 2fd282ec61..954560a24e 100644 --- a/tests/suites/test_suite_psa_crypto_init.function +++ b/tests/suites/test_suite_psa_crypto_init.function @@ -219,6 +219,8 @@ void init_deinit(int count) } PSA_DONE(); } +exit: + PSA_DONE(); } /* END_CASE */ diff --git a/tests/suites/test_suite_psa_crypto_slot_management.function b/tests/suites/test_suite_psa_crypto_slot_management.function index 013945e759..f679f2e889 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.function +++ b/tests/suites/test_suite_psa_crypto_slot_management.function @@ -847,6 +847,16 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MAX_VOLATILE_KEYS */ +/* + * 1. Fill the key store with volatile keys. + * 2. Check that attempting to create another volatile key fails without + * corrupting the key store. + * 3. Destroy the key specified by key_to_destroy. This is the number of the + * key in creation order (e.g. 0 means the first key that was created). + * It can also be a negative value to count in reverse order (e.g. + * -1 means to destroy the last key that was created). + * 4. Check that creating another volatile key succeeds. + */ void fill_key_store(int key_to_destroy_arg) { mbedtls_svc_key_id_t *keys = NULL; @@ -912,6 +922,11 @@ void fill_key_store(int key_to_destroy_arg) replacement_value, sizeof(replacement_value), &keys[key_to_destroy]); PSA_ASSERT(status); + /* Since the key store was full except for one key, the new key must be + * in the same slot in the key store as the destroyed key. + * Since volatile keys IDs are assigned based on which slot contains + * the key, the new key should have the same ID as the destroyed key. + */ TEST_ASSERT(mbedtls_svc_key_id_equal(reused_id, keys[key_to_destroy])); /* Check that the keys are not corrupted and destroy them. */