From 91577020a2a134b51bdeec0c1c9da9c7b82ea3c5 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 11 Oct 2024 14:55:24 +0200 Subject: [PATCH 1/7] pkwrite: fix buffer overrun This commit fixes potential buffer overrun in: - pk_write_rsa_der - pk_write_ec_pubkey In both functions, when dealing with opaque keys, there was no check that the provided buffer was large enough to contain the key being exported. This commit fixes this problem and it also adds some testing in test_suite_pkwrite to trigger these checks. Signed-off-by: Valerio Setti --- tf-psa-crypto/drivers/builtin/src/pkwrite.c | 14 +++++++++++--- .../tests/suites/test_suite_pkwrite.function | 19 ++++++++++++++++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/tf-psa-crypto/drivers/builtin/src/pkwrite.c b/tf-psa-crypto/drivers/builtin/src/pkwrite.c index 8c01b440ae..0b57995849 100644 --- a/tf-psa-crypto/drivers/builtin/src/pkwrite.c +++ b/tf-psa-crypto/drivers/builtin/src/pkwrite.c @@ -65,17 +65,21 @@ static int pk_write_rsa_der(unsigned char **p, unsigned char *buf, #if defined(MBEDTLS_USE_PSA_CRYPTO) if (mbedtls_pk_get_type(pk) == MBEDTLS_PK_OPAQUE) { uint8_t tmp[PSA_EXPORT_KEY_PAIR_MAX_SIZE]; - size_t len = 0, tmp_len = 0; + size_t tmp_len = 0; if (psa_export_key(pk->priv_id, tmp, sizeof(tmp), &tmp_len) != PSA_SUCCESS) { return MBEDTLS_ERR_PK_BAD_INPUT_DATA; } + /* Ensure there's enough space in the provided buffer before copying data into it. */ + if (tmp_len > (size_t) (*p - buf)) { + mbedtls_platform_zeroize(tmp, sizeof(tmp)); + return MBEDTLS_ERR_ASN1_BUF_TOO_SMALL; + } *p -= tmp_len; memcpy(*p, tmp, tmp_len); - len += tmp_len; mbedtls_platform_zeroize(tmp, sizeof(tmp)); - return (int) len; + return (int) tmp_len; } #endif /* MBEDTLS_USE_PSA_CRYPTO */ return mbedtls_rsa_write_key(mbedtls_pk_rsa(*pk), buf, p); @@ -125,6 +129,10 @@ static int pk_write_ec_pubkey(unsigned char **p, unsigned char *start, if (psa_export_public_key(pk->priv_id, buf, sizeof(buf), &len) != PSA_SUCCESS) { return MBEDTLS_ERR_PK_BAD_INPUT_DATA; } + /* Ensure there's enough space in the provided buffer before copying data into it. */ + if (len > (size_t) (*p - start)) { + return MBEDTLS_ERR_ASN1_BUF_TOO_SMALL; + } *p -= len; memcpy(*p, buf, len); return (int) len; diff --git a/tf-psa-crypto/tests/suites/test_suite_pkwrite.function b/tf-psa-crypto/tests/suites/test_suite_pkwrite.function index 735c12547c..3392528115 100644 --- a/tf-psa-crypto/tests/suites/test_suite_pkwrite.function +++ b/tf-psa-crypto/tests/suites/test_suite_pkwrite.function @@ -2,6 +2,7 @@ #include "pk_internal.h" #include "mbedtls/pem.h" #include "mbedtls/oid.h" +#include "mbedtls/base64.h" #include "psa/crypto_sizes.h" typedef enum { @@ -72,7 +73,8 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der) unsigned char *buf = NULL; unsigned char *check_buf = NULL; unsigned char *start_buf; - size_t buf_len, check_buf_len; + size_t buf_len, check_buf_len, wrong_buf_len = 1; + int expected_result; #if defined(MBEDTLS_USE_PSA_CRYPTO) mbedtls_svc_key_id_t opaque_id = MBEDTLS_SVC_KEY_ID_INIT; psa_key_attributes_t key_attr = PSA_KEY_ATTRIBUTES_INIT; @@ -109,6 +111,17 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der) start_buf = buf; buf_len = check_buf_len; + if (is_der) { + expected_result = MBEDTLS_ERR_ASN1_BUF_TOO_SMALL; + } else { + expected_result = MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL; + } + /* Intentionally pass a wrong size for the provided output buffer and check + * that the writing functions fails as expected. */ + TEST_EQUAL(pk_write_any_key(&key, &start_buf, &wrong_buf_len, is_public_key, + is_der), expected_result); + TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key, + is_der), 0); TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key, is_der), 0); @@ -127,6 +140,10 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der) TEST_EQUAL(mbedtls_pk_setup_opaque(&key, opaque_id), 0); start_buf = buf; buf_len = check_buf_len; + /* Intentionally pass a wrong size for the provided output buffer and check + * that the writing functions fails as expected. */ + TEST_EQUAL(pk_write_any_key(&key, &start_buf, &wrong_buf_len, is_public_key, + is_der), expected_result); TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key, is_der), 0); From 63348bed3064164c02406a9bf3ea21655170b47b Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Mon, 14 Oct 2024 09:44:06 +0200 Subject: [PATCH 2/7] test_suite_pkwrite: extend coverage of wrong output buffer sizes in pk_write_check_common() Signed-off-by: Valerio Setti --- .../tests/suites/test_suite_pkwrite.function | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/tf-psa-crypto/tests/suites/test_suite_pkwrite.function b/tf-psa-crypto/tests/suites/test_suite_pkwrite.function index 3392528115..491bc489aa 100644 --- a/tf-psa-crypto/tests/suites/test_suite_pkwrite.function +++ b/tf-psa-crypto/tests/suites/test_suite_pkwrite.function @@ -73,7 +73,7 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der) unsigned char *buf = NULL; unsigned char *check_buf = NULL; unsigned char *start_buf; - size_t buf_len, check_buf_len, wrong_buf_len = 1; + size_t buf_len, check_buf_len; int expected_result; #if defined(MBEDTLS_USE_PSA_CRYPTO) mbedtls_svc_key_id_t opaque_id = MBEDTLS_SVC_KEY_ID_INIT; @@ -118,10 +118,10 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der) } /* Intentionally pass a wrong size for the provided output buffer and check * that the writing functions fails as expected. */ - TEST_EQUAL(pk_write_any_key(&key, &start_buf, &wrong_buf_len, is_public_key, - is_der), expected_result); - TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key, - is_der), 0); + for (size_t i = 1; i < buf_len; i++) { + TEST_EQUAL(pk_write_any_key(&key, &start_buf, &i, is_public_key, + is_der), expected_result); + } TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key, is_der), 0); @@ -142,8 +142,10 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der) buf_len = check_buf_len; /* Intentionally pass a wrong size for the provided output buffer and check * that the writing functions fails as expected. */ - TEST_EQUAL(pk_write_any_key(&key, &start_buf, &wrong_buf_len, is_public_key, - is_der), expected_result); + for (size_t i = 1; i < buf_len; i++) { + TEST_EQUAL(pk_write_any_key(&key, &start_buf, &i, is_public_key, + is_der), expected_result); + } TEST_EQUAL(pk_write_any_key(&key, &start_buf, &buf_len, is_public_key, is_der), 0); From e298eeb739be318f44c8766ec8987461cc105a32 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 14 Oct 2024 11:03:24 +0200 Subject: [PATCH 3/7] Changelog entry for security fix Signed-off-by: Gilles Peskine --- ChangeLog.d/9690.txt | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 ChangeLog.d/9690.txt diff --git a/ChangeLog.d/9690.txt b/ChangeLog.d/9690.txt new file mode 100644 index 0000000000..8dda75cc7c --- /dev/null +++ b/ChangeLog.d/9690.txt @@ -0,0 +1,8 @@ +Security + * Fix a buffer underrun in mbedtls_pk_write_pubkey_der() when + called on an opaque key, MBEDTLS_USE_PSA_CRYPTO is enabled, + and the output buffer is smaller than the actual output. + Fix a related buffer underrun in mbedtls_pk_write_pubkey_pem() + when called on an opaque RSA key, MBEDTLS_USE_PSA_CRYPTO is enabled + and MBEDTLS_MPI_MAX_SIZE is smaller than needed for a 4096-bit RSA key. + CVE-2024-49195 From a4415d992a6a167991250caba170378585a31a1d Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Fri, 11 Oct 2024 17:24:16 +0100 Subject: [PATCH 4/7] Defer static keystore to 3.6.3 Signed-off-by: David Horstmann --- docs/architecture/psa-keystore-design.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/psa-keystore-design.md b/docs/architecture/psa-keystore-design.md index cdd2cac3ab..be082a812a 100644 --- a/docs/architecture/psa-keystore-design.md +++ b/docs/architecture/psa-keystore-design.md @@ -67,7 +67,7 @@ Note that a slot must not be moved in memory while it is being read or written. There are three variants of the key store implementation, responding to different needs. * Hybrid key store ([static key slots](#static-key-store) with dynamic key data): the key store is a statically allocated array of slots, of size `MBEDTLS_PSA_KEY_SLOT_COUNT`. Key material is allocated on the heap. This is the historical implementation. It remains the default in the Mbed TLS 3.6 long-time support (LTS) branch when using a handwritten `mbedtls_config.h`, as is common on resource-constrained platforms, because the alternatives have tradeoffs (key size limit and larger RAM usage at rest for the static key store, larger code size and more risk due to code complexity for the dynamic key store). -* Fully [static key store](#static-key-store) (since Mbed TLS 3.6.2): the key store is a statically allocated array of slots, of size `MBEDTLS_PSA_KEY_SLOT_COUNT`. Each key slot contains the key representation directly, and the key representation must be no more than `MBEDTLS_PSA_STATIC_KEY_SLOT_BUFFER_SIZE` bytes. This is intended for very constrained devices that do not have a heap. +* Fully [static key store](#static-key-store) (since Mbed TLS 3.6.3): the key store is a statically allocated array of slots, of size `MBEDTLS_PSA_KEY_SLOT_COUNT`. Each key slot contains the key representation directly, and the key representation must be no more than `MBEDTLS_PSA_STATIC_KEY_SLOT_BUFFER_SIZE` bytes. This is intended for very constrained devices that do not have a heap. * [Dynamic key store](#dynamic-key-store) (since Mbed TLS 3.6.1): the key store is dynamically allocated as multiple slices on the heap, with a size that adjusts to the application's usage. Key material is allocated on the heap. Compared to the hybrid key store, the code size and RAM consumption are larger. This is intended for higher-end devices where applications are not expected to have a highly predicatable resource usage. This is the default implementation when using the default `mbedtls_config.h` file, as is common on platforms such as Linux, starting with Mbed TLS 3.6.1. #### Future improvement: merging the key store variants @@ -95,7 +95,7 @@ When creating a volatile key, the slice containing the slot and index of the slo The static key store is the historical implementation. The key store is a statically allocated array of slots, of size `MBEDTLS_PSA_KEY_SLOT_COUNT`. This value is an upper bound for the total number of volatile keys plus loaded keys. -Since Mbed TLS 3.6.2, there are two variants for the static key store: a hybrid variant (default), and a fully-static variant enabled by the configuration option `MBEDTLS_PSA_STATIC_KEY_SLOTS`. The two variants have the same key store management: the only difference is in how the memory for key data is managed. With fully static key slots, the key data is directly inside the slot, and limited to `MBEDTLS_PSA_KEY_SLOT_BUFFER_SIZE` bytes. With the hybrid key store, the slot contains a pointer to the key data, which is allocated on the heap. +Since Mbed TLS 3.6.3, there are two variants for the static key store: a hybrid variant (default), and a fully-static variant enabled by the configuration option `MBEDTLS_PSA_STATIC_KEY_SLOTS`. The two variants have the same key store management: the only difference is in how the memory for key data is managed. With fully static key slots, the key data is directly inside the slot, and limited to `MBEDTLS_PSA_KEY_SLOT_BUFFER_SIZE` bytes. With the hybrid key store, the slot contains a pointer to the key data, which is allocated on the heap. #### Volatile key identifiers in the static key store From 604e4d99dd0163cf6810a05b79ca2b92e6a51a38 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 14 Oct 2024 11:34:18 +0200 Subject: [PATCH 5/7] Fix completion mistake in changelog entry Signed-off-by: Gilles Peskine --- ChangeLog.d/9690.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog.d/9690.txt b/ChangeLog.d/9690.txt index 8dda75cc7c..d00eb16bc9 100644 --- a/ChangeLog.d/9690.txt +++ b/ChangeLog.d/9690.txt @@ -1,8 +1,8 @@ Security - * Fix a buffer underrun in mbedtls_pk_write_pubkey_der() when + * Fix a buffer underrun in mbedtls_pk_write_key_der() when called on an opaque key, MBEDTLS_USE_PSA_CRYPTO is enabled, and the output buffer is smaller than the actual output. - Fix a related buffer underrun in mbedtls_pk_write_pubkey_pem() + Fix a related buffer underrun in mbedtls_pk_write_key_pem() when called on an opaque RSA key, MBEDTLS_USE_PSA_CRYPTO is enabled and MBEDTLS_MPI_MAX_SIZE is smaller than needed for a 4096-bit RSA key. CVE-2024-49195 From c76f82db27bba36f4f5945dca20359a6c48eac7e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 15 Oct 2024 12:03:26 +0200 Subject: [PATCH 6/7] Work around GCC 5 performance problem with Asan+UBSan and -O3 Old GCC versions hit a performance problem with test_suite_pkwrite "Private keey write check EC" tests when building with Asan+UBSan and -O3: those tests take more than 100x time than normal, with test_suite_pkwrite taking >3h on the CI. Observed with GCC 5.4 on Ubuntu 16.04 x86_64 and GCC 6.5 on Ubuntu 18.04 x86_64. GCC 7.5 and above on Ubuntu 18.04 appear fine. To avoid the performance problem, use -O2 instead of -O3 in then "Asan" build type with GCC. It doesn't slow down much even with modern compiler versions. Signed-off-by: Gilles Peskine --- CMakeLists.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8372905d0d..561498c5d4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -243,7 +243,15 @@ if(CMAKE_COMPILER_IS_GNU) set(CMAKE_C_FLAGS_RELEASE "-O2") set(CMAKE_C_FLAGS_DEBUG "-O0 -g3") set(CMAKE_C_FLAGS_COVERAGE "-O0 -g3 --coverage") - set(CMAKE_C_FLAGS_ASAN "-fsanitize=address -fno-common -fsanitize=undefined -fno-sanitize-recover=all -O3") + # Old GCC versions hit a performance problem with test_suite_pkwrite + # "Private keey write check EC" tests when building with Asan+UBSan + # and -O3: those tests take more than 100x time than normal, with + # test_suite_pkwrite taking >3h on the CI. Observed with GCC 5.4 on + # Ubuntu 16.04 x86_64 and GCC 6.5 on Ubuntu 18.04 x86_64. + # GCC 7.5 and above on Ubuntu 18.04 appear fine. + # To avoid the performance problem, we use -O2 here. It doesn't slow + # down much even with modern compiler versions. + set(CMAKE_C_FLAGS_ASAN "-fsanitize=address -fno-common -fsanitize=undefined -fno-sanitize-recover=all -O2") set(CMAKE_C_FLAGS_ASANDBG "-fsanitize=address -fno-common -fsanitize=undefined -fno-sanitize-recover=all -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls") set(CMAKE_C_FLAGS_TSAN "-fsanitize=thread -O3") set(CMAKE_C_FLAGS_TSANDBG "-fsanitize=thread -O1 -g3 -fno-omit-frame-pointer -fno-optimize-sibling-calls") From 50d7579dd1230e4e77c2a6e14ea8c75110cd4bcb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 15 Oct 2024 09:57:34 +0200 Subject: [PATCH 7/7] Temporarily comment out tests that are clogging the CI The pk_write_pubkey_check sometimes take ~3 hours to run with GCC+Asan on the CI in the full config. Comment out the slowest ones while we investigate and release 3.6.2. Signed-off-by: Gilles Peskine --- .../tests/suites/test_suite_pkwrite.data | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tf-psa-crypto/tests/suites/test_suite_pkwrite.data b/tf-psa-crypto/tests/suites/test_suite_pkwrite.data index 67a846b807..d895d39d3a 100644 --- a/tf-psa-crypto/tests/suites/test_suite_pkwrite.data +++ b/tf-psa-crypto/tests/suites/test_suite_pkwrite.data @@ -30,13 +30,16 @@ Public key write check EC 521 bits (DER) depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:PSA_WANT_ECC_SECP_R1_521 pk_write_pubkey_check:"../../framework/data_files/ec_521_pub.der":TEST_DER -Public key write check EC Brainpool 512 bits -depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:MBEDTLS_PEM_PARSE_C:MBEDTLS_PEM_WRITE_C:PSA_WANT_ECC_BRAINPOOL_P_R1_512 -pk_write_pubkey_check:"../../framework/data_files/ec_bp512_pub.pem":TEST_PEM +## The pk_write_pubkey_check sometimes take ~3 hours to run with +## GCC+Asan on the CI in the full config. Comment out the slowest +## ones while we investigate and release 3.6.2. +# Public key write check EC Brainpool 512 bits +# depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:MBEDTLS_PEM_PARSE_C:MBEDTLS_PEM_WRITE_C:PSA_WANT_ECC_BRAINPOOL_P_R1_512 +# pk_write_pubkey_check:"../../framework/data_files/ec_bp512_pub.pem":TEST_PEM -Public key write check EC Brainpool 512 bits (DER) -depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:PSA_WANT_ECC_BRAINPOOL_P_R1_512 -pk_write_pubkey_check:"../../framework/data_files/ec_bp512_pub.der":TEST_DER +# Public key write check EC Brainpool 512 bits (DER) +# depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:PSA_WANT_ECC_BRAINPOOL_P_R1_512 +# pk_write_pubkey_check:"../../framework/data_files/ec_bp512_pub.der":TEST_DER Public key write check EC X25519 depends_on:PSA_WANT_KEY_TYPE_ECC_PUBLIC_KEY:MBEDTLS_PEM_PARSE_C:MBEDTLS_PEM_WRITE_C:PSA_WANT_ECC_MONTGOMERY_255