From a0e810de4b7a457348d5d6a516e8303cd41220e8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 16:04:27 +0200 Subject: [PATCH 1/6] 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 7c2c818d8..ac02e548a 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -705,7 +705,7 @@ static int ssl_save_session_serialize(mbedtls_ssl_context *ssl, } /* get size of the buffer needed */ - mbedtls_ssl_session_save(&exported_session, NULL, 0, session_data_len); + (void) mbedtls_ssl_session_save(&exported_session, 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 21e46b39ccf6ca1e47f13df25aacfd1defc5e889 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 16:35:20 +0200 Subject: [PATCH 2/6] 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 | 4 +--- .../test_suite_psa_crypto_se_driver_hal.function | 2 +- tests/suites/test_suite_ssl.function | 9 ++++----- tests/suites/test_suite_x509parse.function | 12 +++++++----- tests/suites/test_suite_x509write.function | 14 +++++++------- 6 files changed, 21 insertions(+), 24 deletions(-) diff --git a/tests/suites/test_suite_ctr_drbg.function b/tests/suites/test_suite_ctr_drbg.function index 7d816080a..c6896998e 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 730bb881b..733909ebc 100644 --- a/tests/suites/test_suite_pkwrite.function +++ b/tests/suites/test_suite_pkwrite.function @@ -68,6 +68,7 @@ static int pk_write_any_key(mbedtls_pk_context *pk, unsigned char **p, 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; @@ -78,9 +79,6 @@ static void pk_write_check_common(char *key_file, int is_public_key, int is_der) USE_PSA_INIT(); - mbedtls_pk_init(&key); - USE_PSA_INIT(); - /* Note: if mbedtls_pk_load_file() successfully reads the file, then it also allocates check_buf, which should be freed on exit */ TEST_EQUAL(mbedtls_pk_load_file(key_file, &check_buf, &check_buf_len), 0); 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 9c5ef23a6..8e9698443 100644 --- a/tests/suites/test_suite_psa_crypto_se_driver_hal.function +++ b/tests/suites/test_suite_psa_crypto_se_driver_hal.function @@ -1297,7 +1297,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 eb2407d2e..8a837bb9c 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -24,6 +24,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]; @@ -43,8 +44,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)) @@ -1787,7 +1786,9 @@ void ssl_tls13_record_protection(int ciphersuite, { mbedtls_ssl_key_set keys; mbedtls_ssl_transform transform_send; + mbedtls_ssl_transform_init(&transform_send); mbedtls_ssl_transform transform_recv; + mbedtls_ssl_transform_init(&transform_recv); mbedtls_record rec; unsigned char *buf = NULL; size_t buf_len; @@ -1818,8 +1819,6 @@ void ssl_tls13_record_protection(int ciphersuite, keys.key_len = server_write_key->len; keys.iv_len = server_write_iv->len; - mbedtls_ssl_transform_init(&transform_recv); - mbedtls_ssl_transform_init(&transform_send); MD_OR_USE_PSA_INIT(); TEST_ASSERT(mbedtls_ssl_tls13_populate_transform( @@ -3122,6 +3121,7 @@ void raw_key_agreement_fail(int bad_server_ecdhe_key) mbedtls_psa_stats_t stats; size_t free_slots_before = -1; mbedtls_test_handshake_test_options options; + mbedtls_test_init_handshake_options(&options); uint16_t iana_tls_group_list[] = { MBEDTLS_SSL_IANA_TLS_GROUP_SECP256R1, MBEDTLS_SSL_IANA_TLS_GROUP_NONE }; @@ -3129,7 +3129,6 @@ void raw_key_agreement_fail(int bad_server_ecdhe_key) mbedtls_platform_zeroize(&client, sizeof(client)); mbedtls_platform_zeroize(&server, sizeof(server)); - mbedtls_test_init_handshake_options(&options); options.pk_alg = MBEDTLS_PK_ECDSA; options.server_min_version = MBEDTLS_SSL_VERSION_TLS1_2; options.server_max_version = MBEDTLS_SSL_VERSION_TLS1_2; diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 114bd5277..c38a37283 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -928,15 +928,17 @@ void mbedtls_x509_dn_get_next(char *name_str, int ret = 0, i; size_t len = 0, out_size; mbedtls_asn1_named_data *names = NULL; - mbedtls_x509_name parsed, *parsed_cur; + mbedtls_x509_name parsed; + memset(&parsed, 0, sizeof(parsed)); + mbedtls_x509_name *parsed_cur; // Size of buf is maximum required for test cases - unsigned char buf[80], *out = NULL, *c; + unsigned char buf[80] = {0}; + unsigned char *out = NULL; + unsigned char *c = buf + sizeof(buf); const char *short_name; USE_PSA_INIT(); - memset(&parsed, 0, sizeof(parsed)); - memset(buf, 0, sizeof(buf)); - c = buf + sizeof(buf); + // Additional size required for trailing space out_size = strlen(expected_oids) + 2; TEST_CALLOC(out, out_size); diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index a7ed26295..06e08168c 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -699,16 +699,16 @@ void mbedtls_x509_string_to_names(char *name, char *parsed_name, 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_EQUAL(ret, result); From bb7d92c4b259e9b01760fc24624651dbd111172a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 17:26:44 +0200 Subject: [PATCH 3/6] 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 c38a37283..938917ad7 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -729,7 +729,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 d681ffdb54568353206bf1111dfcecbffeeb7c24 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 17:31:50 +0200 Subject: [PATCH 4/6] 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 8a837bb9c..9ebc56c34 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -1199,7 +1199,7 @@ void ssl_crypt_record(int cipher_type, int hash_id, TEST_ASSERT(ret == 0); - TEST_ASSERT((buf = mbedtls_calloc(1, buflen)) != NULL); + TEST_CALLOC(buf, buflen); while (num_records-- > 0) { mbedtls_ssl_transform *t_dec, *t_enc; @@ -1353,7 +1353,7 @@ void ssl_crypt_record_small(int cipher_type, int hash_id, TEST_ASSERT(ret == 0); - TEST_ASSERT((buf = mbedtls_calloc(1, buflen)) != NULL); + TEST_CALLOC(buf, buflen); for (mode = 1; mode <= 3; mode++) { seen_success = 0; @@ -1957,7 +1957,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); @@ -2171,7 +2171,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); @@ -2214,7 +2215,7 @@ void ssl_serialize_session_load_buf_size(int ticket_len, char *crt_file, } 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); @@ -2223,8 +2224,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 bbd92917d837d6c2a94b6b871ed454ca7b1280a1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2023 18:08:24 +0200 Subject: [PATCH 5/6] 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 0e013b740..ed9f3ac3c 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; } From f2574206e5cf70d978a0cf4b880534271fc788cc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 18 Oct 2023 17:39:48 +0200 Subject: [PATCH 6/6] Fix code style Signed-off-by: Gilles Peskine --- tests/suites/test_suite_x509parse.function | 2 +- tests/suites/test_suite_x509write.function | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_x509parse.function b/tests/suites/test_suite_x509parse.function index 938917ad7..894e0bb18 100644 --- a/tests/suites/test_suite_x509parse.function +++ b/tests/suites/test_suite_x509parse.function @@ -932,7 +932,7 @@ void mbedtls_x509_dn_get_next(char *name_str, memset(&parsed, 0, sizeof(parsed)); mbedtls_x509_name *parsed_cur; // Size of buf is maximum required for test cases - unsigned char buf[80] = {0}; + unsigned char buf[80] = { 0 }; unsigned char *out = NULL; unsigned char *c = buf + sizeof(buf); const char *short_name; diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index 06e08168c..4de9addca 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -703,8 +703,8 @@ void mbedtls_x509_string_to_names(char *name, char *parsed_name, 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 buf[1024] = { 0 }; + unsigned char out[1024] = { 0 }; unsigned char *c = buf + sizeof(buf); USE_PSA_INIT();