From 4eafc9c47e48a8592dfddaea8186121c9761a056 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2024 18:07:55 +0200 Subject: [PATCH 1/8] Modernize mpi_write_binary and mpi_write_binary_le Use TEST_CALLOC instead of a fixed-size buffer, so that Asan/Valgrind builds will detect a buffer overflow. Honor output_size regardless of the value of the number. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_bignum.function | 44 +++++++++--------------- tests/suites/test_suite_bignum.misc.data | 4 +-- 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/tests/suites/test_suite_bignum.function b/tests/suites/test_suite_bignum.function index 3d2b8a1240..36f1476d76 100644 --- a/tests/suites/test_suite_bignum.function +++ b/tests/suites/test_suite_bignum.function @@ -212,28 +212,22 @@ void mpi_write_binary(char *input_X, data_t *input_A, int output_size, int result) { mbedtls_mpi X; - unsigned char buf[1000]; - size_t buflen; - - memset(buf, 0x00, 1000); - mbedtls_mpi_init(&X); + unsigned char *buf = NULL; - TEST_ASSERT(mbedtls_test_read_mpi(&X, input_X) == 0); + TEST_EQUAL(mbedtls_test_read_mpi(&X, input_X), 0); - buflen = mbedtls_mpi_size(&X); - if (buflen > (size_t) output_size) { - buflen = (size_t) output_size; - } + TEST_CALLOC(buf, output_size); + + TEST_EQUAL(mbedtls_mpi_write_binary(&X, buf, output_size), result); - TEST_ASSERT(mbedtls_mpi_write_binary(&X, buf, buflen) == result); if (result == 0) { - - TEST_ASSERT(mbedtls_test_hexcmp(buf, input_A->x, - buflen, input_A->len) == 0); + TEST_EQUAL(mbedtls_test_hexcmp(buf, input_A->x, + output_size, input_A->len), 0); } exit: + mbedtls_free(buf); mbedtls_mpi_free(&X); } /* END_CASE */ @@ -243,28 +237,22 @@ void mpi_write_binary_le(char *input_X, data_t *input_A, int output_size, int result) { mbedtls_mpi X; - unsigned char buf[1000]; - size_t buflen; - - memset(buf, 0x00, 1000); - mbedtls_mpi_init(&X); + unsigned char *buf = NULL; - TEST_ASSERT(mbedtls_test_read_mpi(&X, input_X) == 0); + TEST_EQUAL(mbedtls_test_read_mpi(&X, input_X), 0); - buflen = mbedtls_mpi_size(&X); - if (buflen > (size_t) output_size) { - buflen = (size_t) output_size; - } + TEST_CALLOC(buf, output_size); + + TEST_EQUAL(mbedtls_mpi_write_binary_le(&X, buf, output_size), result); - TEST_ASSERT(mbedtls_mpi_write_binary_le(&X, buf, buflen) == result); if (result == 0) { - - TEST_ASSERT(mbedtls_test_hexcmp(buf, input_A->x, - buflen, input_A->len) == 0); + TEST_EQUAL(mbedtls_test_hexcmp(buf, input_A->x, + output_size, input_A->len), 0); } exit: + mbedtls_free(buf); mbedtls_mpi_free(&X); } /* END_CASE */ diff --git a/tests/suites/test_suite_bignum.misc.data b/tests/suites/test_suite_bignum.misc.data index c16c6890aa..e60331c69f 100644 --- a/tests/suites/test_suite_bignum.misc.data +++ b/tests/suites/test_suite_bignum.misc.data @@ -92,7 +92,7 @@ Base test mbedtls_mpi_read_binary_le #1 mpi_read_binary_le:"0941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":"24448B952FBBEF93F89286BA330E62528B151EAC265CC8CE3038519D09E148AF89288E91F48B41ACAD55D9DC5E2B18097C106BE4CE132721BF6359EAF403E7FF90623E8866EE5C192320418DAA682F144ADEDF84F25DE11F49D1FE009D374109" Base test mbedtls_mpi_write_binary #1 -mpi_write_binary:"941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":"0941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":200:0 +mpi_write_binary:"941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":"000000000941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":100:0 Test mbedtls_mpi_write_binary #1 (Buffer just fits) mpi_write_binary:"123123123123123123123123123":"0123123123123123123123123123":14:0 @@ -101,7 +101,7 @@ Test mbedtls_mpi_write_binary #2 (Buffer too small) mpi_write_binary:"123123123123123123123123123":"23123123123123123123123123":13:MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL Base test mbedtls_mpi_write_binary_le #1 -mpi_write_binary_le:"941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":"24448b952fbbef93f89286ba330e62528b151eac265cc8ce3038519d09e148af89288e91f48b41acad55d9dc5e2b18097c106be4ce132721bf6359eaf403e7ff90623e8866ee5c192320418daa682f144adedf84f25de11f49d1fe009d374109":200:0 +mpi_write_binary_le:"941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":"24448b952fbbef93f89286ba330e62528b151eac265cc8ce3038519d09e148af89288e91f48b41acad55d9dc5e2b18097c106be4ce132721bf6359eaf403e7ff90623e8866ee5c192320418daa682f144adedf84f25de11f49d1fe009d37410900000000":100:0 Test mbedtls_mpi_write_binary_le #1 (Buffer just fits) mpi_write_binary_le:"123123123123123123123123123":"2331122331122331122331122301":14:0 From ad70136703045540d2410dd9f3306598191ddbe1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2024 18:09:00 +0200 Subject: [PATCH 2/8] mbedtls_mpi_write_binary{,_le}: test 0-size output Signed-off-by: Gilles Peskine --- tests/suites/test_suite_bignum.misc.data | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/suites/test_suite_bignum.misc.data b/tests/suites/test_suite_bignum.misc.data index e60331c69f..2e3ff1ecc0 100644 --- a/tests/suites/test_suite_bignum.misc.data +++ b/tests/suites/test_suite_bignum.misc.data @@ -94,21 +94,39 @@ mpi_read_binary_le:"0941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e Base test mbedtls_mpi_write_binary #1 mpi_write_binary:"941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":"000000000941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":100:0 +Test mbedtls_mpi_write_binary #1 (Buffer is larger) +mpi_write_binary:"123123123123123123123123123":"000123123123123123123123123123":15:0 + Test mbedtls_mpi_write_binary #1 (Buffer just fits) mpi_write_binary:"123123123123123123123123123":"0123123123123123123123123123":14:0 Test mbedtls_mpi_write_binary #2 (Buffer too small) mpi_write_binary:"123123123123123123123123123":"23123123123123123123123123":13:MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL +Test mbedtls_mpi_write_binary: nonzero to NULL +mpi_write_binary:"01":"":0:MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL + +Test mbedtls_mpi_write_binary: 0 to NULL +mpi_write_binary:"00":"":0:0 + Base test mbedtls_mpi_write_binary_le #1 mpi_write_binary_le:"941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":"24448b952fbbef93f89286ba330e62528b151eac265cc8ce3038519d09e148af89288e91f48b41acad55d9dc5e2b18097c106be4ce132721bf6359eaf403e7ff90623e8866ee5c192320418daa682f144adedf84f25de11f49d1fe009d37410900000000":100:0 +Test mbedtls_mpi_write_binary_le #1 (Buffer is larger) +mpi_write_binary_le:"123123123123123123123123123":"233112233112233112233112230100":15:0 + Test mbedtls_mpi_write_binary_le #1 (Buffer just fits) mpi_write_binary_le:"123123123123123123123123123":"2331122331122331122331122301":14:0 Test mbedtls_mpi_write_binary_le #2 (Buffer too small) mpi_write_binary_le:"123123123123123123123123123":"23311223311223311223311223":13:MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL +Test mbedtls_mpi_write_binary_le: nonzero to NULL +mpi_write_binary_le:"01":"":0:MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL + +Test mbedtls_mpi_write_binary_le: 0 to NULL +mpi_write_binary_le:"00":"":0:0 + Base test mbedtls_mpi_read_file #1 mpi_read_file:"../framework/data_files/mpi_16":"01f55332c3a48b910f9942f6c914e58bef37a47ee45cb164a5b6b8d1006bf59a059c21449939ebebfdf517d2e1dbac88010d7b1f141e997bd6801ddaec9d05910f4f2de2b2c4d714e2c14a72fc7f17aa428d59c531627f09":0 From b482e44e4961c81164a286cf8ef46ebc0f8790be Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2024 18:35:01 +0200 Subject: [PATCH 3/8] Document errors for mbedtls_ecdsa_raw_to_der and mbedtls_ecdsa_der_to_raw Document the return value of mbedtls_ecdsa_raw_to_der() and mbedtls_ecdsa_der_to_raw(). Document that mbedtls_ecdsa_raw_to_der() has undefined behavior when the output buffer parameter is null, even with a size of 0. Signed-off-by: Gilles Peskine --- include/mbedtls/psa_util.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/include/mbedtls/psa_util.h b/include/mbedtls/psa_util.h index c78cc23333..b898f1f8d3 100644 --- a/include/mbedtls/psa_util.h +++ b/include/mbedtls/psa_util.h @@ -161,6 +161,16 @@ static inline mbedtls_md_type_t mbedtls_md_type_from_psa_alg(psa_algorithm_t psa * \param[out] der_len On success it contains the amount of valid data * (in bytes) written to \p der. It's undefined * in case of failure. + * + * \note The behavior is undefined if \p der is null, + * even if \p der_size is 0. + * + * \return 0 if successful. + * \return #MBEDTLS_ERR_ASN1_BUF_TOO_SMALL if \p der_size + * is too small or if \p bits is larger than the + * largest supported curve. + * \return #MBEDTLS_ERR_ASN1_INVALID_DATA if one of the + * numbers in the signature is 0. */ int mbedtls_ecdsa_raw_to_der(size_t bits, const unsigned char *raw, size_t raw_len, unsigned char *der, size_t der_size, size_t *der_len); @@ -177,6 +187,15 @@ int mbedtls_ecdsa_raw_to_der(size_t bits, const unsigned char *raw, size_t raw_l * \param[out] raw_len On success it is updated with the amount of valid * data (in bytes) written to \p raw. It's undefined * in case of failure. + * + * \return 0 if successful. + * \return #MBEDTLS_ERR_ASN1_BUF_TOO_SMALL if \p raw_size + * is too small or if \p bits is larger than the + * largest supported curve. + * \return #MBEDTLS_ERR_ASN1_INVALID_DATA if the data in + * \p der is inconsistent with \p bits. + * \return An \c MBEDTLS_ERR_ASN1_xxx error code if + * \p der is malformed. */ int mbedtls_ecdsa_der_to_raw(size_t bits, const unsigned char *der, size_t der_len, unsigned char *raw, size_t raw_size, size_t *raw_len); From 5dea5f355a4707f03dafa558c161ae7a4fa6eb4e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2024 18:56:17 +0200 Subject: [PATCH 4/8] mbedtls_ecdsa_raw_to_der and mbedtls_ecdsa_der_to_raw: reject bits==0 Cleanly reject bits == 0 when calling mbedtls_ecdsa_raw_to_der() and mbedtls_ecdsa_der_to_raw(). This can plausibly happen when bits is user-provided data that the calling application doesn't check. Before this patch, there was typically-benign undefined behavior, such as adding 0 to a null pointer or calling memcpy on a null pointer with a size of 0. Signed-off-by: Gilles Peskine --- library/psa_util.c | 6 ++++++ tests/suites/test_suite_psa_crypto_util.data | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/library/psa_util.c b/library/psa_util.c index 679d00ea9b..014e648ad1 100644 --- a/library/psa_util.c +++ b/library/psa_util.c @@ -440,6 +440,9 @@ int mbedtls_ecdsa_raw_to_der(size_t bits, const unsigned char *raw, size_t raw_l unsigned char *p = der + der_size; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + if (bits == 0) { + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } if (raw_len != (2 * coordinate_len)) { return MBEDTLS_ERR_ASN1_INVALID_DATA; } @@ -559,6 +562,9 @@ int mbedtls_ecdsa_der_to_raw(size_t bits, const unsigned char *der, size_t der_l size_t coordinate_size = PSA_BITS_TO_BYTES(bits); int ret; + if (bits == 0) { + return MBEDTLS_ERR_ASN1_INVALID_DATA; + } /* The output raw buffer should be at least twice the size of a raw * coordinate in order to store r and s. */ if (raw_size < coordinate_size * 2) { diff --git a/tests/suites/test_suite_psa_crypto_util.data b/tests/suites/test_suite_psa_crypto_util.data index c84a8368cd..a0ec9fd554 100644 --- a/tests/suites/test_suite_psa_crypto_util.data +++ b/tests/suites/test_suite_psa_crypto_util.data @@ -1,3 +1,12 @@ +# mbedtls_ecdsa_der_to_raw() doesn't accept a null output buffer, +# even with otherwise invalid paramters, +# so we pass it a (non-null) buffer of length 1. +ECDSA Raw -> DER, 0bit +ecdsa_raw_to_der:0:"":"00":MBEDTLS_ERR_ASN1_INVALID_DATA + +ECDSA DER -> Raw, 0bit +ecdsa_der_to_raw:0:"":"":MBEDTLS_ERR_ASN1_INVALID_DATA + ECDSA Raw -> DER, 256bit, Success depends_on:PSA_VENDOR_ECC_MAX_CURVE_BITS >= 256 ecdsa_raw_to_der:256:"11111111111111111111111111111111111111111111111111111111111111112222222222222222222222222222222222222222222222222222222222222222":"30440220111111111111111111111111111111111111111111111111111111111111111102202222222222222222222222222222222222222222222222222222222222222222":0 From efe30760e5dad5c73f976cfefda5a11c5ab86a9a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 7 Oct 2024 11:11:32 +0200 Subject: [PATCH 5/8] Initialize CCM context before doing anything fallible Otherwise mbedtls_ccm_free() in cleanup could corrupt memory if a failure happens. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ccm.function | 42 ++++++++++++++-------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/tests/suites/test_suite_ccm.function b/tests/suites/test_suite_ccm.function index dbb313b939..8c39d27a11 100644 --- a/tests/suites/test_suite_ccm.function +++ b/tests/suites/test_suite_ccm.function @@ -79,11 +79,11 @@ void mbedtls_ccm_self_test() void mbedtls_ccm_setkey(int cipher_id, int key_size, int result) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); unsigned char key[32]; int ret; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); memset(key, 0x2A, sizeof(key)); TEST_ASSERT((unsigned) key_size <= 8 * sizeof(key)); @@ -101,6 +101,7 @@ exit: void ccm_lengths(int msg_len, int iv_len, int add_len, int tag_len, int res) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); unsigned char key[16]; unsigned char msg[10]; unsigned char iv[14]; @@ -110,7 +111,6 @@ void ccm_lengths(int msg_len, int iv_len, int add_len, int tag_len, int res) int decrypt_ret; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_CALLOC_OR_SKIP(add, add_len); memset(key, 0, sizeof(key)); @@ -146,6 +146,7 @@ void ccm_star_lengths(int msg_len, int iv_len, int add_len, int tag_len, int res) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); unsigned char key[16]; unsigned char msg[10]; unsigned char iv[14]; @@ -155,7 +156,6 @@ void ccm_star_lengths(int msg_len, int iv_len, int add_len, int tag_len, int decrypt_ret; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); memset(key, 0, sizeof(key)); memset(msg, 0, sizeof(msg)); @@ -191,6 +191,7 @@ void mbedtls_ccm_encrypt_and_tag(int cipher_id, data_t *key, data_t *add, data_t *result) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); size_t n1, n1_add; uint8_t *io_msg_buf = NULL; uint8_t *tag_buf = NULL; @@ -207,7 +208,6 @@ void mbedtls_ccm_encrypt_and_tag(int cipher_id, data_t *key, TEST_CALLOC(tag_buf, expected_tag_len); BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); /* Test with input == output */ TEST_EQUAL(mbedtls_ccm_encrypt_and_tag(&ctx, msg->len, iv->x, iv->len, add->x, add->len, @@ -248,11 +248,11 @@ void mbedtls_ccm_star_no_tag(int cipher_id, int mode, data_t *key, data_t *msg, data_t *iv, data_t *result) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; size_t olen; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); TEST_EQUAL(0, mbedtls_ccm_set_lengths(&ctx, 0, msg->len, 0)); @@ -277,6 +277,7 @@ void mbedtls_ccm_auth_decrypt(int cipher_id, data_t *key, data_t *expected_msg) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); size_t n1, n1_add; const size_t expected_msg_len = msg->len - expected_tag_len; @@ -290,7 +291,6 @@ void mbedtls_ccm_auth_decrypt(int cipher_id, data_t *key, } BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); /* Test with input == output */ TEST_EQUAL(mbedtls_ccm_auth_decrypt(&ctx, expected_msg_len, iv->x, iv->len, add->x, add->len, @@ -343,6 +343,7 @@ void mbedtls_ccm_star_encrypt_and_tag(int cipher_id, { unsigned char iv[13]; mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); size_t iv_len, expected_tag_len; size_t n1, n1_add; uint8_t *io_msg_buf = NULL; @@ -379,7 +380,6 @@ void mbedtls_ccm_star_encrypt_and_tag(int cipher_id, iv_len = sizeof(iv); BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); /* Test with input == output */ @@ -430,6 +430,7 @@ void mbedtls_ccm_star_auth_decrypt(int cipher_id, { unsigned char iv[13]; mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); size_t iv_len, expected_tag_len; size_t n1, n1_add; @@ -460,7 +461,6 @@ void mbedtls_ccm_star_auth_decrypt(int cipher_id, iv_len = sizeof(iv); BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_ASSERT(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8) == 0); /* Test with input == output */ TEST_EQUAL(mbedtls_ccm_star_auth_decrypt(&ctx, expected_msg_len, iv, iv_len, @@ -507,6 +507,7 @@ void mbedtls_ccm_skip_ad(int cipher_id, int mode, data_t *result, data_t *tag) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; size_t olen; @@ -514,7 +515,6 @@ void mbedtls_ccm_skip_ad(int cipher_id, int mode, TEST_EQUAL(msg->len, result->len); BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); TEST_EQUAL(0, mbedtls_ccm_set_lengths(&ctx, 0, msg->len, tag->len)); @@ -547,10 +547,10 @@ void mbedtls_ccm_skip_update(int cipher_id, int mode, data_t *tag) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); TEST_EQUAL(0, mbedtls_ccm_set_lengths(&ctx, add->len, 0, tag->len)); @@ -577,9 +577,9 @@ void mbedtls_ccm_overflow_ad(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded values for msg length and tag length. They are not a part of this test @@ -600,9 +600,9 @@ void mbedtls_ccm_unexpected_ad(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded values for msg length and tag length. They are not a part of this test @@ -622,11 +622,11 @@ void mbedtls_ccm_unexpected_text(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; size_t olen; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded value for tag length. It is not a part of this test @@ -651,10 +651,10 @@ void mbedtls_ccm_incomplete_ad(int cipher_id, int mode, data_t *key, data_t *iv, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded values for msg length and tag length. They are not a part of this test @@ -680,9 +680,9 @@ void mbedtls_ccm_full_ad_and_overflow(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded values for msg length and tag length. They are not a part of this test @@ -706,13 +706,13 @@ void mbedtls_ccm_incomplete_ad_and_overflow(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t add_second_buffer[2]; add_second_buffer[0] = add->x[add->len - 1]; add_second_buffer[1] = 0xAB; // some magic value BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded values for msg length and tag length. They are not a part of this test @@ -735,11 +735,11 @@ void mbedtls_ccm_overflow_update(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; size_t olen; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded value for tag length. It is a not a part of this test @@ -765,11 +765,11 @@ void mbedtls_ccm_incomplete_update(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; size_t olen; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded value for tag length. It is not a part of this test @@ -801,11 +801,11 @@ void mbedtls_ccm_full_update_and_overflow(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; size_t olen; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded value for tag length. It is a not a part of this test @@ -834,6 +834,7 @@ void mbedtls_ccm_incomplete_update_overflow(int cipher_id, int mode, data_t *add) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; size_t olen; uint8_t msg_second_buffer[2]; @@ -842,7 +843,6 @@ void mbedtls_ccm_incomplete_update_overflow(int cipher_id, int mode, msg_second_buffer[1] = 0xAB; // some magic value BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded value for tag length. It is a not a part of this test @@ -869,10 +869,10 @@ void mbedtls_ccm_instant_finish(int cipher_id, int mode, data_t *key, data_t *iv) { mbedtls_ccm_context ctx; + mbedtls_ccm_init(&ctx); uint8_t *output = NULL; BLOCK_CIPHER_PSA_INIT(); - mbedtls_ccm_init(&ctx); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); // use hardcoded values for add length, msg length and tag length. From 42919e082138e1fa09066787ae3712a3c82d6809 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 7 Oct 2024 11:12:17 +0200 Subject: [PATCH 6/8] Assert non-empty data when needed Pacify Coverity about subtracting from the length, and give a signal to human readers. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ccm.function | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/suites/test_suite_ccm.function b/tests/suites/test_suite_ccm.function index 8c39d27a11..798be77013 100644 --- a/tests/suites/test_suite_ccm.function +++ b/tests/suites/test_suite_ccm.function @@ -579,6 +579,9 @@ void mbedtls_ccm_overflow_ad(int cipher_id, int mode, mbedtls_ccm_context ctx; mbedtls_ccm_init(&ctx); + /* This test can't be run with empty additional data */ + TEST_LE_U(1, add->len); + BLOCK_CIPHER_PSA_INIT(); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); @@ -654,6 +657,9 @@ void mbedtls_ccm_incomplete_ad(int cipher_id, int mode, mbedtls_ccm_init(&ctx); uint8_t *output = NULL; + /* This test can't be run with empty additional data */ + TEST_LE_U(1, add->len); + BLOCK_CIPHER_PSA_INIT(); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); @@ -709,6 +715,9 @@ void mbedtls_ccm_incomplete_ad_and_overflow(int cipher_id, int mode, mbedtls_ccm_init(&ctx); uint8_t add_second_buffer[2]; + /* This test can't be run with empty additional data */ + TEST_LE_U(1, add->len); + add_second_buffer[0] = add->x[add->len - 1]; add_second_buffer[1] = 0xAB; // some magic value @@ -739,6 +748,9 @@ void mbedtls_ccm_overflow_update(int cipher_id, int mode, uint8_t *output = NULL; size_t olen; + /* This test can't be run with an empty message */ + TEST_LE_U(1, msg->len); + BLOCK_CIPHER_PSA_INIT(); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); @@ -769,6 +781,9 @@ void mbedtls_ccm_incomplete_update(int cipher_id, int mode, uint8_t *output = NULL; size_t olen; + /* This test can't be run with an empty message */ + TEST_LE_U(1, msg->len); + BLOCK_CIPHER_PSA_INIT(); TEST_EQUAL(mbedtls_ccm_setkey(&ctx, cipher_id, key->x, key->len * 8), 0); TEST_EQUAL(0, mbedtls_ccm_starts(&ctx, mode, iv->x, iv->len)); @@ -839,6 +854,9 @@ void mbedtls_ccm_incomplete_update_overflow(int cipher_id, int mode, size_t olen; uint8_t msg_second_buffer[2]; + /* This test can't be run with an empty message */ + TEST_LE_U(1, msg->len); + msg_second_buffer[0] = msg->x[msg->len - 1]; msg_second_buffer[1] = 0xAB; // some magic value From f7b62e063d2a818fa7d5bb50447ea2eb7a179a0e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 7 Oct 2024 11:23:39 +0200 Subject: [PATCH 7/8] Remove unreachable assignments This is harmless, but we might as well remove the unreachable line. If we ever add a break to the loop and we don't think of changing the surrounding code, it would make more sense not to set exit_code to SUCCESS. Signed-off-by: Gilles Peskine --- programs/ssl/ssl_fork_server.c | 2 -- programs/test/udp_proxy.c | 2 -- 2 files changed, 4 deletions(-) diff --git a/programs/ssl/ssl_fork_server.c b/programs/ssl/ssl_fork_server.c index 0edadd4b74..9cee43f081 100644 --- a/programs/ssl/ssl_fork_server.c +++ b/programs/ssl/ssl_fork_server.c @@ -359,8 +359,6 @@ int main(void) goto exit; } - exit_code = MBEDTLS_EXIT_SUCCESS; - exit: mbedtls_net_free(&client_fd); mbedtls_net_free(&listen_fd); diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index 7213f8aea0..43d2e8cf73 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -938,8 +938,6 @@ accept: } - exit_code = MBEDTLS_EXIT_SUCCESS; - exit: #ifdef MBEDTLS_ERROR_C From 138312315e2605e3eb987c3d40b10ea9b519a525 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 7 Oct 2024 11:27:37 +0200 Subject: [PATCH 8/8] Changelog entry for ECDSA conversion functions called with bits=0 Signed-off-by: Gilles Peskine --- ChangeLog.d/psa_util-bits-0.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 ChangeLog.d/psa_util-bits-0.txt diff --git a/ChangeLog.d/psa_util-bits-0.txt b/ChangeLog.d/psa_util-bits-0.txt new file mode 100644 index 0000000000..9aa70ad978 --- /dev/null +++ b/ChangeLog.d/psa_util-bits-0.txt @@ -0,0 +1,3 @@ +Bugfix + * Fix undefined behavior in some cases when mbedtls_psa_raw_to_der() or + mbedtls_psa_der_to_raw() is called with bits=0.