From f555a4e26f9909322e0cb382c5133ee403c9d674 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Jun 2024 19:13:19 +0200 Subject: [PATCH 1/9] 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 | 15 ++++++++------- library/psa_crypto.c | 6 +++--- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/library/common.h b/library/common.h index 49e2c97ea049..5565b307cdd1 100644 --- a/library/common.h +++ b/library/common.h @@ -337,17 +337,18 @@ static inline const unsigned char *mbedtls_buffer_offset_const( #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 will expand to nothing on some systems. */ +/* 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 /* Suppress compiler warnings for unused functions and variables. */ diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 46e1b3723fd4..45e1aa9d6571 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1622,11 +1622,11 @@ psa_status_t psa_export_public_key(mbedtls_svc_key_id_t key, } MBEDTLS_STATIC_ASSERT((MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0, - "One or more key attribute flag is listed as both external-only and dual-use") + "One or more key attribute flag is listed as both external-only and dual-use"); MBEDTLS_STATIC_ASSERT((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_DUAL_USE) == 0, - "One or more key attribute flag is listed as both internal-only and dual-use") + "One or more key attribute flag is listed as both internal-only and dual-use"); MBEDTLS_STATIC_ASSERT((PSA_KA_MASK_INTERNAL_ONLY & MBEDTLS_PSA_KA_MASK_EXTERNAL_ONLY) == 0, - "One or more key attribute flag is listed as both internal-only and external-only") + "One or more key attribute flag is listed as both internal-only and external-only"); /** Validate that a key policy is internally well-formed. * From 37a4fcc5b4b22d39c78e2bcd82c994e3b4629762 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 13 Jun 2024 16:06:45 +0200 Subject: [PATCH 2/9] 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 000000000000..2fc2751ac03f --- /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 a2d547f5cf06..5096748e924b 100644 --- a/include/psa/crypto.h +++ b/include/psa/crypto.h @@ -130,6 +130,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 a1b2af7a7303..4039acf16d16 100644 --- a/include/psa/crypto_extra.h +++ b/include/psa/crypto_extra.h @@ -155,6 +155,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 45e1aa9d6571..502ddc274b48 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 2bcf4e4b7bc1..dbe22b813ea3 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.data +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.data @@ -147,7 +147,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 91773db3315f07d3a162bb997b650db767bb830e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Jun 2024 22:10:08 +0200 Subject: [PATCH 3/9] 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 4039acf16d16..ea58e87b5964 100644 --- a/include/psa/crypto_extra.h +++ b/include/psa/crypto_extra.h @@ -717,7 +717,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 a58f0835b261..f1a0bf075d91 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: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 eba637f2235f..709fe4fbd918 100644 --- a/tests/suites/test_suite_psa_crypto_driver_wrappers.function +++ b/tests/suites/test_suite_psa_crypto_driver_wrappers.function @@ -7,6 +7,21 @@ * 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 f16263e286deb8ce660a2ccec1a8072ebf78f8fc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 20 Jun 2024 22:15:42 +0200 Subject: [PATCH 4/9] 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 b79c713abbab..f0cb4ee9f6ee 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -26,6 +26,37 @@ #define ARRAY_LENGTH(array) (sizeof(array) / sizeof(*(array))) + + +/* 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]; unsigned key_slots_initialized : 1; From 18f659b1e79a55c4094788c197545ccc11b6660c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Jul 2024 20:02:37 +0200 Subject: [PATCH 5/9] 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 f0cb4ee9f6ee..2c4da7833b73 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -64,6 +64,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"); + int psa_is_valid_key_id(mbedtls_svc_key_id_t key, int vendor_ok) { psa_key_id_t key_id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(key); From 70de13d92002aea0a749a6c66cec334ba9869e8a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 Jul 2024 21:29:13 +0200 Subject: [PATCH 6/9] 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/config.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 4842fd494c9b..5b100781b73a 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -4029,13 +4029,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 3a51fdc8c61054ccd76bd069e50feb6733fb1db9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 21 Jun 2024 00:09:07 +0200 Subject: [PATCH 7/9] 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 1477734165c5..a731044f017c 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 5bd12eb09ee7..c0e980db2288 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 @@ -821,21 +826,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), @@ -851,6 +854,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 d0ba2b0d1f78db51acee98f9032c0ac677d55a0a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Aug 2024 20:08:23 +0200 Subject: [PATCH 8/9] 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 2c4da7833b73..1a5442066519 100644 --- a/library/psa_crypto_slot_management.c +++ b/library/psa_crypto_slot_management.c @@ -66,7 +66,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"); int psa_is_valid_key_id(mbedtls_svc_key_id_t key, int vendor_ok) { From 4c9d43fb181637233490aadb86eb68941a581b89 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Aug 2024 20:09:49 +0200 Subject: [PATCH 9/9] 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 63767f020216..eb6bb1a4698c 100644 --- a/tests/suites/test_suite_psa_crypto_init.function +++ b/tests/suites/test_suite_psa_crypto_init.function @@ -161,6 +161,8 @@ void init_deinit(int count) PSA_ASSERT(status); 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 c0e980db2288..ee214ea247c3 100644 --- a/tests/suites/test_suite_psa_crypto_slot_management.function +++ b/tests/suites/test_suite_psa_crypto_slot_management.function @@ -855,6 +855,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; @@ -920,6 +930,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. */