From 9602ce7d8b88af731070d874feffd8294011e877 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 4 Nov 2024 18:21:57 +0100 Subject: [PATCH] Fix MD_PSA_INIT called before initializing some data structures This fixes accesses to uninitialized memory in test code if `psa_crypto_init()` fails. A lot of those were pointed out by Coverity. I quickly reviewed all calls to `MD_PSA_INIT()` manually, rather than follow any particular list. Signed-off-by: Gilles Peskine --- .../tests/suites/test_suite_ecdsa.function | 17 +++++----- .../tests/suites/test_suite_ecjpake.function | 14 +++----- .../suites/test_suite_hmac_drbg.function | 23 +++++++------ .../tests/suites/test_suite_md.function | 17 ++++------ .../tests/suites/test_suite_pem.function | 3 +- .../suites/test_suite_pkcs1_v21.function | 33 +++++++++---------- .../tests/suites/test_suite_pkparse.function | 4 +-- .../tests/suites/test_suite_random.function | 8 ++--- 8 files changed, 54 insertions(+), 65 deletions(-) diff --git a/tf-psa-crypto/tests/suites/test_suite_ecdsa.function b/tf-psa-crypto/tests/suites/test_suite_ecdsa.function index 78c9875b5a..23f83b02d9 100644 --- a/tf-psa-crypto/tests/suites/test_suite_ecdsa.function +++ b/tf-psa-crypto/tests/suites/test_suite_ecdsa.function @@ -196,13 +196,12 @@ void ecdsa_det_test_vectors(int id, char *d_str, int md_alg, data_t *hash, { mbedtls_ecp_group grp; mbedtls_mpi d, r, s, r_check, s_check; - - MD_PSA_INIT(); - mbedtls_ecp_group_init(&grp); mbedtls_mpi_init(&d); mbedtls_mpi_init(&r); mbedtls_mpi_init(&s); mbedtls_mpi_init(&r_check); mbedtls_mpi_init(&s_check); + MD_PSA_INIT(); + TEST_ASSERT(mbedtls_ecp_group_load(&grp, id) == 0); TEST_ASSERT(mbedtls_test_read_mpi(&d, d_str) == 0); TEST_ASSERT(mbedtls_test_read_mpi(&r_check, r_str) == 0); @@ -235,13 +234,13 @@ void ecdsa_write_read_zero(int id) unsigned char sig[200]; size_t sig_len, i; - MD_PSA_INIT(); - mbedtls_ecdsa_init(&ctx); memset(&rnd_info, 0x00, sizeof(mbedtls_test_rnd_pseudo_info)); memset(hash, 0, sizeof(hash)); memset(sig, 0x2a, sizeof(sig)); + MD_PSA_INIT(); + /* generate signing key */ TEST_ASSERT(mbedtls_ecdsa_genkey(&ctx, id, &mbedtls_test_rnd_pseudo_rand, @@ -300,13 +299,13 @@ void ecdsa_write_read_random(int id) unsigned char sig[200]; size_t sig_len, i; - MD_PSA_INIT(); - mbedtls_ecdsa_init(&ctx); memset(&rnd_info, 0x00, sizeof(mbedtls_test_rnd_pseudo_info)); memset(hash, 0, sizeof(hash)); memset(sig, 0x2a, sizeof(sig)); + MD_PSA_INIT(); + /* prepare material for signature */ TEST_ASSERT(mbedtls_test_rnd_pseudo_rand(&rnd_info, hash, sizeof(hash)) == 0); @@ -436,12 +435,12 @@ void ecdsa_write_restart(int id, char *d_str, int md_alg, unsigned char sig[MBEDTLS_ECDSA_MAX_LEN]; size_t slen; - MD_PSA_INIT(); - mbedtls_ecdsa_restart_init(&rs_ctx); mbedtls_ecdsa_init(&ctx); memset(sig, 0, sizeof(sig)); + MD_PSA_INIT(); + TEST_ASSERT(mbedtls_ecp_group_load(&ctx.grp, id) == 0); TEST_ASSERT(mbedtls_test_read_mpi(&ctx.d, d_str) == 0); diff --git a/tf-psa-crypto/tests/suites/test_suite_ecjpake.function b/tf-psa-crypto/tests/suites/test_suite_ecjpake.function index 534c7bafbe..1723010e4f 100644 --- a/tf-psa-crypto/tests/suites/test_suite_ecjpake.function +++ b/tf-psa-crypto/tests/suites/test_suite_ecjpake.function @@ -102,6 +102,7 @@ cleanup: void ecjpake_invalid_param() { mbedtls_ecjpake_context ctx; + mbedtls_ecjpake_init(&ctx); unsigned char buf[42] = { 0 }; size_t const len = sizeof(buf); mbedtls_ecjpake_role invalid_role = (mbedtls_ecjpake_role) 42; @@ -110,8 +111,6 @@ void ecjpake_invalid_param() MD_PSA_INIT(); - mbedtls_ecjpake_init(&ctx); - TEST_EQUAL(MBEDTLS_ERR_ECP_BAD_INPUT_DATA, mbedtls_ecjpake_setup(&ctx, invalid_role, @@ -139,13 +138,13 @@ exit: void read_bad_md(data_t *msg) { mbedtls_ecjpake_context corrupt_ctx; + mbedtls_ecjpake_init(&corrupt_ctx); const unsigned char *pw = NULL; const size_t pw_len = 0; int any_role = MBEDTLS_ECJPAKE_CLIENT; MD_PSA_INIT(); - mbedtls_ecjpake_init(&corrupt_ctx); TEST_ASSERT(mbedtls_ecjpake_setup(&corrupt_ctx, any_role, MBEDTLS_MD_SHA256, MBEDTLS_ECP_DP_SECP256R1, pw, pw_len) == 0); @@ -164,13 +163,12 @@ exit: void read_round_one(int role, data_t *msg, int ref_ret) { mbedtls_ecjpake_context ctx; + mbedtls_ecjpake_init(&ctx); const unsigned char *pw = NULL; const size_t pw_len = 0; MD_PSA_INIT(); - mbedtls_ecjpake_init(&ctx); - TEST_ASSERT(mbedtls_ecjpake_setup(&ctx, role, MBEDTLS_MD_SHA256, MBEDTLS_ECP_DP_SECP256R1, pw, pw_len) == 0); @@ -187,13 +185,12 @@ exit: void read_round_two_cli(data_t *msg, int ref_ret) { mbedtls_ecjpake_context ctx; + mbedtls_ecjpake_init(&ctx); const unsigned char *pw = NULL; const size_t pw_len = 0; MD_PSA_INIT(); - mbedtls_ecjpake_init(&ctx); - TEST_ASSERT(mbedtls_ecjpake_setup(&ctx, MBEDTLS_ECJPAKE_CLIENT, MBEDTLS_MD_SHA256, MBEDTLS_ECP_DP_SECP256R1, pw, pw_len) == 0); @@ -216,13 +213,12 @@ exit: void read_round_two_srv(data_t *msg, int ref_ret) { mbedtls_ecjpake_context ctx; + mbedtls_ecjpake_init(&ctx); const unsigned char *pw = NULL; const size_t pw_len = 0; MD_PSA_INIT(); - mbedtls_ecjpake_init(&ctx); - TEST_ASSERT(mbedtls_ecjpake_setup(&ctx, MBEDTLS_ECJPAKE_SERVER, MBEDTLS_MD_SHA256, MBEDTLS_ECP_DP_SECP256R1, pw, pw_len) == 0); diff --git a/tf-psa-crypto/tests/suites/test_suite_hmac_drbg.function b/tf-psa-crypto/tests/suites/test_suite_hmac_drbg.function index 0a50c6c534..fbe1b03dbe 100644 --- a/tf-psa-crypto/tests/suites/test_suite_hmac_drbg.function +++ b/tf-psa-crypto/tests/suites/test_suite_hmac_drbg.function @@ -41,8 +41,6 @@ void hmac_drbg_entropy_usage(int md_alg) size_t default_entropy_len; size_t expected_consumed_entropy = 0; - MD_PSA_INIT(); - mbedtls_hmac_drbg_init(&ctx); memset(buf, 0, sizeof(buf)); memset(out, 0, sizeof(out)); @@ -50,6 +48,8 @@ void hmac_drbg_entropy_usage(int md_alg) entropy.len = sizeof(buf); entropy.p = buf; + MD_PSA_INIT(); + md_info = mbedtls_md_info_from_type(md_alg); TEST_ASSERT(md_info != NULL); if (mbedtls_md_get_size(md_info) <= 20) { @@ -129,11 +129,10 @@ void hmac_drbg_seed_file(int md_alg, char *path, int ret) { const mbedtls_md_info_t *md_info; mbedtls_hmac_drbg_context ctx; + mbedtls_hmac_drbg_init(&ctx); MD_PSA_INIT(); - mbedtls_hmac_drbg_init(&ctx); - md_info = mbedtls_md_info_from_type(md_alg); TEST_ASSERT(md_info != NULL); @@ -159,12 +158,12 @@ void hmac_drbg_buf(int md_alg) mbedtls_hmac_drbg_context ctx; size_t i; - MD_PSA_INIT(); - mbedtls_hmac_drbg_init(&ctx); memset(buf, 0, sizeof(buf)); memset(out, 0, sizeof(out)); + MD_PSA_INIT(); + md_info = mbedtls_md_info_from_type(md_alg); TEST_ASSERT(md_info != NULL); TEST_ASSERT(mbedtls_hmac_drbg_seed_buf(&ctx, md_info, buf, sizeof(buf)) == 0); @@ -194,13 +193,13 @@ void hmac_drbg_no_reseed(int md_alg, data_t *entropy, const mbedtls_md_info_t *md_info; mbedtls_hmac_drbg_context ctx; - MD_PSA_INIT(); - mbedtls_hmac_drbg_init(&ctx); p_entropy.p = entropy->x; p_entropy.len = entropy->len; + MD_PSA_INIT(); + md_info = mbedtls_md_info_from_type(md_alg); TEST_ASSERT(md_info != NULL); @@ -244,13 +243,13 @@ void hmac_drbg_nopr(int md_alg, data_t *entropy, data_t *custom, const mbedtls_md_info_t *md_info; mbedtls_hmac_drbg_context ctx; - MD_PSA_INIT(); - mbedtls_hmac_drbg_init(&ctx); p_entropy.p = entropy->x; p_entropy.len = entropy->len; + MD_PSA_INIT(); + md_info = mbedtls_md_info_from_type(md_alg); TEST_ASSERT(md_info != NULL); @@ -279,13 +278,13 @@ void hmac_drbg_pr(int md_alg, data_t *entropy, data_t *custom, const mbedtls_md_info_t *md_info; mbedtls_hmac_drbg_context ctx; - MD_PSA_INIT(); - mbedtls_hmac_drbg_init(&ctx); p_entropy.p = entropy->x; p_entropy.len = entropy->len; + MD_PSA_INIT(); + md_info = mbedtls_md_info_from_type(md_alg); TEST_ASSERT(md_info != NULL); diff --git a/tf-psa-crypto/tests/suites/test_suite_md.function b/tf-psa-crypto/tests/suites/test_suite_md.function index 2a885e2371..4e62154f22 100644 --- a/tf-psa-crypto/tests/suites/test_suite_md.function +++ b/tf-psa-crypto/tests/suites/test_suite_md.function @@ -21,10 +21,10 @@ void mbedtls_md_list() const int *md_type_ptr; const mbedtls_md_info_t *info; mbedtls_md_context_t ctx; + mbedtls_md_init(&ctx); unsigned char out[MBEDTLS_MD_MAX_SIZE] = { 0 }; MD_PSA_INIT(); - mbedtls_md_init(&ctx); /* * Test that mbedtls_md_list() only returns valid MDs. @@ -87,13 +87,13 @@ void md_to_from_psa() void md_null_args() { mbedtls_md_context_t ctx; + mbedtls_md_init(&ctx); #if defined(MBEDTLS_MD_C) const mbedtls_md_info_t *info = mbedtls_md_info_from_type(*(mbedtls_md_list())); #endif unsigned char buf[1] = { 0 }; MD_PSA_INIT(); - mbedtls_md_init(&ctx); TEST_EQUAL(0, mbedtls_md_get_size(NULL)); #if defined(MBEDTLS_MD_C) @@ -245,12 +245,11 @@ void md_text_multi(int md_type, char *text_src_string, const mbedtls_md_info_t *md_info = NULL; mbedtls_md_context_t ctx, ctx_copy; - - MD_PSA_INIT(); - mbedtls_md_init(&ctx); mbedtls_md_init(&ctx_copy); + MD_PSA_INIT(); + halfway = src_len / 2; md_info = mbedtls_md_info_from_type(md_type); @@ -291,13 +290,12 @@ void md_hex_multi(int md_type, data_t *src_str, data_t *hash) unsigned char output[MBEDTLS_MD_MAX_SIZE] = { 0 }; const mbedtls_md_info_t *md_info = NULL; mbedtls_md_context_t ctx, ctx_copy; + mbedtls_md_init(&ctx); + mbedtls_md_init(&ctx_copy); int halfway; MD_PSA_INIT(); - mbedtls_md_init(&ctx); - mbedtls_md_init(&ctx_copy); - md_info = mbedtls_md_info_from_type(md_type); TEST_ASSERT(md_info != NULL); TEST_EQUAL(0, mbedtls_md_setup(&ctx, md_info, 0)); @@ -363,12 +361,11 @@ void md_hmac_multi(int md_type, int trunc_size, data_t *key_str, unsigned char output[MBEDTLS_MD_MAX_SIZE] = { 0 }; const mbedtls_md_info_t *md_info = NULL; mbedtls_md_context_t ctx; + mbedtls_md_init(&ctx); int halfway; MD_PSA_INIT(); - mbedtls_md_init(&ctx); - md_info = mbedtls_md_info_from_type(md_type); TEST_ASSERT(md_info != NULL); TEST_EQUAL(0, mbedtls_md_setup(&ctx, md_info, 1)); diff --git a/tf-psa-crypto/tests/suites/test_suite_pem.function b/tf-psa-crypto/tests/suites/test_suite_pem.function index 413dc551c3..091cbd15e5 100644 --- a/tf-psa-crypto/tests/suites/test_suite_pem.function +++ b/tf-psa-crypto/tests/suites/test_suite_pem.function @@ -66,6 +66,7 @@ void mbedtls_pem_read_buffer(char *header, char *footer, char *data, char *pwd, int res, data_t *out) { mbedtls_pem_context ctx; + mbedtls_pem_init(&ctx); int ret; size_t use_len = 0; size_t pwd_len = strlen(pwd); @@ -73,8 +74,6 @@ void mbedtls_pem_read_buffer(char *header, char *footer, char *data, MD_PSA_INIT(); - mbedtls_pem_init(&ctx); - ret = mbedtls_pem_read_buffer(&ctx, header, footer, (unsigned char *) data, (unsigned char *) pwd, pwd_len, &use_len); TEST_ASSERT(ret == res); diff --git a/tf-psa-crypto/tests/suites/test_suite_pkcs1_v21.function b/tf-psa-crypto/tests/suites/test_suite_pkcs1_v21.function index 6261979953..a15d5d7a50 100644 --- a/tf-psa-crypto/tests/suites/test_suite_pkcs1_v21.function +++ b/tf-psa-crypto/tests/suites/test_suite_pkcs1_v21.function @@ -14,18 +14,18 @@ void pkcs1_rsaes_oaep_encrypt(int mod, data_t *input_N, data_t *input_E, { unsigned char output[256]; mbedtls_rsa_context ctx; + mbedtls_rsa_init(&ctx); mbedtls_test_rnd_buf_info info; mbedtls_mpi N, E; - - MD_PSA_INIT(); + mbedtls_mpi_init(&N); mbedtls_mpi_init(&E); info.fallback_f_rng = mbedtls_test_rnd_std_rand; info.fallback_p_rng = NULL; info.buf = rnd_buf->x; info.length = rnd_buf->len; - mbedtls_mpi_init(&N); mbedtls_mpi_init(&E); - mbedtls_rsa_init(&ctx); + MD_PSA_INIT(); + TEST_ASSERT(mbedtls_rsa_set_padding(&ctx, MBEDTLS_RSA_PKCS_V21, hash) == 0); memset(output, 0x00, sizeof(output)); @@ -66,17 +66,16 @@ void pkcs1_rsaes_oaep_decrypt(int mod, data_t *input_P, data_t *input_Q, { unsigned char output[64]; mbedtls_rsa_context ctx; + mbedtls_rsa_init(&ctx); size_t output_len; mbedtls_test_rnd_pseudo_info rnd_info; mbedtls_mpi N, P, Q, E; + mbedtls_mpi_init(&N); mbedtls_mpi_init(&P); + mbedtls_mpi_init(&Q); mbedtls_mpi_init(&E); ((void) seed); MD_PSA_INIT(); - mbedtls_mpi_init(&N); mbedtls_mpi_init(&P); - mbedtls_mpi_init(&Q); mbedtls_mpi_init(&E); - - mbedtls_rsa_init(&ctx); TEST_ASSERT(mbedtls_rsa_set_padding(&ctx, MBEDTLS_RSA_PKCS_V21, hash) == 0); @@ -131,19 +130,19 @@ void pkcs1_rsassa_pss_sign(int mod, data_t *input_P, data_t *input_Q, { unsigned char output[512]; mbedtls_rsa_context ctx; + mbedtls_rsa_init(&ctx); mbedtls_test_rnd_buf_info info; mbedtls_mpi N, P, Q, E; - - MD_PSA_INIT(); + mbedtls_mpi_init(&N); mbedtls_mpi_init(&P); + mbedtls_mpi_init(&Q); mbedtls_mpi_init(&E); info.fallback_f_rng = mbedtls_test_rnd_std_rand; info.fallback_p_rng = NULL; info.buf = rnd_buf->x; info.length = rnd_buf->len; - mbedtls_mpi_init(&N); mbedtls_mpi_init(&P); - mbedtls_mpi_init(&Q); mbedtls_mpi_init(&E); - mbedtls_rsa_init(&ctx); + MD_PSA_INIT(); + TEST_ASSERT(mbedtls_rsa_set_padding(&ctx, MBEDTLS_RSA_PKCS_V21, hash) == 0); @@ -196,13 +195,13 @@ void pkcs1_rsassa_pss_verify(int mod, data_t *input_N, data_t *input_E, char *salt, data_t *result_str, int result) { mbedtls_rsa_context ctx; + mbedtls_rsa_init(&ctx); mbedtls_mpi N, E; + mbedtls_mpi_init(&N); mbedtls_mpi_init(&E); ((void) salt); MD_PSA_INIT(); - mbedtls_mpi_init(&N); mbedtls_mpi_init(&E); - mbedtls_rsa_init(&ctx); TEST_ASSERT(mbedtls_rsa_set_padding(&ctx, MBEDTLS_RSA_PKCS_V21, hash) == 0); @@ -236,12 +235,12 @@ void pkcs1_rsassa_pss_verify_ext(int mod, data_t *input_N, data_t *input_E, int result_full) { mbedtls_rsa_context ctx; + mbedtls_rsa_init(&ctx); mbedtls_mpi N, E; + mbedtls_mpi_init(&N); mbedtls_mpi_init(&E); MD_PSA_INIT(); - mbedtls_mpi_init(&N); mbedtls_mpi_init(&E); - mbedtls_rsa_init(&ctx); TEST_ASSERT(mbedtls_rsa_set_padding(&ctx, MBEDTLS_RSA_PKCS_V21, ctx_hash) == 0); diff --git a/tf-psa-crypto/tests/suites/test_suite_pkparse.function b/tf-psa-crypto/tests/suites/test_suite_pkparse.function index 15c6de039b..6f4b36de52 100644 --- a/tf-psa-crypto/tests/suites/test_suite_pkparse.function +++ b/tf-psa-crypto/tests/suites/test_suite_pkparse.function @@ -115,10 +115,10 @@ static int pk_can_ecdsa(const mbedtls_pk_context *ctx) void pk_parse_keyfile_rsa(char *key_file, char *password, int result) { mbedtls_pk_context ctx; + mbedtls_pk_init(&ctx); int res; char *pwd = password; - mbedtls_pk_init(&ctx); MD_PSA_INIT(); if (strcmp(pwd, "NULL") == 0) { @@ -162,9 +162,9 @@ exit: void pk_parse_public_keyfile_rsa(char *key_file, int result) { mbedtls_pk_context ctx; + mbedtls_pk_init(&ctx); int res; - mbedtls_pk_init(&ctx); MD_PSA_INIT(); res = mbedtls_pk_parse_public_keyfile(&ctx, key_file); diff --git a/tf-psa-crypto/tests/suites/test_suite_random.function b/tf-psa-crypto/tests/suites/test_suite_random.function index 155b8e7083..b58b22f9fe 100644 --- a/tf-psa-crypto/tests/suites/test_suite_random.function +++ b/tf-psa-crypto/tests/suites/test_suite_random.function @@ -22,7 +22,9 @@ void random_twice_with_ctr_drbg() { mbedtls_entropy_context entropy; + mbedtls_entropy_init(&entropy); mbedtls_ctr_drbg_context drbg; + mbedtls_ctr_drbg_init(&drbg); unsigned char output1[OUTPUT_SIZE]; unsigned char output2[OUTPUT_SIZE]; @@ -34,8 +36,6 @@ void random_twice_with_ctr_drbg() /* First round */ - mbedtls_entropy_init(&entropy); - mbedtls_ctr_drbg_init(&drbg); TEST_EQUAL(0, mbedtls_ctr_drbg_seed(&drbg, mbedtls_entropy_func, &entropy, NULL, 0)); @@ -73,7 +73,9 @@ exit: void random_twice_with_hmac_drbg(int md_type) { mbedtls_entropy_context entropy; + mbedtls_entropy_init(&entropy); mbedtls_hmac_drbg_context drbg; + mbedtls_hmac_drbg_init(&drbg); unsigned char output1[OUTPUT_SIZE]; unsigned char output2[OUTPUT_SIZE]; const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type(md_type); @@ -81,8 +83,6 @@ void random_twice_with_hmac_drbg(int md_type) MD_PSA_INIT(); /* First round */ - mbedtls_entropy_init(&entropy); - mbedtls_hmac_drbg_init(&drbg); TEST_EQUAL(0, mbedtls_hmac_drbg_seed(&drbg, md_info, mbedtls_entropy_func, &entropy, NULL, 0));