From a7a480bb81d7a7139c5933c52216211f5a3402c8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 5 Feb 2025 19:41:52 +0100 Subject: [PATCH 1/6] Initialize driver context in setup functions In API functions that set up a multipart or interruptible operation, make sure to initialize the driver-specific part of the context. This is a union, and initializing the union to `{0}` only guarantees that the first member of the union is initialized, not necessarily the member used by the driver. Most compilers do initialize the whole union to all-bits-zero, but some don't. With compilers that don't, the lack of initialization caused failures of built-in MAC, interruptible-sign and interruptible-verify. It could also cause failures for other operations with third-party drivers: we promise that drivers' setup entry points receive a zero-initialized operation structure, but this promise was not kept. Signed-off-by: Gilles Peskine --- library/psa_crypto.c | 56 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index e7f99ad3bc..f0434114ea 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -2400,8 +2400,11 @@ psa_status_t psa_hash_setup(psa_hash_operation_t *operation, goto exit; } - /* Ensure all of the context is zeroized, since PSA_HASH_OPERATION_INIT only - * directly zeroes the int-sized dummy member of the context union. */ + /* Make sure the driver-dependent part of the operation is zeroed. + * This is a guarantee we make to drivers. Initializing the operation + * does not necessarily take care of it, since the context is a + * union and initializing a union does not necessarily initialize + * all of its members. */ memset(&operation->ctx, 0, sizeof(operation->ctx)); status = psa_driver_wrapper_hash_setup(operation, alg); @@ -2596,6 +2599,13 @@ psa_status_t psa_hash_clone(const psa_hash_operation_t *source_operation, return PSA_ERROR_BAD_STATE; } + /* Make sure the driver-dependent part of the operation is zeroed. + * This is a guarantee we make to drivers. Initializing the operation + * does not necessarily take care of it, since the context is a + * union and initializing a union does not necessarily initialize + * all of its members. */ + memset(&target_operation->ctx, 0, sizeof(target_operation->ctx)); + psa_status_t status = psa_driver_wrapper_hash_clone(source_operation, target_operation); if (status != PSA_SUCCESS) { @@ -2693,6 +2703,13 @@ static psa_status_t psa_mac_setup(psa_mac_operation_t *operation, goto exit; } + /* Make sure the driver-dependent part of the operation is zeroed. + * This is a guarantee we make to drivers. Initializing the operation + * does not necessarily take care of it, since the context is a + * union and initializing a union does not necessarily initialize + * all of its members. */ + memset(&operation->ctx, 0, sizeof(operation->ctx)); + status = psa_get_and_lock_key_slot_with_policy( key, &slot, @@ -3619,6 +3636,13 @@ psa_status_t psa_sign_hash_start( return PSA_ERROR_BAD_STATE; } + /* Make sure the driver-dependent part of the operation is zeroed. + * This is a guarantee we make to drivers. Initializing the operation + * does not necessarily take care of it, since the context is a + * union and initializing a union does not necessarily initialize + * all of its members. */ + memset(&operation->ctx, 0, sizeof(operation->ctx)); + status = psa_sign_verify_check_alg(0, alg); if (status != PSA_SUCCESS) { operation->error_occurred = 1; @@ -3779,6 +3803,13 @@ psa_status_t psa_verify_hash_start( return PSA_ERROR_BAD_STATE; } + /* Make sure the driver-dependent part of the operation is zeroed. + * This is a guarantee we make to drivers. Initializing the operation + * does not necessarily take care of it, since the context is a + * union and initializing a union does not necessarily initialize + * all of its members. */ + memset(&operation->ctx, 0, sizeof(operation->ctx)); + status = psa_sign_verify_check_alg(0, alg); if (status != PSA_SUCCESS) { operation->error_occurred = 1; @@ -4446,6 +4477,14 @@ static psa_status_t psa_cipher_setup(psa_cipher_operation_t *operation, } operation->default_iv_length = PSA_CIPHER_IV_LENGTH(slot->attr.type, alg); + + /* Make sure the driver-dependent part of the operation is zeroed. + * This is a guarantee we make to drivers. Initializing the operation + * does not necessarily take care of it, since the context is a + * union and initializing a union does not necessarily initialize + * all of its members. */ + memset(&operation->ctx, 0, sizeof(operation->ctx)); + /* Try doing the operation through a driver before using software fallback. */ if (cipher_operation == MBEDTLS_ENCRYPT) { status = psa_driver_wrapper_cipher_encrypt_setup(operation, @@ -5079,6 +5118,13 @@ static psa_status_t psa_aead_setup(psa_aead_operation_t *operation, goto exit; } + /* Make sure the driver-dependent part of the operation is zeroed. + * This is a guarantee we make to drivers. Initializing the operation + * does not necessarily take care of it, since the context is a + * union and initializing a union does not necessarily initialize + * all of its members. */ + memset(&operation->ctx, 0, sizeof(operation->ctx)); + if (is_encrypt) { key_usage = PSA_KEY_USAGE_ENCRYPT; } else { @@ -8629,7 +8675,11 @@ psa_status_t psa_pake_setup( goto exit; } - memset(&operation->data.inputs, 0, sizeof(operation->data.inputs)); + /* Make sure the variable-purpose part of the operation is zeroed. + * Initializing the operation does not necessarily take care of it, + * since the context is a union and initializing a union does not + * necessarily initialize all of its members. */ + memset(&operation->data, 0, sizeof(operation->data)); operation->alg = cipher_suite->algorithm; operation->primitive = PSA_PAKE_PRIMITIVE(cipher_suite->type, From bbec1c1d25c848ed3d3d5c82864feb13eedf9cfd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 5 Feb 2025 19:53:28 +0100 Subject: [PATCH 2/6] Initialize MAC context in internal functions for KDF In functions that bypass the API functions and call the MAC driver wrapper `psa_driver_wrapper_mac_sign_setup()` directly, make sure to initialize the driver-specific part of the context. This is a union, and initializing the union to `{0}` only guarantees that the first member of the union is initialized, not necessarily the member used by the driver. Most compilers do initialize the whole union to all-bits-zero, but some don't. With compilers that don't, the lack of initialization caused failures of the affected operations. This affected several key derivation operations. Signed-off-by: Gilles Peskine --- library/psa_crypto.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index f0434114ea..3342b1b47f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5645,6 +5645,17 @@ psa_status_t psa_aead_abort(psa_aead_operation_t *operation) #if defined(BUILTIN_ALG_ANY_HKDF) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_TLS12_PRF) || \ defined(MBEDTLS_PSA_BUILTIN_ALG_TLS12_PSK_TO_MS) + +/** Internal helper to set up an HMAC operation with a key passed directly. + * + * \param[in,out] operation A MAC operation object. It does not need to + * be initialized. + * \param hash_alg The hash algorithm used for HMAC. + * \param hmac_key The HMAC key. + * \param hmac_key_length Length of \p hmac_key in bytes. + * + * \return A PSA status code. + */ static psa_status_t psa_key_derivation_start_hmac( psa_mac_operation_t *operation, psa_algorithm_t hash_alg, @@ -5657,6 +5668,14 @@ static psa_status_t psa_key_derivation_start_hmac( psa_set_key_bits(&attributes, PSA_BYTES_TO_BITS(hmac_key_length)); psa_set_key_usage_flags(&attributes, PSA_KEY_USAGE_SIGN_HASH); + /* Make sure the whole the operation is zeroed. + * It isn't enough to require the caller to initialize operation to + * PSA_MAC_OPERATION_INIT, since one field is a union and initializing + * a union does not necessarily initialize all of its members. + * psa_mac_setup() would could handle PSA_MAC_OPERATION_INIT, but here we + * bypass it and call lower-level functions directly. */ + memset(operation, 0, sizeof(*operation)); + operation->is_sign = 1; operation->mac_size = PSA_HASH_LENGTH(hash_alg); @@ -5881,7 +5900,7 @@ static psa_status_t psa_key_derivation_tls12_prf_generate_next_block( { psa_algorithm_t hash_alg = PSA_ALG_HKDF_GET_HASH(alg); uint8_t hash_length = PSA_HASH_LENGTH(hash_alg); - psa_mac_operation_t hmac = PSA_MAC_OPERATION_INIT; + psa_mac_operation_t hmac; size_t hmac_output_length; psa_status_t status, cleanup_status; @@ -6082,7 +6101,14 @@ static psa_status_t psa_key_derivation_pbkdf2_generate_block( psa_key_attributes_t *attributes) { psa_status_t status; - psa_mac_operation_t mac_operation = PSA_MAC_OPERATION_INIT; + psa_mac_operation_t mac_operation; + /* Make sure the whole the operation is zeroed. + * PSA_MAC_OPERATION_INIT does not necessarily do it fully, + * since one field is a union and initializing a union does not + * necessarily initialize all of its members. + * psa_mac_setup() would do it, but here we bypass it and call + * lower-level functions directly. */ + memset(&mac_operation, 0, sizeof(mac_operation)); size_t mac_output_length; uint8_t U_i[PSA_MAC_MAX_SIZE]; uint8_t *U_accumulator = pbkdf2->output_block; From 0e4907d4f5d27305e6ee9d06be7a38efbdd68e1c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 5 Feb 2025 19:53:54 +0100 Subject: [PATCH 3/6] Initialize MAC context in internal functions for one-shot MAC In functions that bypass the API functions and call an internal MAC setup function directly, make sure to initialize the driver-specific part of the context. This is a union, and initializing the union to `{0}` only guarantees that the first member of the union is initialized, not necessarily the member used by the driver. Most compilers do initialize the whole union to all-bits-zero, but some don't. With compilers that don't, the lack of initialization caused failures of the affected operations. This affected one-shot MAC operations using the built-in implementation. Signed-off-by: Gilles Peskine --- library/psa_crypto_mac.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/library/psa_crypto_mac.c b/library/psa_crypto_mac.c index 8fe6218118..7f625c549d 100644 --- a/library/psa_crypto_mac.c +++ b/library/psa_crypto_mac.c @@ -465,6 +465,15 @@ psa_status_t mbedtls_psa_mac_compute( { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; mbedtls_psa_mac_operation_t operation = MBEDTLS_PSA_MAC_OPERATION_INIT; + /* Make sure the whole the operation is zeroed. + * PSA_MAC_OPERATION_INIT does not necessarily do it fully, + * since one field is a union and initializing a union does not + * necessarily initialize all of its members. + * In multipart operations, this is done in the API functions, + * before driver dispatch, since it needs to be done before calling + * the driver entry point. Here, we bypass the multipart API, + * so it's our job. */ + memset(&operation, 0, sizeof(operation)); status = psa_mac_setup(&operation, attributes, key_buffer, key_buffer_size, From f4ebf807e665d259a9e75894dfd8562f343d7180 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 May 2025 18:50:51 +0200 Subject: [PATCH 4/6] Test with GCC 15 with sloppy union initialization This is a non-regression test for https://github.com/Mbed-TLS/mbedtls/issues/9814 Signed-off-by: Gilles Peskine --- tests/scripts/components-compiler.sh | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/scripts/components-compiler.sh b/tests/scripts/components-compiler.sh index d69d529bf2..286408717e 100644 --- a/tests/scripts/components-compiler.sh +++ b/tests/scripts/components-compiler.sh @@ -94,10 +94,7 @@ component_test_gcc15_drivers_opt () { loc_cflags="$ASAN_CFLAGS -DPSA_CRYPTO_DRIVER_TEST_ALL" loc_cflags="${loc_cflags} '-DMBEDTLS_USER_CONFIG_FILE=\"../tests/configs/user-config-for-test.h\"'" loc_cflags="${loc_cflags} -I../framework/tests/include -O2" - # Until https://github.com/Mbed-TLS/mbedtls/issues/9814 is fixed, - # disable the new problematic optimization. - loc_cflags="${loc_cflags} -fzero-init-padding-bits=unions" - # Also allow a warning that we don't yet comply to. + # Allow a warning that we don't yet comply to. # https://github.com/Mbed-TLS/mbedtls/issues/9944 loc_cflags="${loc_cflags} -Wno-error=unterminated-string-initialization" From 65b548386f0ad8c451b04f470a474a74c4045317 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 5 Feb 2025 20:33:15 +0100 Subject: [PATCH 5/6] Changelog entry for the union initialization fixes Signed-off-by: Gilles Peskine --- ChangeLog.d/union-initialization.txt | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 ChangeLog.d/union-initialization.txt diff --git a/ChangeLog.d/union-initialization.txt b/ChangeLog.d/union-initialization.txt new file mode 100644 index 0000000000..a63e1ebc00 --- /dev/null +++ b/ChangeLog.d/union-initialization.txt @@ -0,0 +1,15 @@ +Bugfix + * Fix failures of PSA multipart or interruptible operations when the + library or the application is built with a compiler where + "union foo x = {0}" does not initialize non-default members of the + union, such as GCC 15 and some versions of Clang 18. This affected MAC + multipart operations, MAC-based key derivation operations, interruptible + signature, interruptible verification, and potentially other operations + when using third-party drivers. This also affected one-shot MAC + operations using the built-in implementation. Fixes #9814. + * On entry to PSA driver entry points that set up a multipart operation + ("xxx_setup"), the operation object is supposed to be all-bits-zero. + This was sometimes not the case when an operation object is reused, + or with compilers where "union foo x = {0}" does not initialize + non-default members of the union. The PSA core now ensures that this + guarantee is met in all cases. Fixes #9975. From 91b29a0bd91454b4b3fe58b3ba141e080ce47444 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 13 May 2025 11:53:31 +0200 Subject: [PATCH 6/6] Grammar in comments Signed-off-by: Gilles Peskine --- library/psa_crypto.c | 2 +- library/psa_crypto_mac.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 3342b1b47f..9c28609d7e 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5672,7 +5672,7 @@ static psa_status_t psa_key_derivation_start_hmac( * It isn't enough to require the caller to initialize operation to * PSA_MAC_OPERATION_INIT, since one field is a union and initializing * a union does not necessarily initialize all of its members. - * psa_mac_setup() would could handle PSA_MAC_OPERATION_INIT, but here we + * psa_mac_setup() would handle PSA_MAC_OPERATION_INIT, but here we * bypass it and call lower-level functions directly. */ memset(operation, 0, sizeof(*operation)); diff --git a/library/psa_crypto_mac.c b/library/psa_crypto_mac.c index 7f625c549d..4464158f93 100644 --- a/library/psa_crypto_mac.c +++ b/library/psa_crypto_mac.c @@ -465,7 +465,7 @@ psa_status_t mbedtls_psa_mac_compute( { psa_status_t status = PSA_ERROR_CORRUPTION_DETECTED; mbedtls_psa_mac_operation_t operation = MBEDTLS_PSA_MAC_OPERATION_INIT; - /* Make sure the whole the operation is zeroed. + /* Make sure the whole operation is zeroed. * PSA_MAC_OPERATION_INIT does not necessarily do it fully, * since one field is a union and initializing a union does not * necessarily initialize all of its members.