From 316b162ac34456bb36e7f8cef37bd6a50f191048 Mon Sep 17 00:00:00 2001 From: junyeonLEE Date: Wed, 20 Dec 2017 16:29:30 +0900 Subject: [PATCH 1/7] Separate psk and psk_identity buffers free Sometimes, psk_identity buffer can't released because psk buffer is NULL. So, separate it. --- library/ssl_tls.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 236e52d76..036876cb0 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7741,10 +7741,14 @@ void mbedtls_ssl_config_free( mbedtls_ssl_config *conf ) if( conf->psk != NULL ) { mbedtls_zeroize( conf->psk, conf->psk_len ); - mbedtls_zeroize( conf->psk_identity, conf->psk_identity_len ); mbedtls_free( conf->psk ); - mbedtls_free( conf->psk_identity ); conf->psk_len = 0; + } + + if( conf->psk_identity != NULL ) + { + mbedtls_zeroize( conf->psk_identity, conf->psk_identity_len ); + mbedtls_free( conf->psk_identity ); conf->psk_identity_len = 0; } #endif From 27e8a120b2401dbe9d8aec479d7a203cb3143398 Mon Sep 17 00:00:00 2001 From: Azim Khan Date: Wed, 21 Mar 2018 14:24:11 +0000 Subject: [PATCH 2/7] Assign NULL after freeing psk and psk_identity --- library/ssl_tls.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 036876cb0..3802e230e 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7742,6 +7742,7 @@ void mbedtls_ssl_config_free( mbedtls_ssl_config *conf ) { mbedtls_zeroize( conf->psk, conf->psk_len ); mbedtls_free( conf->psk ); + conf->psk = NULL; conf->psk_len = 0; } @@ -7749,6 +7750,7 @@ void mbedtls_ssl_config_free( mbedtls_ssl_config *conf ) { mbedtls_zeroize( conf->psk_identity, conf->psk_identity_len ); mbedtls_free( conf->psk_identity ); + conf->psk_identity = NULL; conf->psk_identity_len = 0; } #endif From bc30c5fec289ec3c10508a759e4f6a9cbaaeb05b Mon Sep 17 00:00:00 2001 From: Azim Khan Date: Thu, 22 Mar 2018 10:24:06 +0000 Subject: [PATCH 3/7] Add change log entry for mbedtls_ssl_config_free() fix --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index cfe27f3eb..28ae0b370 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.x.x branch released xxxx-xx-xx + +Changes + * Fix possible memory leak in mbedtls_ssl_config_free(). + by junyeonLEE + = mbed TLS 2.7.x branch released 2018-xx-xx Default behavior changes From 4d58881f521ed5e4fbcbda1d33aadc59c44432fa Mon Sep 17 00:00:00 2001 From: Azim Khan Date: Thu, 22 Mar 2018 12:04:25 +0000 Subject: [PATCH 4/7] Clarify bug scenario in Changlog --- ChangeLog | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 28ae0b370..7a710bdb8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,9 +2,11 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.x.x branch released xxxx-xx-xx -Changes +Bugfix * Fix possible memory leak in mbedtls_ssl_config_free(). - by junyeonLEE + This can occur only if the user doesn't use mbedtls_ssl_conf_psk() and + instead incorrectly manipulates conf->psk and/or conf->psk_identity + directly. Fix submitted by junyeonLEE. = mbed TLS 2.7.x branch released 2018-xx-xx From bdfc14e4a3a4538ccffc8f09313cbbd70ea14669 Mon Sep 17 00:00:00 2001 From: Azim Khan Date: Thu, 22 Mar 2018 12:17:36 +0000 Subject: [PATCH 5/7] Add reference to original PR in Changelog --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 7a710bdb8..13dda4db5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,7 +6,7 @@ Bugfix * Fix possible memory leak in mbedtls_ssl_config_free(). This can occur only if the user doesn't use mbedtls_ssl_conf_psk() and instead incorrectly manipulates conf->psk and/or conf->psk_identity - directly. Fix submitted by junyeonLEE. + directly. Found and fix submitted by junyeonLEE in #1220. = mbed TLS 2.7.x branch released 2018-xx-xx From f4659efedcb5a0d2e77dc1617f5ba8c26cd5d6e0 Mon Sep 17 00:00:00 2001 From: Azim Khan Date: Mon, 26 Mar 2018 22:11:24 +0100 Subject: [PATCH 6/7] Document config restrictions of psk fields --- include/mbedtls/ssl.h | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 5ee9e9d97..d0c367771 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -673,10 +673,18 @@ struct mbedtls_ssl_config #endif #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED) - unsigned char *psk; /*!< pre-shared key */ - size_t psk_len; /*!< length of the pre-shared key */ - unsigned char *psk_identity; /*!< identity for PSK negotiation */ - size_t psk_identity_len;/*!< length of identity */ + unsigned char *psk; /*!< pre-shared key. This field should + only be set via + mbedtls_ssl_conf_psk() */ + size_t psk_len; /*!< length of the pre-shared key. This + field should only be set via + mbedtls_ssl_conf_psk() */ + unsigned char *psk_identity; /*!< identity for PSK negotiation. This + field should only be set via + mbedtls_ssl_conf_psk() */ + size_t psk_identity_len;/*!< length of identity. This field should + only be set via + mbedtls_ssl_conf_psk() */ #endif #if defined(MBEDTLS_SSL_ALPN) From 71830318d3e9000395d6178fbb0c6d0088b2f73e Mon Sep 17 00:00:00 2001 From: Azim Khan Date: Wed, 28 Mar 2018 09:48:29 +0100 Subject: [PATCH 7/7] Rephrase Changelog entry Bugfix->Changes --- ChangeLog | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 13dda4db5..024854d60 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,9 +2,9 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.x.x branch released xxxx-xx-xx -Bugfix - * Fix possible memory leak in mbedtls_ssl_config_free(). - This can occur only if the user doesn't use mbedtls_ssl_conf_psk() and +Changes + * Harden mbedtls_ssl_config_free() against misuse, so that it doesn't + leak memory in case the user doesn't use mbedtls_ssl_conf_psk() and instead incorrectly manipulates conf->psk and/or conf->psk_identity directly. Found and fix submitted by junyeonLEE in #1220.