ARMmbed/mbedtls
1
0
Fork 0
mirror of https://github.com/ARMmbed/mbedtls.git synced 2025-05-10 00:49:04 +08:00
Code Issues Releases Wiki Activity

Merge pull request #9690 from valeriosetti/fix-pk-write-buffer-overrun

Browse Source 
pkwrite: fix buffer overrun
This commit is contained in:
Gilles Peskine 2024-10-16 12:00:52 +00:00 committed by GitHub
commit 4e4647a4e7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 58 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
CMakeLists.txt
View File

@ -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")

8
ChangeLog.d/9690.txt Normal file
View File

@ -0,0 +1,8 @@
Security
* 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_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

4
docs/architecture/psa-keystore-design.md
View File

@ -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

14
tf-psa-crypto/drivers/builtin/src/pkwrite.c
View File

@ -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;

15
tf-psa-crypto/tests/suites/test_suite_pkwrite.data
View File

@ -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

19
tf-psa-crypto/tests/suites/test_suite_pkwrite.function
View File

@ -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 {
@ -73,6 +74,7 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der)
unsigned char *check_buf = NULL;
unsigned char *start_buf;
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;
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. */
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);
@ -127,6 +140,12 @@ 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. */
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);