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/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 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..ea58e87b5964 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. @@ -709,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/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 a58033395ec3..21d8420163ca 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. * @@ -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/library/psa_crypto_slot_management.c b/library/psa_crypto_slot_management.c index b79c713abbab..1a5442066519 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; @@ -33,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 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) { psa_key_id_t key_id = MBEDTLS_SVC_KEY_ID_GET_KEY_ID(key); 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, 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_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) 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..ee214ea247c3 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,112 @@ 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; + 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); + /* 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. */ + 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) {