From cbecd0a40b4d530d846c2a2edff2ca9a3a8f0761 Mon Sep 17 00:00:00 2001 From: Valerio Setti Date: Fri, 11 Oct 2024 14:55:24 +0200 Subject: [PATCH] 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 --- library/pkwrite.c | 14 +++++++++++--- tests/suites/test_suite_pkwrite.function | 19 ++++++++++++++++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/library/pkwrite.c b/library/pkwrite.c index 5e009c565e..2a698448be 100644 --- a/library/pkwrite.c +++ b/library/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/tests/suites/test_suite_pkwrite.function b/tests/suites/test_suite_pkwrite.function index 735c12547c..3392528115 100644 --- a/tests/suites/test_suite_pkwrite.function +++ b/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);