From 5fdd0bddb4ad19fb7fc1b2fbd423da7dcecfb060 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 16:04:27 +0200 Subject: [PATCH 1/8] Convey that it's ok for mbedtls_ssl_session_save to fail mbedtls_ssl_session_save() always outputs the output length, even on error. Here, we're only calling it to get the needed output length, so it's ok to ignore the return value. Convey this to linters. Signed-off-by: Gilles Peskine --- programs/ssl/ssl_client2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index ca74c002c..c0c546eca 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -2145,8 +2145,8 @@ usage: } /* get size of the buffer needed */ - mbedtls_ssl_session_save(mbedtls_ssl_get_session_pointer(&ssl), - NULL, 0, &session_data_len); + (void) mbedtls_ssl_session_save(mbedtls_ssl_get_session_pointer(&ssl), + NULL, 0, &session_data_len); session_data = mbedtls_calloc(1, session_data_len); if (session_data == NULL) { mbedtls_printf(" failed\n ! alloc %u bytes for session data\n", From 11f41793f8fcf2e2fdb56220f57ab040d57ba6a4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 16:35:20 +0200 Subject: [PATCH 2/8] Fix missing initializations on some error paths Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ctr_drbg.function | 4 +--- tests/suites/test_suite_pkwrite.function | 2 +- .../test_suite_psa_crypto_se_driver_hal.function | 2 +- tests/suites/test_suite_ssl.function | 3 +-- tests/suites/test_suite_x509write.function | 14 +++++++------- 5 files changed, 11 insertions(+), 14 deletions(-) diff --git a/tests/suites/test_suite_ctr_drbg.function b/tests/suites/test_suite_ctr_drbg.function index 4c6ee6f5a..a4627de58 100644 --- a/tests/suites/test_suite_ctr_drbg.function +++ b/tests/suites/test_suite_ctr_drbg.function @@ -31,15 +31,13 @@ static void ctr_drbg_validate_internal(int reseed_mode, data_t *nonce, data_t *result) { mbedtls_ctr_drbg_context ctx; + mbedtls_ctr_drbg_init(&ctx); unsigned char buf[64]; size_t entropy_chunk_len = (size_t) entropy_len_arg; - TEST_ASSERT(entropy_chunk_len <= sizeof(buf)); test_offset_idx = 0; - mbedtls_ctr_drbg_init(&ctx); - test_max_idx = entropy->len; /* CTR_DRBG_Instantiate(entropy[:entropy->len], nonce, perso, ) diff --git a/tests/suites/test_suite_pkwrite.function b/tests/suites/test_suite_pkwrite.function index d1f7813b7..97fd92a5b 100644 --- a/tests/suites/test_suite_pkwrite.function +++ b/tests/suites/test_suite_pkwrite.function @@ -31,13 +31,13 @@ static void fix_new_lines(unsigned char *in_str, size_t *len) static void pk_write_check_common(char *key_file, int is_public_key, int is_der) { mbedtls_pk_context key; + mbedtls_pk_init(&key); unsigned char *buf = NULL; unsigned char *check_buf = NULL; unsigned char *start_buf; size_t buf_len, check_buf_len; int ret; - mbedtls_pk_init(&key); USE_PSA_INIT(); /* Note: if mbedtls_pk_load_file() successfully reads the file, then diff --git a/tests/suites/test_suite_psa_crypto_se_driver_hal.function b/tests/suites/test_suite_psa_crypto_se_driver_hal.function index 15232a44f..ff0ccdd09 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -1290,7 +1290,7 @@ void sign_verify(int flow, mbedtls_svc_key_id_t returned_id; mbedtls_svc_key_id_t sw_key = MBEDTLS_SVC_KEY_ID_INIT; psa_key_attributes_t sw_attributes = PSA_KEY_ATTRIBUTES_INIT; - psa_key_attributes_t drv_attributes; + psa_key_attributes_t drv_attributes = PSA_KEY_ATTRIBUTES_INIT; uint8_t signature[PSA_SIGNATURE_MAX_SIZE]; size_t signature_length; diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 0a35aa973..19a9f3209 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -19,6 +19,7 @@ void test_callback_buffer_sanity() { enum { MSGLEN = 10 }; mbedtls_test_ssl_buffer buf; + mbedtls_test_ssl_buffer_init(&buf); unsigned char input[MSGLEN]; unsigned char output[MSGLEN]; @@ -38,8 +39,6 @@ void test_callback_buffer_sanity() /* Make sure calling put and get on a buffer that hasn't been set up results * in error. */ - mbedtls_test_ssl_buffer_init(&buf); - TEST_ASSERT(mbedtls_test_ssl_buffer_put(&buf, input, sizeof(input)) == -1); TEST_ASSERT(mbedtls_test_ssl_buffer_get(&buf, output, sizeof(output)) diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index b4509e235..ad0f2a6f5 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -457,16 +457,16 @@ void mbedtls_x509_string_to_names(char *name, char *parsed_name, int result int ret; size_t len = 0; mbedtls_asn1_named_data *names = NULL; - mbedtls_x509_name parsed, *parsed_cur, *parsed_prv; - unsigned char buf[1024], out[1024], *c; + mbedtls_x509_name parsed; + memset(&parsed, 0, sizeof(parsed)); + mbedtls_x509_name *parsed_cur = NULL; + mbedtls_x509_name *parsed_prv = NULL; + unsigned char buf[1024] = { 0 }; + unsigned char out[1024] = { 0 }; + unsigned char *c = buf + sizeof(buf); USE_PSA_INIT(); - memset(&parsed, 0, sizeof(parsed)); - memset(out, 0, sizeof(out)); - memset(buf, 0, sizeof(buf)); - c = buf + sizeof(buf); - ret = mbedtls_x509_string_to_names(&names, name); TEST_ASSERT(ret == result); From 5b5da941a4707fea8199aa3bca9168805003e76b Mon Sep 17 00:00:00 2001 From: valerio Date: Thu, 20 Apr 2023 11:59:52 +0200 Subject: [PATCH 3/8] test: proper positioning of USE_PSA_INIT + fixed some exit labels Very partial backport of 32f2ac9a180e08c35f4643e8e969f864a2d79ada Signed-off-by: valerio Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ssl.function | 6 +++++- tests/suites/test_suite_x509parse.function | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 19a9f3209..ca81e2acb 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -1907,6 +1907,8 @@ void ssl_session_serialize_version_check(int corrupt_major, *byte ^= corrupted_bit; } } + +exit: USE_PSA_DONE(); } /* END_CASE */ @@ -2269,8 +2271,10 @@ void cookie_parsing(data_t *cookie, int exp_ret) size_t len; mbedtls_ssl_init(&ssl); - USE_PSA_INIT(); mbedtls_ssl_config_init(&conf); + + USE_PSA_INIT(); + TEST_EQUAL(mbedtls_ssl_config_defaults(&conf, MBEDTLS_SSL_IS_SERVER, MBEDTLS_SSL_TRANSPORT_DATAGRAM, MBEDTLS_SSL_PRESET_DEFAULT), diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 6e327924c..f38dea980 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -431,7 +431,6 @@ void x509_parse_san(char *crt_file, char *result_str) TEST_EQUAL(strcmp(buf, result_str), 0); exit: - mbedtls_x509_crt_free(&crt); USE_PSA_DONE(); } @@ -864,9 +863,8 @@ void mbedtls_x509_get_name(char *rdn_sequence, int exp_ret) TEST_EQUAL(ret, exp_ret); - mbedtls_free(name); - exit: + mbedtls_free(name); USE_PSA_DONE(); } /* END_CASE */ @@ -1252,6 +1250,7 @@ void x509_oid_desc(data_t *buf, char *ref_desc) int ret; USE_PSA_INIT(); + oid.tag = MBEDTLS_ASN1_OID; oid.p = buf->x; oid.len = buf->len; @@ -1279,6 +1278,7 @@ void x509_oid_numstr(data_t *oid_buf, char *numstr, int blen, int ret) char num_buf[100]; USE_PSA_INIT(); + memset(num_buf, 0x2a, sizeof(num_buf)); oid.tag = MBEDTLS_ASN1_OID; From ce9c4f52c4178b97b04111276396209154046959 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 17:26:44 +0200 Subject: [PATCH 4/8] Remove redundant null check crl_file is a test argument and can't be null. Besides the code above already assumes that it's non-null. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_x509parse.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index f38dea980..8c72e5af3 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -674,7 +674,7 @@ void x509_verify(char *crt_file, char *ca_file, char *crl_file, #if defined(MBEDTLS_X509_TRUSTED_CERTIFICATE_CALLBACK) /* CRLs aren't supported with CA callbacks, so skip the CA callback * version of the test if CRLs are in use. */ - if (crl_file == NULL || strcmp(crl_file, "") == 0) { + if (strcmp(crl_file, "") == 0) { flags = 0; res = mbedtls_x509_crt_verify_with_ca_cb(&crt, From 72aa683aae0670bbd5725350d2173b749f04a24f Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 19 Sep 2023 17:34:39 +0100 Subject: [PATCH 5/8] Introduce TEST_CALLOC_NONNULL Signed-off-by: Dave Rodgman --- tests/include/test/macros.h | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/include/test/macros.h b/tests/include/test/macros.h index f94288999..fb4547863 100644 --- a/tests/include/test/macros.h +++ b/tests/include/test/macros.h @@ -153,6 +153,32 @@ } \ } while (0) +/** Allocate memory dynamically and fail the test case if this fails. + * The allocated memory will be filled with zeros. + * + * You must set \p pointer to \c NULL before calling this macro and + * put `mbedtls_free(pointer)` in the test's cleanup code. + * + * If \p item_count is zero, the resulting \p pointer will not be \c NULL. + * + * This macro expands to an instruction, not an expression. + * It may jump to the \c exit label. + * + * \param pointer An lvalue where the address of the allocated buffer + * will be stored. + * This expression may be evaluated multiple times. + * \param item_count Number of elements to allocate. + * This expression may be evaluated multiple times. + * + */ +#define TEST_CALLOC_NONNULL(pointer, item_count) \ + do { \ + TEST_ASSERT((pointer) == NULL); \ + (pointer) = mbedtls_calloc(sizeof(*(pointer)), \ + (item_count)); \ + TEST_ASSERT((pointer) != NULL); \ + } while (0) + /* For backwards compatibility */ #define ASSERT_ALLOC(pointer, item_count) TEST_CALLOC(pointer, item_count) From 3ca2f5cd018d16b72887283137a7ae72a9411771 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 19 Sep 2023 18:30:25 +0100 Subject: [PATCH 6/8] Make TEST_CALLOC_NONNULL more robust Signed-off-by: Dave Rodgman --- tests/include/test/macros.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/include/test/macros.h b/tests/include/test/macros.h index fb4547863..d67e487a3 100644 --- a/tests/include/test/macros.h +++ b/tests/include/test/macros.h @@ -170,12 +170,18 @@ * \param item_count Number of elements to allocate. * This expression may be evaluated multiple times. * + * Note: if passing size 0, mbedtls_calloc may return NULL. In this case, + * we reattempt to allocate with the smallest possible buffer to assure a + * non-NULL pointer. */ #define TEST_CALLOC_NONNULL(pointer, item_count) \ do { \ TEST_ASSERT((pointer) == NULL); \ (pointer) = mbedtls_calloc(sizeof(*(pointer)), \ (item_count)); \ + if (((pointer) == NULL) && ((item_count) == 0)) { \ + (pointer) = mbedtls_calloc(1, 1); \ + } \ TEST_ASSERT((pointer) != NULL); \ } while (0) From bf8520080a91182008c9f168beaf3d2a2454a310 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 17:31:50 +0200 Subject: [PATCH 7/8] Use modern macros for calloc in test code Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ssl.function | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index ca81e2acb..2eb1c4e0e 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -1185,7 +1185,7 @@ void ssl_crypt_record(int cipher_type, int hash_id, (size_t) cid0_len, (size_t) cid1_len) == 0); - TEST_ASSERT((buf = mbedtls_calloc(1, buflen)) != NULL); + TEST_CALLOC(buf, buflen); while (num_records-- > 0) { mbedtls_ssl_transform *t_dec, *t_enc; @@ -1335,7 +1335,7 @@ void ssl_crypt_record_small(int cipher_type, int hash_id, (size_t) cid0_len, (size_t) cid1_len) == 0); - TEST_ASSERT((buf = mbedtls_calloc(1, buflen)) != NULL); + TEST_CALLOC(buf, buflen); for (mode = 1; mode <= 3; mode++) { seen_success = 0; @@ -1636,7 +1636,7 @@ void ssl_serialize_session_save_load(int ticket_len, char *crt_file) /* Serialize it */ TEST_ASSERT(mbedtls_ssl_session_save(&original, NULL, 0, &len) == MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL); - TEST_ASSERT((buf = mbedtls_calloc(1, len)) != NULL); + TEST_CALLOC(buf, len); TEST_ASSERT(mbedtls_ssl_session_save(&original, buf, len, &len) == 0); @@ -1791,7 +1791,8 @@ void ssl_serialize_session_save_buf_size(int ticket_len, char *crt_file) for (bad_len = 1; bad_len < good_len; bad_len++) { /* Allocate exact size so that asan/valgrind can detect any overwrite */ mbedtls_free(buf); - TEST_ASSERT((buf = mbedtls_calloc(1, bad_len)) != NULL); + buf = NULL; + TEST_CALLOC(buf, bad_len); TEST_ASSERT(mbedtls_ssl_session_save(&session, buf, bad_len, &test_len) == MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL); @@ -1823,7 +1824,7 @@ void ssl_serialize_session_load_buf_size(int ticket_len, char *crt_file) &session, ticket_len, crt_file) == 0); TEST_ASSERT(mbedtls_ssl_session_save(&session, NULL, 0, &good_len) == MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL); - TEST_ASSERT((good_buf = mbedtls_calloc(1, good_len)) != NULL); + TEST_CALLOC(good_buf, good_len); TEST_ASSERT(mbedtls_ssl_session_save(&session, good_buf, good_len, &good_len) == 0); mbedtls_ssl_session_free(&session); @@ -1832,8 +1833,8 @@ void ssl_serialize_session_load_buf_size(int ticket_len, char *crt_file) for (bad_len = 0; bad_len < good_len; bad_len++) { /* Allocate exact size so that asan/valgrind can detect any overread */ mbedtls_free(bad_buf); - bad_buf = mbedtls_calloc(1, bad_len ? bad_len : 1); - TEST_ASSERT(bad_buf != NULL); + bad_buf = NULL; + TEST_CALLOC_NONNULL(bad_buf, bad_len); memcpy(bad_buf, good_buf, bad_len); TEST_ASSERT(mbedtls_ssl_session_load(&session, bad_buf, bad_len) From fa276363968abe8952c599e8bab9819d54bbe30f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 18:08:24 +0200 Subject: [PATCH 8/8] Close file on error path Signed-off-by: Gilles Peskine --- tests/suites/test_suite_entropy.function | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/suites/test_suite_entropy.function b/tests/suites/test_suite_entropy.function index 5d8487c59..9b1df8fb1 100644 --- a/tests/suites/test_suite_entropy.function +++ b/tests/suites/test_suite_entropy.function @@ -102,6 +102,7 @@ static int write_nv_seed(unsigned char *buf, size_t buf_len) if (fwrite(buf, 1, MBEDTLS_ENTROPY_BLOCK_SIZE, f) != MBEDTLS_ENTROPY_BLOCK_SIZE) { + fclose(f); return -1; } @@ -124,6 +125,7 @@ int read_nv_seed(unsigned char *buf, size_t buf_len) if (fread(buf, 1, MBEDTLS_ENTROPY_BLOCK_SIZE, f) != MBEDTLS_ENTROPY_BLOCK_SIZE) { + fclose(f); return -1; }