From ca8c61b81519176b5287548b0e789e97f8ca2802 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Mon, 17 Jul 2023 15:17:40 +0100 Subject: [PATCH 1/6] Provide and use internal function mbedtls_zeroize_and_free() Signed-off-by: Tom Cosgrove --- library/bignum_mod.c | 9 +++------ library/bignum_mod_raw.c | 6 ++---- library/cipher.c | 6 ++---- library/common.h | 12 ++++++++++++ library/dhm.c | 6 ++---- library/lms.c | 6 ++---- library/md.c | 3 +-- library/pem.c | 15 +++++---------- library/pk_wrap.c | 3 +-- library/pkparse.c | 12 ++++-------- library/platform_util.c | 9 +++++++++ library/psa_crypto.c | 24 ++++++++---------------- library/psa_crypto_pake.c | 3 +-- library/psa_crypto_storage.c | 11 +++-------- library/rsa.c | 12 ++++-------- library/ssl_cache.c | 6 ++---- library/ssl_msg.c | 3 +-- library/ssl_tls.c | 34 +++++++++++----------------------- library/ssl_tls12_client.c | 6 ++---- library/ssl_tls13_keys.c | 3 +-- library/x509_crl.c | 9 +++------ library/x509_crt.c | 6 ++---- library/x509_csr.c | 6 ++---- 23 files changed, 83 insertions(+), 127 deletions(-) diff --git a/library/bignum_mod.c b/library/bignum_mod.c index 84f3896d4..4d6782972 100644 --- a/library/bignum_mod.c +++ b/library/bignum_mod.c @@ -80,9 +80,8 @@ void mbedtls_mpi_mod_modulus_free(mbedtls_mpi_mod_modulus *N) switch (N->int_rep) { case MBEDTLS_MPI_MOD_REP_MONTGOMERY: if (N->rep.mont.rr != NULL) { - mbedtls_platform_zeroize((mbedtls_mpi_uint *) N->rep.mont.rr, + mbedtls_zeroize_and_free((mbedtls_mpi_uint *) N->rep.mont.rr, N->limbs * sizeof(mbedtls_mpi_uint)); - mbedtls_free((mbedtls_mpi_uint *) N->rep.mont.rr); N->rep.mont.rr = NULL; } N->rep.mont.mm = 0; @@ -295,9 +294,8 @@ int mbedtls_mpi_mod_inv(mbedtls_mpi_mod_residue *X, break; } - mbedtls_platform_zeroize(working_memory, + mbedtls_zeroize_and_free(working_memory, working_limbs * sizeof(mbedtls_mpi_uint)); - mbedtls_free(working_memory); return ret; } @@ -399,8 +397,7 @@ cleanup: if (N->int_rep == MBEDTLS_MPI_MOD_REP_MONTGOMERY && working_memory != NULL) { - mbedtls_platform_zeroize(working_memory, working_memory_len); - mbedtls_free(working_memory); + mbedtls_zeroize_and_free(working_memory, working_memory_len); } return ret; diff --git a/library/bignum_mod_raw.c b/library/bignum_mod_raw.c index bf72c1825..eff562739 100644 --- a/library/bignum_mod_raw.c +++ b/library/bignum_mod_raw.c @@ -253,8 +253,7 @@ int mbedtls_mpi_mod_raw_to_mont_rep(mbedtls_mpi_uint *X, mbedtls_mpi_core_to_mont_rep(X, X, N->p, N->limbs, N->rep.mont.mm, N->rep.mont.rr, T); - mbedtls_platform_zeroize(T, t_limbs * ciL); - mbedtls_free(T); + mbedtls_zeroize_and_free(T, t_limbs * ciL); return 0; } @@ -270,8 +269,7 @@ int mbedtls_mpi_mod_raw_from_mont_rep(mbedtls_mpi_uint *X, mbedtls_mpi_core_from_mont_rep(X, X, N->p, N->limbs, N->rep.mont.mm, T); - mbedtls_platform_zeroize(T, t_limbs * ciL); - mbedtls_free(T); + mbedtls_zeroize_and_free(T, t_limbs * ciL); return 0; } diff --git a/library/cipher.c b/library/cipher.c index 490326a6b..de7f8378e 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -231,8 +231,7 @@ void mbedtls_cipher_free(mbedtls_cipher_context_t *ctx) (void) psa_destroy_key(cipher_psa->slot); } - mbedtls_platform_zeroize(cipher_psa, sizeof(*cipher_psa)); - mbedtls_free(cipher_psa); + mbedtls_zeroize_and_free(cipher_psa, sizeof(*cipher_psa)); } mbedtls_platform_zeroize(ctx, sizeof(mbedtls_cipher_context_t)); @@ -242,9 +241,8 @@ void mbedtls_cipher_free(mbedtls_cipher_context_t *ctx) #if defined(MBEDTLS_CMAC_C) if (ctx->cmac_ctx) { - mbedtls_platform_zeroize(ctx->cmac_ctx, + mbedtls_zeroize_and_free(ctx->cmac_ctx, sizeof(mbedtls_cmac_context_t)); - mbedtls_free(ctx->cmac_ctx); } #endif diff --git a/library/common.h b/library/common.h index 839b7d119..ce92238c8 100644 --- a/library/common.h +++ b/library/common.h @@ -114,6 +114,18 @@ extern void (*mbedtls_test_hook_test_fail)(const char *test, int line, const cha */ #define MBEDTLS_ALLOW_PRIVATE_ACCESS +/** + * \brief Securely zeroize a buffer then free it. + * + * Exactly the same as consecutive calls to + * \c mbedtls_platform_zeroize() and \c mbedtls_free(), but has a + * code size savings, and potential for optimisation in the future. + * + * \param buf Buffer to be zeroized then freed. May be \c NULL. + * \param len Length of the buffer in bytes + */ +void mbedtls_zeroize_and_free(void *buf, size_t len); + /** Return an offset into a buffer. * * This is just the addition of an offset to a pointer, except that this diff --git a/library/dhm.c b/library/dhm.c index 94137a264..174137d54 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -617,8 +617,7 @@ static int load_file(const char *path, unsigned char **buf, size_t *n) if (fread(*buf, 1, *n, f) != *n) { fclose(f); - mbedtls_platform_zeroize(*buf, *n + 1); - mbedtls_free(*buf); + mbedtls_zeroize_and_free(*buf, *n + 1); return MBEDTLS_ERR_DHM_FILE_IO_ERROR; } @@ -649,8 +648,7 @@ int mbedtls_dhm_parse_dhmfile(mbedtls_dhm_context *dhm, const char *path) ret = mbedtls_dhm_parse_dhm(dhm, buf, n); - mbedtls_platform_zeroize(buf, n); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, n); return ret; } diff --git a/library/lms.c b/library/lms.c index 4a42f679a..c647730f5 100644 --- a/library/lms.c +++ b/library/lms.c @@ -537,9 +537,8 @@ static int get_merkle_path(mbedtls_lms_private_t *ctx, ret = 0; exit: - mbedtls_platform_zeroize(tree, node_bytes * + mbedtls_zeroize_and_free(tree, node_bytes * MERKLE_TREE_NODE_AM(ctx->params.type)); - mbedtls_free(tree); return ret; } @@ -700,9 +699,8 @@ int mbedtls_lms_calculate_public_key(mbedtls_lms_public_t *ctx, ret = 0; exit: - mbedtls_platform_zeroize(tree, node_bytes * + mbedtls_zeroize_and_free(tree, node_bytes * MERKLE_TREE_NODE_AM(priv_ctx->params.type)); - mbedtls_free(tree); return ret; } diff --git a/library/md.c b/library/md.c index 964d4bd30..8c0393bc7 100644 --- a/library/md.c +++ b/library/md.c @@ -346,9 +346,8 @@ void mbedtls_md_free(mbedtls_md_context_t *ctx) #if defined(MBEDTLS_MD_C) if (ctx->hmac_ctx != NULL) { - mbedtls_platform_zeroize(ctx->hmac_ctx, + mbedtls_zeroize_and_free(ctx->hmac_ctx, 2 * ctx->md_info->block_size); - mbedtls_free(ctx->hmac_ctx); } #endif diff --git a/library/pem.c b/library/pem.c index 056c98c77..bd269dda7 100644 --- a/library/pem.c +++ b/library/pem.c @@ -406,16 +406,14 @@ int mbedtls_pem_read_buffer(mbedtls_pem_context *ctx, const char *header, const } if ((ret = mbedtls_base64_decode(buf, len, &len, s1, s2 - s1)) != 0) { - mbedtls_platform_zeroize(buf, len); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, len); return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_PEM_INVALID_DATA, ret); } if (enc != 0) { #if defined(PEM_RFC1421) if (pwd == NULL) { - mbedtls_platform_zeroize(buf, len); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, len); return MBEDTLS_ERR_PEM_PASSWORD_REQUIRED; } @@ -451,13 +449,11 @@ int mbedtls_pem_read_buffer(mbedtls_pem_context *ctx, const char *header, const * Use that as a heuristic to try to detect password mismatches. */ if (len <= 2 || buf[0] != 0x30 || buf[1] > 0x83) { - mbedtls_platform_zeroize(buf, len); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, len); return MBEDTLS_ERR_PEM_PASSWORD_MISMATCH; } #else - mbedtls_platform_zeroize(buf, len); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, len); return MBEDTLS_ERR_PEM_FEATURE_UNAVAILABLE; #endif /* PEM_RFC1421 */ } @@ -471,8 +467,7 @@ int mbedtls_pem_read_buffer(mbedtls_pem_context *ctx, const char *header, const void mbedtls_pem_free(mbedtls_pem_context *ctx) { if (ctx->buf != NULL) { - mbedtls_platform_zeroize(ctx->buf, ctx->buflen); - mbedtls_free(ctx->buf); + mbedtls_zeroize_and_free(ctx->buf, ctx->buflen); } mbedtls_free(ctx->info); diff --git a/library/pk_wrap.c b/library/pk_wrap.c index 3fe2c3e0d..a4dc6556d 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -1637,8 +1637,7 @@ static void *rsa_alt_alloc_wrap(void) static void rsa_alt_free_wrap(void *ctx) { - mbedtls_platform_zeroize(ctx, sizeof(mbedtls_rsa_alt_context)); - mbedtls_free(ctx); + mbedtls_zeroize_and_free(ctx, sizeof(mbedtls_rsa_alt_context)); } const mbedtls_pk_info_t mbedtls_rsa_alt_info = { diff --git a/library/pkparse.c b/library/pkparse.c index fa0570c07..f03ace261 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -107,8 +107,7 @@ int mbedtls_pk_load_file(const char *path, unsigned char **buf, size_t *n) if (fread(*buf, 1, *n, f) != *n) { fclose(f); - mbedtls_platform_zeroize(*buf, *n); - mbedtls_free(*buf); + mbedtls_zeroize_and_free(*buf, *n); return MBEDTLS_ERR_PK_FILE_IO_ERROR; } @@ -146,8 +145,7 @@ int mbedtls_pk_parse_keyfile(mbedtls_pk_context *ctx, (const unsigned char *) pwd, strlen(pwd), f_rng, p_rng); } - mbedtls_platform_zeroize(buf, n); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, n); return ret; } @@ -167,8 +165,7 @@ int mbedtls_pk_parse_public_keyfile(mbedtls_pk_context *ctx, const char *path) ret = mbedtls_pk_parse_public_key(ctx, buf, n); - mbedtls_platform_zeroize(buf, n); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, n); return ret; } @@ -1686,8 +1683,7 @@ int mbedtls_pk_parse_key(mbedtls_pk_context *pk, ret = pk_parse_key_pkcs8_encrypted_der(pk, key_copy, keylen, pwd, pwdlen, f_rng, p_rng); - mbedtls_platform_zeroize(key_copy, keylen); - mbedtls_free(key_copy); + mbedtls_zeroize_and_free(key_copy, keylen); } if (ret == 0) { diff --git a/library/platform_util.c b/library/platform_util.c index c67b80dea..4a9a7491c 100644 --- a/library/platform_util.c +++ b/library/platform_util.c @@ -129,6 +129,15 @@ void mbedtls_platform_zeroize(void *buf, size_t len) } #endif /* MBEDTLS_PLATFORM_ZEROIZE_ALT */ +void mbedtls_zeroize_and_free(void *buf, size_t len) +{ + if (buf != NULL) { + mbedtls_platform_zeroize(buf, len); + } + + mbedtls_free(buf); +} + #if defined(MBEDTLS_HAVE_TIME_DATE) && !defined(MBEDTLS_PLATFORM_GMTIME_R_ALT) #include #if !defined(_WIN32) && (defined(unix) || \ diff --git a/library/psa_crypto.c b/library/psa_crypto.c index a2a67556d..df7057b0f 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -5148,27 +5148,23 @@ psa_status_t psa_key_derivation_abort(psa_key_derivation_operation_t *operation) /* TLS-1.2 PSK-to-MS KDF uses the same core as TLS-1.2 PRF */ PSA_ALG_IS_TLS12_PSK_TO_MS(kdf_alg)) { if (operation->ctx.tls12_prf.secret != NULL) { - mbedtls_platform_zeroize(operation->ctx.tls12_prf.secret, + mbedtls_zeroize_and_free(operation->ctx.tls12_prf.secret, operation->ctx.tls12_prf.secret_length); - mbedtls_free(operation->ctx.tls12_prf.secret); } if (operation->ctx.tls12_prf.seed != NULL) { - mbedtls_platform_zeroize(operation->ctx.tls12_prf.seed, + mbedtls_zeroize_and_free(operation->ctx.tls12_prf.seed, operation->ctx.tls12_prf.seed_length); - mbedtls_free(operation->ctx.tls12_prf.seed); } if (operation->ctx.tls12_prf.label != NULL) { - mbedtls_platform_zeroize(operation->ctx.tls12_prf.label, + mbedtls_zeroize_and_free(operation->ctx.tls12_prf.label, operation->ctx.tls12_prf.label_length); - mbedtls_free(operation->ctx.tls12_prf.label); } #if defined(MBEDTLS_PSA_BUILTIN_ALG_TLS12_PSK_TO_MS) if (operation->ctx.tls12_prf.other_secret != NULL) { - mbedtls_platform_zeroize(operation->ctx.tls12_prf.other_secret, + mbedtls_zeroize_and_free(operation->ctx.tls12_prf.other_secret, operation->ctx.tls12_prf.other_secret_length); - mbedtls_free(operation->ctx.tls12_prf.other_secret); } #endif /* MBEDTLS_PSA_BUILTIN_ALG_TLS12_PSK_TO_MS */ status = PSA_SUCCESS; @@ -5187,9 +5183,8 @@ psa_status_t psa_key_derivation_abort(psa_key_derivation_operation_t *operation) #if defined(MBEDTLS_PSA_BUILTIN_ALG_PBKDF2_HMAC) if (PSA_ALG_IS_PBKDF2_HMAC(kdf_alg)) { if (operation->ctx.pbkdf2.salt != NULL) { - mbedtls_platform_zeroize(operation->ctx.pbkdf2.salt, + mbedtls_zeroize_and_free(operation->ctx.pbkdf2.salt, operation->ctx.pbkdf2.salt_length); - mbedtls_free(operation->ctx.pbkdf2.salt); } status = PSA_SUCCESS; @@ -6549,8 +6544,7 @@ static psa_status_t psa_tls12_prf_psk_to_ms_set_key( status = psa_tls12_prf_set_key(prf, pms, cur - pms); - mbedtls_platform_zeroize(pms, pms_len); - mbedtls_free(pms); + mbedtls_zeroize_and_free(pms, pms_len); return status; } @@ -7988,8 +7982,7 @@ static psa_status_t psa_pake_complete_inputs( status = psa_driver_wrapper_pake_setup(operation, &inputs); /* Driver is responsible for creating its own copy of the password. */ - mbedtls_platform_zeroize(inputs.password, inputs.password_len); - mbedtls_free(inputs.password); + mbedtls_zeroize_and_free(inputs.password, inputs.password_len); /* User and peer are translated to role. */ mbedtls_free(inputs.user); @@ -8290,9 +8283,8 @@ psa_status_t psa_pake_abort( if (operation->stage == PSA_PAKE_OPERATION_STAGE_COLLECT_INPUTS) { if (operation->data.inputs.password != NULL) { - mbedtls_platform_zeroize(operation->data.inputs.password, + mbedtls_zeroize_and_free(operation->data.inputs.password, operation->data.inputs.password_len); - mbedtls_free(operation->data.inputs.password); } if (operation->data.inputs.user != NULL) { mbedtls_free(operation->data.inputs.user); diff --git a/library/psa_crypto_pake.c b/library/psa_crypto_pake.c index c2e7dba24..caba5a115 100644 --- a/library/psa_crypto_pake.c +++ b/library/psa_crypto_pake.c @@ -559,8 +559,7 @@ psa_status_t mbedtls_psa_pake_get_implicit_key( psa_status_t mbedtls_psa_pake_abort(mbedtls_psa_pake_operation_t *operation) { - mbedtls_platform_zeroize(operation->password, operation->password_len); - mbedtls_free(operation->password); + mbedtls_zeroize_and_free(operation->password, operation->password_len); operation->password = NULL; operation->password_len = 0; diff --git a/library/psa_crypto_storage.c b/library/psa_crypto_storage.c index a8ed93753..574d4b05e 100644 --- a/library/psa_crypto_storage.c +++ b/library/psa_crypto_storage.c @@ -354,18 +354,14 @@ psa_status_t psa_save_persistent_key(const psa_core_key_attributes_t *attr, status = psa_crypto_storage_store(attr->id, storage_data, storage_data_length); - mbedtls_platform_zeroize(storage_data, storage_data_length); - mbedtls_free(storage_data); + mbedtls_zeroize_and_free(storage_data, storage_data_length); return status; } void psa_free_persistent_key_data(uint8_t *key_data, size_t key_data_length) { - if (key_data != NULL) { - mbedtls_platform_zeroize(key_data, key_data_length); - } - mbedtls_free(key_data); + mbedtls_zeroize_and_free(key_data, key_data_length); } psa_status_t psa_load_persistent_key(psa_core_key_attributes_t *attr, @@ -403,8 +399,7 @@ psa_status_t psa_load_persistent_key(psa_core_key_attributes_t *attr, } exit: - mbedtls_platform_zeroize(loaded_data, storage_data_length); - mbedtls_free(loaded_data); + mbedtls_zeroize_and_free(loaded_data, storage_data_length); return status; } diff --git a/library/rsa.c b/library/rsa.c index 8126ae9cf..970a56bd0 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1905,10 +1905,8 @@ int mbedtls_rsa_rsassa_pkcs1_v15_sign(mbedtls_rsa_context *ctx, memcpy(sig, sig_try, ctx->len); cleanup: - mbedtls_platform_zeroize(sig_try, ctx->len); - mbedtls_platform_zeroize(verif, ctx->len); - mbedtls_free(sig_try); - mbedtls_free(verif); + mbedtls_zeroize_and_free(sig_try, ctx->len); + mbedtls_zeroize_and_free(verif, ctx->len); if (ret != 0) { memset(sig, '!', ctx->len); @@ -2152,13 +2150,11 @@ int mbedtls_rsa_rsassa_pkcs1_v15_verify(mbedtls_rsa_context *ctx, cleanup: if (encoded != NULL) { - mbedtls_platform_zeroize(encoded, sig_len); - mbedtls_free(encoded); + mbedtls_zeroize_and_free(encoded, sig_len); } if (encoded_expected != NULL) { - mbedtls_platform_zeroize(encoded_expected, sig_len); - mbedtls_free(encoded_expected); + mbedtls_zeroize_and_free(encoded_expected, sig_len); } return ret; diff --git a/library/ssl_cache.c b/library/ssl_cache.c index e29b0bcd2..1c285ec3c 100644 --- a/library/ssl_cache.c +++ b/library/ssl_cache.c @@ -131,8 +131,7 @@ static void ssl_cache_entry_zeroize(mbedtls_ssl_cache_entry *entry) /* zeroize and free session structure */ if (entry->session != NULL) { - mbedtls_platform_zeroize(entry->session, entry->session_len); - mbedtls_free(entry->session); + mbedtls_zeroize_and_free(entry->session, entry->session_len); } /* zeroize the whole entry structure */ @@ -324,8 +323,7 @@ exit: #endif if (session_serialized != NULL) { - mbedtls_platform_zeroize(session_serialized, session_serialized_len); - mbedtls_free(session_serialized); + mbedtls_zeroize_and_free(session_serialized, session_serialized_len); session_serialized = NULL; } diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 2aba17b57..1a314a87f 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5829,8 +5829,7 @@ static void ssl_buffering_free_slot(mbedtls_ssl_context *ssl, if (hs_buf->is_valid == 1) { hs->buffering.total_bytes_buffered -= hs_buf->data_len; - mbedtls_platform_zeroize(hs_buf->data, hs_buf->data_len); - mbedtls_free(hs_buf->data); + mbedtls_zeroize_and_free(hs_buf->data, hs_buf->data_len); memset(hs_buf, 0, sizeof(mbedtls_ssl_hs_buffer)); } } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d18b80a72..24a103f82 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -327,8 +327,7 @@ static int resize_buffer(unsigned char **buffer, size_t len_new, size_t *len_old * lost, are done outside of this function. */ memcpy(resized_buffer, *buffer, (len_new < *len_old) ? len_new : *len_old); - mbedtls_platform_zeroize(*buffer, *len_old); - mbedtls_free(*buffer); + mbedtls_zeroize_and_free(*buffer, *len_old); *buffer = resized_buffer; *len_old = len_new; @@ -2123,9 +2122,7 @@ static void ssl_conf_remove_psk(mbedtls_ssl_config *conf) } #endif /* MBEDTLS_USE_PSA_CRYPTO */ if (conf->psk != NULL) { - mbedtls_platform_zeroize(conf->psk, conf->psk_len); - - mbedtls_free(conf->psk); + mbedtls_zeroize_and_free(conf->psk, conf->psk_len); conf->psk = NULL; conf->psk_len = 0; } @@ -2217,9 +2214,8 @@ static void ssl_remove_psk(mbedtls_ssl_context *ssl) } #else if (ssl->handshake->psk != NULL) { - mbedtls_platform_zeroize(ssl->handshake->psk, + mbedtls_zeroize_and_free(ssl->handshake->psk, ssl->handshake->psk_len); - mbedtls_free(ssl->handshake->psk); ssl->handshake->psk_len = 0; } #endif /* MBEDTLS_USE_PSA_CRYPTO */ @@ -2975,8 +2971,7 @@ int mbedtls_ssl_set_hostname(mbedtls_ssl_context *ssl, const char *hostname) * so we can free it safely */ if (ssl->hostname != NULL) { - mbedtls_platform_zeroize(ssl->hostname, strlen(ssl->hostname)); - mbedtls_free(ssl->hostname); + mbedtls_zeroize_and_free(ssl->hostname, strlen(ssl->hostname)); } /* Passing NULL as hostname shall clear the old one */ @@ -4177,8 +4172,7 @@ void mbedtls_ssl_handshake_free(mbedtls_ssl_context *ssl) } #else if (handshake->psk != NULL) { - mbedtls_platform_zeroize(handshake->psk, handshake->psk_len); - mbedtls_free(handshake->psk); + mbedtls_zeroize_and_free(handshake->psk, handshake->psk_len); } #endif /* MBEDTLS_USE_PSA_CRYPTO */ #endif /* MBEDTLS_SSL_HANDSHAKE_WITH_PSK_ENABLED */ @@ -4851,8 +4845,7 @@ void mbedtls_ssl_free(mbedtls_ssl_context *ssl) size_t out_buf_len = MBEDTLS_SSL_OUT_BUFFER_LEN; #endif - mbedtls_platform_zeroize(ssl->out_buf, out_buf_len); - mbedtls_free(ssl->out_buf); + mbedtls_zeroize_and_free(ssl->out_buf, out_buf_len); ssl->out_buf = NULL; } @@ -4863,8 +4856,7 @@ void mbedtls_ssl_free(mbedtls_ssl_context *ssl) size_t in_buf_len = MBEDTLS_SSL_IN_BUFFER_LEN; #endif - mbedtls_platform_zeroize(ssl->in_buf, in_buf_len); - mbedtls_free(ssl->in_buf); + mbedtls_zeroize_and_free(ssl->in_buf, in_buf_len); ssl->in_buf = NULL; } @@ -4898,8 +4890,7 @@ void mbedtls_ssl_free(mbedtls_ssl_context *ssl) #if defined(MBEDTLS_X509_CRT_PARSE_C) if (ssl->hostname != NULL) { - mbedtls_platform_zeroize(ssl->hostname, strlen(ssl->hostname)); - mbedtls_free(ssl->hostname); + mbedtls_zeroize_and_free(ssl->hostname, strlen(ssl->hostname)); } #endif @@ -5382,15 +5373,13 @@ void mbedtls_ssl_config_free(mbedtls_ssl_config *conf) } #endif /* MBEDTLS_USE_PSA_CRYPTO */ if (conf->psk != NULL) { - mbedtls_platform_zeroize(conf->psk, conf->psk_len); - mbedtls_free(conf->psk); + mbedtls_zeroize_and_free(conf->psk, conf->psk_len); conf->psk = NULL; conf->psk_len = 0; } if (conf->psk_identity != NULL) { - mbedtls_platform_zeroize(conf->psk_identity, conf->psk_identity_len); - mbedtls_free(conf->psk_identity); + mbedtls_zeroize_and_free(conf->psk_identity, conf->psk_identity_len); conf->psk_identity = NULL; conf->psk_identity_len = 0; } @@ -9549,9 +9538,8 @@ int mbedtls_ssl_session_set_hostname(mbedtls_ssl_session *session, /* Now it's clear that we will overwrite the old hostname, * so we can free it safely */ if (session->hostname != NULL) { - mbedtls_platform_zeroize(session->hostname, + mbedtls_zeroize_and_free(session->hostname, strlen(session->hostname)); - mbedtls_free(session->hostname); } /* Passing NULL as hostname shall clear the old one */ diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c index 4d8442eca..5a98165d0 100644 --- a/library/ssl_tls12_client.c +++ b/library/ssl_tls12_client.c @@ -3459,16 +3459,14 @@ static int ssl_parse_new_session_ticket(mbedtls_ssl_context *ssl) } if (ssl->session != NULL && ssl->session->ticket != NULL) { - mbedtls_platform_zeroize(ssl->session->ticket, + mbedtls_zeroize_and_free(ssl->session->ticket, ssl->session->ticket_len); - mbedtls_free(ssl->session->ticket); ssl->session->ticket = NULL; ssl->session->ticket_len = 0; } - mbedtls_platform_zeroize(ssl->session_negotiate->ticket, + mbedtls_zeroize_and_free(ssl->session_negotiate->ticket, ssl->session_negotiate->ticket_len); - mbedtls_free(ssl->session_negotiate->ticket); ssl->session_negotiate->ticket = NULL; ssl->session_negotiate->ticket_len = 0; diff --git a/library/ssl_tls13_keys.c b/library/ssl_tls13_keys.c index 18997e96b..afd84a974 100644 --- a/library/ssl_tls13_keys.c +++ b/library/ssl_tls13_keys.c @@ -1559,8 +1559,7 @@ static int ssl_tls13_key_schedule_stage_handshake(mbedtls_ssl_context *ssl) cleanup: if (shared_secret != NULL) { - mbedtls_platform_zeroize(shared_secret, shared_secret_len); - mbedtls_free(shared_secret); + mbedtls_zeroize_and_free(shared_secret, shared_secret_len); } return ret; diff --git a/library/x509_crl.c b/library/x509_crl.c index f6442030d..79ace8fa0 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -587,8 +587,7 @@ int mbedtls_x509_crl_parse_file(mbedtls_x509_crl *chain, const char *path) ret = mbedtls_x509_crl_parse(chain, buf, n); - mbedtls_platform_zeroize(buf, n); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, n); return ret; } @@ -704,14 +703,12 @@ void mbedtls_x509_crl_free(mbedtls_x509_crl *crl) while (entry_cur != NULL) { entry_prv = entry_cur; entry_cur = entry_cur->next; - mbedtls_platform_zeroize(entry_prv, + mbedtls_zeroize_and_free(entry_prv, sizeof(mbedtls_x509_crl_entry)); - mbedtls_free(entry_prv); } if (crl_cur->raw.p != NULL) { - mbedtls_platform_zeroize(crl_cur->raw.p, crl_cur->raw.len); - mbedtls_free(crl_cur->raw.p); + mbedtls_zeroize_and_free(crl_cur->raw.p, crl_cur->raw.len); } crl_prv = crl_cur; diff --git a/library/x509_crt.c b/library/x509_crt.c index 30e9668b2..b40bad2f4 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1526,8 +1526,7 @@ int mbedtls_x509_crt_parse_file(mbedtls_x509_crt *chain, const char *path) ret = mbedtls_x509_crt_parse(chain, buf, n); - mbedtls_platform_zeroize(buf, n); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, n); return ret; } @@ -3258,8 +3257,7 @@ void mbedtls_x509_crt_free(mbedtls_x509_crt *crt) mbedtls_asn1_sequence_free(cert_cur->authority_key_id.authorityCertIssuer.next); if (cert_cur->raw.p != NULL && cert_cur->own_buffer) { - mbedtls_platform_zeroize(cert_cur->raw.p, cert_cur->raw.len); - mbedtls_free(cert_cur->raw.p); + mbedtls_zeroize_and_free(cert_cur->raw.p, cert_cur->raw.len); } cert_prv = cert_cur; diff --git a/library/x509_csr.c b/library/x509_csr.c index cd117cbd4..0b2bb6f3b 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -460,8 +460,7 @@ int mbedtls_x509_csr_parse_file(mbedtls_x509_csr *csr, const char *path) ret = mbedtls_x509_csr_parse(csr, buf, n); - mbedtls_platform_zeroize(buf, n); - mbedtls_free(buf); + mbedtls_zeroize_and_free(buf, n); return ret; } @@ -578,8 +577,7 @@ void mbedtls_x509_csr_free(mbedtls_x509_csr *csr) mbedtls_asn1_sequence_free(csr->subject_alt_names.next); if (csr->raw.p != NULL) { - mbedtls_platform_zeroize(csr->raw.p, csr->raw.len); - mbedtls_free(csr->raw.p); + mbedtls_zeroize_and_free(csr->raw.p, csr->raw.len); } mbedtls_platform_zeroize(csr, sizeof(mbedtls_x509_csr)); From 3a11bb82131c266018486b50c004f409497992b2 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Tue, 18 Jul 2023 16:26:29 +0100 Subject: [PATCH 2/6] Better wording around passing NULL to mbedtls_zeroize_and_free() Signed-off-by: Tom Cosgrove --- library/common.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/library/common.h b/library/common.h index ce92238c8..b68089e18 100644 --- a/library/common.h +++ b/library/common.h @@ -117,11 +117,13 @@ extern void (*mbedtls_test_hook_test_fail)(const char *test, int line, const cha /** * \brief Securely zeroize a buffer then free it. * - * Exactly the same as consecutive calls to - * \c mbedtls_platform_zeroize() and \c mbedtls_free(), but has a + * Similar to making consecutive calls to + * \c mbedtls_platform_zeroize() and \c mbedtls_free(), but has * code size savings, and potential for optimisation in the future. * - * \param buf Buffer to be zeroized then freed. May be \c NULL. + * Guaranteed to be a no-op if \p buf is \c NULL and \p len is 0. + * + * \param buf Buffer to be zeroized then freed. * \param len Length of the buffer in bytes */ void mbedtls_zeroize_and_free(void *buf, size_t len); From 46259f670fb7942fe0003e6c22e817f4eb4382b6 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Tue, 18 Jul 2023 16:44:14 +0100 Subject: [PATCH 3/6] Internal function mbedtls_mpi_zeroize() can be mbedtls_mpi_zeroize_and_free() Signed-off-by: Tom Cosgrove --- library/bignum.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 36effaf8d..70081de09 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -55,9 +55,9 @@ MBEDTLS_INTERNAL_VALIDATE(cond) /* Implementation that should never be optimized out by the compiler */ -static void mbedtls_mpi_zeroize(mbedtls_mpi_uint *v, size_t n) +static void mbedtls_mpi_zeroize_and_free(mbedtls_mpi_uint *v, size_t n) { - mbedtls_platform_zeroize(v, ciL * n); + mbedtls_zeroize_and_free(v, ciL * n); } /* @@ -82,8 +82,7 @@ void mbedtls_mpi_free(mbedtls_mpi *X) } if (X->p != NULL) { - mbedtls_mpi_zeroize(X->p, X->n); - mbedtls_free(X->p); + mbedtls_mpi_zeroize_and_free(X->p, X->n); } X->s = 1; @@ -110,8 +109,7 @@ int mbedtls_mpi_grow(mbedtls_mpi *X, size_t nblimbs) if (X->p != NULL) { memcpy(p, X->p, X->n * ciL); - mbedtls_mpi_zeroize(X->p, X->n); - mbedtls_free(X->p); + mbedtls_mpi_zeroize_and_free(X->p, X->n); } X->n = nblimbs; @@ -158,8 +156,7 @@ int mbedtls_mpi_shrink(mbedtls_mpi *X, size_t nblimbs) if (X->p != NULL) { memcpy(p, X->p, i * ciL); - mbedtls_mpi_zeroize(X->p, X->n); - mbedtls_free(X->p); + mbedtls_mpi_zeroize_and_free(X->p, X->n); } X->n = i; From 350226f636c4d75639c9b96316a9a58c7a0dc8a8 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Tue, 25 Jul 2023 14:58:25 +0100 Subject: [PATCH 4/6] Use a macro for mbedtls_mpi_zeroize_and_free() Signed-off-by: Tom Cosgrove --- library/bignum.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 70081de09..06626c69c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -55,10 +55,7 @@ MBEDTLS_INTERNAL_VALIDATE(cond) /* Implementation that should never be optimized out by the compiler */ -static void mbedtls_mpi_zeroize_and_free(mbedtls_mpi_uint *v, size_t n) -{ - mbedtls_zeroize_and_free(v, ciL * n); -} +#define mbedtls_mpi_zeroize_and_free(v, n) mbedtls_zeroize_and_free(v, ciL * n) /* * Initialize one MPI From bc345e8685f2e5c5ce9e9a9d2f0c61f7cd7f9e06 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Tue, 25 Jul 2023 15:17:39 +0100 Subject: [PATCH 5/6] Protect macro parameter expansion with parentheses Signed-off-by: Tom Cosgrove --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 06626c69c..3cbeedc46 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -55,7 +55,7 @@ MBEDTLS_INTERNAL_VALIDATE(cond) /* Implementation that should never be optimized out by the compiler */ -#define mbedtls_mpi_zeroize_and_free(v, n) mbedtls_zeroize_and_free(v, ciL * n) +#define mbedtls_mpi_zeroize_and_free(v, n) mbedtls_zeroize_and_free(v, ciL * (n)) /* * Initialize one MPI From 52f7e18042babe6e121d01323b8c2608a3b287e5 Mon Sep 17 00:00:00 2001 From: Tom Cosgrove Date: Tue, 1 Aug 2023 09:08:48 +0100 Subject: [PATCH 6/6] Use mbedtls_zeroize_and_free() in psa_remove_key_data_from_memory() Signed-off-by: Tom Cosgrove --- library/psa_crypto.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/library/psa_crypto.c b/library/psa_crypto.c index df7057b0f..f6ad775ad 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1088,13 +1088,10 @@ static psa_status_t psa_get_and_lock_transparent_key_slot_with_policy( psa_status_t psa_remove_key_data_from_memory(psa_key_slot_t *slot) { - /* Data pointer will always be either a valid pointer or NULL in an - * initialized slot, so we can just free it. */ if (slot->key.data != NULL) { - mbedtls_platform_zeroize(slot->key.data, slot->key.bytes); + mbedtls_zeroize_and_free(slot->key.data, slot->key.bytes); } - mbedtls_free(slot->key.data); slot->key.data = NULL; slot->key.bytes = 0;