From 2be031e2380c457cc306f574e25c64e4d3ff7761 Mon Sep 17 00:00:00 2001 From: David Girault Date: Tue, 26 Mar 2019 10:47:47 +0100 Subject: [PATCH] altcp_tls: ensure no memory leaks and entropy counter is protected --- src/apps/altcp_tls/altcp_tls_mbedtls.c | 102 ++++++++++++++++--------- 1 file changed, 64 insertions(+), 38 deletions(-) diff --git a/src/apps/altcp_tls/altcp_tls_mbedtls.c b/src/apps/altcp_tls/altcp_tls_mbedtls.c index 671fd9a3..8e449e63 100644 --- a/src/apps/altcp_tls/altcp_tls_mbedtls.c +++ b/src/apps/altcp_tls/altcp_tls_mbedtls.c @@ -50,6 +50,7 @@ */ #include "lwip/opt.h" +#include "lwip/sys.h" #if LWIP_ALTCP /* don't build if not configured for use in lwipopts.h */ @@ -714,6 +715,55 @@ altcp_mbedtls_debug(void *ctx, int level, const char *file, int line, const char } #endif +static err_t +altcp_mbedtls_ref_entropy() +{ + SYS_ARCH_DECL_PROTECT(old_level); + SYS_ARCH_PROTECT(old_level); + if (!altcp_tls_entropy_rng) { + altcp_tls_entropy_rng = (struct altcp_tls_entropy_rng *)altcp_mbedtls_alloc_config(sizeof(struct altcp_tls_entropy_rng)); + if (altcp_tls_entropy_rng) { + int ret; + altcp_tls_entropy_rng->ref = 1; + mbedtls_entropy_init(&altcp_tls_entropy_rng->entropy); + mbedtls_ctr_drbg_init(&altcp_tls_entropy_rng->ctr_drbg); + /* Seed the RNG, only once */ + ret = mbedtls_ctr_drbg_seed(&altcp_tls_entropy_rng->ctr_drbg, + ALTCP_MBEDTLS_RNG_FN, &altcp_tls_entropy_rng->entropy, + ALTCP_MBEDTLS_ENTROPY_PTR, ALTCP_MBEDTLS_ENTROPY_LEN); + if (ret != 0) { + LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_ctr_drbg_seed failed: %d\n", ret)); + mbedtls_ctr_drbg_free(&altcp_tls_entropy_rng->ctr_drbg); + mbedtls_entropy_free(&altcp_tls_entropy_rng->entropy); + altcp_mbedtls_free_config(altcp_tls_entropy_rng); + altcp_tls_entropy_rng = NULL; + SYS_ARCH_UNPROTECT(old_level); + return ERR_ARG; + } + } + else { + SYS_ARCH_UNPROTECT(old_level); + return ERR_MEM; + } + } + else + altcp_tls_entropy_rng->ref++; + SYS_ARCH_UNPROTECT(old_level); + return ERR_OK; +} + + + +static void +altcp_mbedtls_unref_entropy() +{ + SYS_ARCH_DECL_PROTECT(old_level); + SYS_ARCH_PROTECT(old_level); + if (altcp_tls_entropy_rng && altcp_tls_entropy_rng->ref) + altcp_tls_entropy_rng->ref--; + SYS_ARCH_UNPROTECT(old_level); +} + /** Create new TLS configuration * ATTENTION: Server certificate and private key have to be added outside this function! */ @@ -764,31 +814,9 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav mbedtls_ssl_config_init(&conf->conf); - if (!altcp_tls_entropy_rng) { - altcp_tls_entropy_rng = (struct altcp_tls_entropy_rng *)altcp_mbedtls_alloc_config(sizeof(struct altcp_tls_entropy_rng)); - if (altcp_tls_entropy_rng) { - altcp_tls_entropy_rng->ref = 1; - mbedtls_entropy_init(&altcp_tls_entropy_rng->entropy); - mbedtls_ctr_drbg_init(&altcp_tls_entropy_rng->ctr_drbg); - /* Seed the RNG, only once */ - ret = mbedtls_ctr_drbg_seed(&altcp_tls_entropy_rng->ctr_drbg, - mbedtls_entropy_func, &altcp_tls_entropy_rng->entropy, - ALTCP_MBEDTLS_ENTROPY_PTR, ALTCP_MBEDTLS_ENTROPY_LEN); - if (ret != 0) { - LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_ctr_drbg_seed failed: %d\n", ret)); - mbedtls_ctr_drbg_free(&altcp_tls_entropy_rng->ctr_drbg); - mbedtls_entropy_free(&altcp_tls_entropy_rng->entropy); - altcp_mbedtls_free_config(altcp_tls_entropy_rng); - altcp_tls_entropy_rng = NULL; - altcp_mbedtls_free_config(conf); - return NULL; - } - } else { - altcp_mbedtls_free_config(conf); - return NULL; - } - } else { - altcp_tls_entropy_rng->ref++; + if (altcp_mbedtls_ref_entropy() != ERR_OK) { + altcp_mbedtls_free_config(conf); + return NULL; } /* Setup ssl context (@todo: what's different for a client here? -> might better be done on listen/connect) */ @@ -796,12 +824,7 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav MBEDTLS_SSL_TRANSPORT_STREAM, MBEDTLS_SSL_PRESET_DEFAULT); if (ret != 0) { LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_ssl_config_defaults failed: %d\n", ret)); - if (altcp_tls_entropy_rng->ref == 1) { - mbedtls_ctr_drbg_free(&altcp_tls_entropy_rng->ctr_drbg); - mbedtls_entropy_free(&altcp_tls_entropy_rng->entropy); - altcp_mbedtls_free_config(altcp_tls_entropy_rng); - altcp_tls_entropy_rng = NULL; - } + altcp_mbedtls_unref_entropy(); altcp_mbedtls_free_config(conf); return NULL; } @@ -824,6 +847,7 @@ altcp_tls_create_config(int is_server, u8_t cert_count, u8_t pkey_count, int hav ALTCP_MBEDTLS_SESSION_TICKET_CIPHER, ALTCP_MBEDTLS_SESSION_TICKET_TIMEOUT_SECONDS); if (ret) { LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_ssl_ticket_setup failed: %d\n", ret)); + altcp_mbedtls_unref_entropy(); altcp_mbedtls_free_config(conf); return NULL; } @@ -911,7 +935,7 @@ altcp_tls_create_config_server_privkey_cert(const u8_t *privkey, size_t privkey_ if (altcp_tls_config_server_add_privkey_cert(conf, privkey, privkey_len, privkey_pass, privkey_pass_len, cert, cert_len) != ERR_OK) { - altcp_mbedtls_free_config(conf); + altcp_tls_free_config(conf); return NULL; } @@ -935,7 +959,7 @@ altcp_tls_create_config_client_common(const u8_t *ca, size_t ca_len, int is_2way ret = mbedtls_x509_crt_parse(conf->ca, ca, ca_len); if (ret != 0) { LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_x509_crt_parse ca failed: %d 0x%x", ret, -1*ret)); - altcp_mbedtls_free_config(conf); + altcp_tls_free_config(conf); return NULL; } @@ -973,7 +997,7 @@ altcp_tls_create_config_client_2wayauth(const u8_t *ca, size_t ca_len, const u8_ ret = mbedtls_x509_crt_parse(conf->cert, cert, cert_len); if (ret != 0) { LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_x509_crt_parse cert failed: %d 0x%x", ret, -1*ret)); - altcp_mbedtls_free_config(conf->cert); + altcp_tls_free_config(conf); return NULL; } @@ -981,14 +1005,14 @@ altcp_tls_create_config_client_2wayauth(const u8_t *ca, size_t ca_len, const u8_ ret = mbedtls_pk_parse_key(conf->pkey, privkey, privkey_len, privkey_pass, privkey_pass_len); if (ret != 0) { LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_pk_parse_key failed: %d 0x%x", ret, -1*ret)); - altcp_mbedtls_free_config(conf); + altcp_tls_free_config(conf); return NULL; } ret = mbedtls_ssl_conf_own_cert(&conf->conf, conf->cert, conf->pkey); if (ret != 0) { LWIP_DEBUGF(ALTCP_MBEDTLS_DEBUG, ("mbedtls_ssl_conf_own_cert failed: %d 0x%x", ret, -1*ret)); - altcp_mbedtls_free_config(conf); + altcp_tls_free_config(conf); return NULL; } @@ -1025,19 +1049,21 @@ altcp_tls_free_config(struct altcp_tls_config *conf) mbedtls_entropy_free(&conf->entropy); mbedtls_ctr_drbg_free(&conf->ctr_drbg); altcp_mbedtls_free_config(conf); - if (altcp_tls_entropy_rng && altcp_tls_entropy_rng->ref) - altcp_tls_entropy_rng->ref--; + altcp_mbedtls_unref_entropy(); } void altcp_tls_free_entropy(void) { + SYS_ARCH_DECL_PROTECT(old_level); + SYS_ARCH_PROTECT(old_level); if (altcp_tls_entropy_rng && altcp_tls_entropy_rng->ref == 0) { mbedtls_ctr_drbg_free(&altcp_tls_entropy_rng->ctr_drbg); mbedtls_entropy_free(&altcp_tls_entropy_rng->entropy); altcp_mbedtls_free_config(altcp_tls_entropy_rng); altcp_tls_entropy_rng = NULL; } + SYS_ARCH_UNPROTECT(old_level); } /* "virtual" functions */