From ee7157e7359b861c20c87c19d87bca1a21bf419e Mon Sep 17 00:00:00 2001 From: Andres AG Date: Wed, 7 Dec 2016 10:25:19 +0000 Subject: [PATCH 1/8] Fix redefinition of macro ssl_set_bio Fix redefinition of macro ssl_set_bio to undefined symbol mbedtls_ssl_set_bio_timeout in compat-1.3.h. --- ChangeLog | 6 ++++++ include/mbedtls/compat-1.3.h | 1 - scripts/data_files/rename-1.3-2.0.txt | 1 - 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index f96786d72..b5f8812cd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS x.x.x branch released xxxx-xx-xx + +Bugfix + * Fix redefinition of macro ssl_set_bio to undefined symbol + mbedtls_ssl_set_bio_timeout in compat-1.3.h. #673 + = mbed TLS 2.4.0 branch released 2016-10-17 Security diff --git a/include/mbedtls/compat-1.3.h b/include/mbedtls/compat-1.3.h index 27abbd972..af51b5f82 100644 --- a/include/mbedtls/compat-1.3.h +++ b/include/mbedtls/compat-1.3.h @@ -2453,7 +2453,6 @@ #define ssl_set_arc4_support mbedtls_ssl_conf_arc4_support #define ssl_set_authmode mbedtls_ssl_conf_authmode #define ssl_set_bio mbedtls_ssl_set_bio -#define ssl_set_bio mbedtls_ssl_set_bio_timeout #define ssl_set_ca_chain mbedtls_ssl_conf_ca_chain #define ssl_set_cbc_record_splitting mbedtls_ssl_conf_cbc_record_splitting #define ssl_set_ciphersuites mbedtls_ssl_conf_ciphersuites diff --git a/scripts/data_files/rename-1.3-2.0.txt b/scripts/data_files/rename-1.3-2.0.txt index 397f6beae..cb3381ab8 100644 --- a/scripts/data_files/rename-1.3-2.0.txt +++ b/scripts/data_files/rename-1.3-2.0.txt @@ -1996,7 +1996,6 @@ ssl_set_alpn_protocols mbedtls_ssl_conf_alpn_protocols ssl_set_arc4_support mbedtls_ssl_conf_arc4_support ssl_set_authmode mbedtls_ssl_conf_authmode ssl_set_bio mbedtls_ssl_set_bio -ssl_set_bio_timeout mbedtls_ssl_set_bio_timeout ssl_set_ca_chain mbedtls_ssl_conf_ca_chain ssl_set_cbc_record_splitting mbedtls_ssl_conf_cbc_record_splitting ssl_set_ciphersuites mbedtls_ssl_conf_ciphersuites From 0368cb7f1c0a6f244fd004d3153eecd9218e5ed1 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Wed, 7 Dec 2016 15:05:53 +0000 Subject: [PATCH 2/8] Fix unused variable/function compilation warnings This PR fixes a number of unused variable/function compilation warnings that arise when using a config.h that does not define the macro MBEDTLS_PEM_PARSE_C. --- ChangeLog | 7 +++++++ library/pem.c | 2 +- library/x509_crt.c | 2 +- library/x509_csr.c | 2 +- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index f96786d72..cbf509ff9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,12 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS x.x.x branch released xxxx-xx-xx + +Bugfix + * Fix unused variable/function compilation warnings in pem.c, x509_crt.c and + x509_csr.c that are reported when building mbed TLS with a config.h that + does not define MBEDTLS_PEM_PARSE_C. #562 + = mbed TLS 2.4.0 branch released 2016-10-17 Security diff --git a/library/pem.c b/library/pem.c index 1ee3966e1..b6ad53b7d 100644 --- a/library/pem.c +++ b/library/pem.c @@ -44,12 +44,12 @@ #define mbedtls_free free #endif +#if defined(MBEDTLS_PEM_PARSE_C) /* Implementation that should never be optimized out by the compiler */ static void mbedtls_zeroize( void *v, size_t n ) { volatile unsigned char *p = v; while( n-- ) *p++ = 0; } -#if defined(MBEDTLS_PEM_PARSE_C) void mbedtls_pem_init( mbedtls_pem_context *ctx ) { memset( ctx, 0, sizeof( mbedtls_pem_context ) ); diff --git a/library/x509_crt.c b/library/x509_crt.c index 80af7259c..056dc16fe 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -969,8 +969,8 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu */ int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, const unsigned char *buf, size_t buflen ) { - int success = 0, first_error = 0, total_failed = 0; #if defined(MBEDTLS_PEM_PARSE_C) + int success = 0, first_error = 0, total_failed = 0; int buf_format = MBEDTLS_X509_FORMAT_DER; #endif diff --git a/library/x509_csr.c b/library/x509_csr.c index 603d06b64..f92b66c58 100644 --- a/library/x509_csr.c +++ b/library/x509_csr.c @@ -265,8 +265,8 @@ int mbedtls_x509_csr_parse_der( mbedtls_x509_csr *csr, */ int mbedtls_x509_csr_parse( mbedtls_x509_csr *csr, const unsigned char *buf, size_t buflen ) { - int ret; #if defined(MBEDTLS_PEM_PARSE_C) + int ret; size_t use_len; mbedtls_pem_context pem; #endif From 14918fbda4da2a857f2166f57f5c90e151cd001b Mon Sep 17 00:00:00 2001 From: Jaakko Korhonen Date: Mon, 9 Jan 2017 11:07:46 +0200 Subject: [PATCH 3/8] Fixed typo in ssl.h --- include/mbedtls/ssl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index ba499d2bd..69fc502f0 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1146,7 +1146,7 @@ void mbedtls_ssl_conf_read_timeout( mbedtls_ssl_config *conf, uint32_t timeout ) * * \note See the documentation of \c mbedtls_ssl_set_timer_t and * \c mbedtls_ssl_get_timer_t for the conventions this pair of - * callbacks must fallow. + * callbacks must follow. * * \note On some platforms, timing.c provides * \c mbedtls_timing_set_delay() and From 18c5c59b5b99107889e57f989eb818f2df79c084 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Thu, 15 Dec 2016 17:01:16 +0000 Subject: [PATCH 4/8] Fix renegotiation at incorrect times in DTLS Fix an incorrect condition in ssl_check_ctr_renegotiate() that compared 64 bits of record counter instead of 48 bits as described in RFC 6347 Section 4.3.1. This would cause the function's return value to be occasionally incorrect and the renegotiation routines to be triggered at unexpected times. --- ChangeLog | 9 +++++++++ include/mbedtls/ssl.h | 6 ++++-- library/ssl_tls.c | 16 ++++++++++++---- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0a857ba76..43aa8bb4e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS x.x.x branch released xxxx-xx-xx + +Bugfix + * Fix incorrect renegotiation condition in ssl_check_ctr_renegotiate() that + would compare 64 bits of the record counter instead of 48 bits as indicated + in RFC 6347 Section 4.3.1. This could cause the execution of the + renegotiation routines at unexpected times when the protocol is DTLS. Found + by wariua. #687 + = mbed TLS 2.4.1 branch released 2016-12-13 Changes diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 2c021900b..19fc1f192 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2183,7 +2183,7 @@ void mbedtls_ssl_conf_renegotiation_enforced( mbedtls_ssl_config *conf, int max_ /** * \brief Set record counter threshold for periodic renegotiation. - * (Default: 2^64 - 256.) + * (Default: 2^48 - 1) * * Renegotiation is automatically triggered when a record * counter (outgoing or ingoing) crosses the defined @@ -2194,9 +2194,11 @@ void mbedtls_ssl_conf_renegotiation_enforced( mbedtls_ssl_config *conf, int max_ * Lower values can be used to enforce policies such as "keys * must be refreshed every N packets with cipher X". * + * \note When the transport is set to MBEDTLS_SSL_TRANSPORT_DATAGRAM, + * the maximum renegotiation period is 2^48 - 1. + * * \param conf SSL configuration * \param period The threshold value: a big-endian 64-bit number. - * Set to 2^64 - 1 to disable periodic renegotiation */ void mbedtls_ssl_conf_renegotiation_period( mbedtls_ssl_config *conf, const unsigned char period[8] ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 121c13526..abad0b385 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6482,6 +6482,10 @@ int mbedtls_ssl_renegotiate( mbedtls_ssl_context *ssl ) */ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) { + size_t ep_len = ssl_ep_len( ssl ); + int in_ctr_cmp; + int out_ctr_cmp; + if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER || ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_PENDING || ssl->conf->disable_renegotiation == MBEDTLS_SSL_RENEGOTIATION_DISABLED ) @@ -6489,8 +6493,12 @@ static int ssl_check_ctr_renegotiate( mbedtls_ssl_context *ssl ) return( 0 ); } - if( memcmp( ssl->in_ctr, ssl->conf->renego_period, 8 ) <= 0 && - memcmp( ssl->out_ctr, ssl->conf->renego_period, 8 ) <= 0 ) + in_ctr_cmp = memcmp( ssl->in_ctr + ep_len, + ssl->conf->renego_period + ep_len, 8 - ep_len ); + out_ctr_cmp = memcmp( ssl->out_ctr + ep_len, + ssl->conf->renego_period + ep_len, 8 - ep_len ); + + if( in_ctr_cmp <= 0 && out_ctr_cmp <= 0 ) { return( 0 ); } @@ -7231,8 +7239,8 @@ int mbedtls_ssl_config_defaults( mbedtls_ssl_config *conf, #if defined(MBEDTLS_SSL_RENEGOTIATION) conf->renego_max_records = MBEDTLS_SSL_RENEGO_MAX_RECORDS_DEFAULT; - memset( conf->renego_period, 0xFF, 7 ); - conf->renego_period[7] = 0x00; + memset( conf->renego_period, 0x00, 2 ); + memset( conf->renego_period + 2, 0xFF, 6 ); #endif #if defined(MBEDTLS_DHM_C) && defined(MBEDTLS_SSL_SRV_C) From 692ad84e5cd73670786faae4c01d4d4840523f68 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Thu, 19 Jan 2017 16:30:57 +0000 Subject: [PATCH 5/8] Add DTLS test to check 6 byte record ctr is cmp Add a test to ssl-opt.sh to ensure that in DTLS a 6 byte record counter is compared in ssl_check_ctr_renegotiate() instead of a 8 byte one as in the TLS case. Because currently there are no testing facilities to check that renegotiation routines are triggered after X number of input/output messages, the test consists on setting a renegotiation period that cannot be represented in 6 bytes, but whose least-significant byte is 2. If the library behaves correctly, the renegotiation routines will be executed after two exchanged. --- programs/ssl/ssl_server2.c | 27 +++++++++++++++++++++------ tests/ssl-opt.sh | 13 +++++++++++++ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 18bda599f..d98b669b5 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -63,6 +63,8 @@ int main( void ) #include #include #include +#include +#include #if !defined(_WIN32) #include @@ -113,7 +115,7 @@ int main( void ) #define DFL_ALLOW_LEGACY -2 #define DFL_RENEGOTIATE 0 #define DFL_RENEGO_DELAY -2 -#define DFL_RENEGO_PERIOD -1 +#define DFL_RENEGO_PERIOD ( (uint64_t)-1 ) #define DFL_EXCHANGES 1 #define DFL_MIN_VERSION -1 #define DFL_MAX_VERSION -1 @@ -292,7 +294,7 @@ int main( void ) " renegotiation=%%d default: 0 (disabled)\n" \ " renegotiate=%%d default: 0 (disabled)\n" \ " renego_delay=%%d default: -2 (library default)\n" \ - " renego_period=%%d default: (library default)\n" + " renego_period=%%d default: (2^64 - 1 for TLS, 2^48 - 1 for DTLS)\n" #else #define USAGE_RENEGO "" #endif @@ -351,6 +353,19 @@ int main( void ) " force_ciphersuite= default: all enabled\n" \ " acceptable ciphersuite names:\n" + +#define PUT_UINT64_BE(out_be,in_le,i) \ +{ \ + (out_be)[(i) + 0] = (unsigned char)( ( (in_le) >> 56 ) & 0xFF ); \ + (out_be)[(i) + 1] = (unsigned char)( ( (in_le) >> 48 ) & 0xFF ); \ + (out_be)[(i) + 2] = (unsigned char)( ( (in_le) >> 40 ) & 0xFF ); \ + (out_be)[(i) + 3] = (unsigned char)( ( (in_le) >> 32 ) & 0xFF ); \ + (out_be)[(i) + 4] = (unsigned char)( ( (in_le) >> 24 ) & 0xFF ); \ + (out_be)[(i) + 5] = (unsigned char)( ( (in_le) >> 16 ) & 0xFF ); \ + (out_be)[(i) + 6] = (unsigned char)( ( (in_le) >> 8 ) & 0xFF ); \ + (out_be)[(i) + 7] = (unsigned char)( ( (in_le) >> 0 ) & 0xFF ); \ +} + /* * global options */ @@ -377,7 +392,7 @@ struct options int allow_legacy; /* allow legacy renegotiation */ int renegotiate; /* attempt renegotiation? */ int renego_delay; /* delay before enforcing renegotiation */ - int renego_period; /* period for automatic renegotiation */ + uint64_t renego_period; /* period for automatic renegotiation */ int exchanges; /* number of data exchanges */ int min_version; /* minimum protocol version accepted */ int max_version; /* maximum protocol version accepted */ @@ -1041,8 +1056,8 @@ int main( int argc, char *argv[] ) } else if( strcmp( p, "renego_period" ) == 0 ) { - opt.renego_period = atoi( q ); - if( opt.renego_period < 2 || opt.renego_period > 255 ) + if( sscanf( q, "%" SCNu64, &opt.renego_period ) != 1 || + opt.renego_period < 2 ) goto usage; } else if( strcmp( p, "exchanges" ) == 0 ) @@ -1757,7 +1772,7 @@ int main( int argc, char *argv[] ) if( opt.renego_period != DFL_RENEGO_PERIOD ) { - renego_period[7] = opt.renego_period; + PUT_UINT64_BE( renego_period, opt.renego_period, 0 ); mbedtls_ssl_conf_renegotiation_period( &conf, renego_period ); } #endif diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 57155b89d..41fbc3d29 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1601,6 +1601,19 @@ run_test "Renegotiation: DTLS, server-initiated" \ -s "=> renegotiate" \ -s "write hello request" +run_test "Renegotiation: DTLS, renego_period overflow" \ + "$P_SRV debug_level=3 dtls=1 exchanges=4 renegotiation=1 renego_period=18446462598732840962 auth_mode=optional" \ + "$P_CLI debug_level=3 dtls=1 exchanges=4 renegotiation=1" \ + 0 \ + -c "client hello, adding renegotiation extension" \ + -s "received TLS_EMPTY_RENEGOTIATION_INFO" \ + -s "found renegotiation extension" \ + -s "server hello, secure renegotiation extension" \ + -s "record counter limit reached: renegotiate" \ + -c "=> renegotiate" \ + -s "=> renegotiate" \ + -s "write hello request" \ + requires_gnutls run_test "Renegotiation: DTLS, gnutls server, client-initiated" \ "$G_SRV -u --mtu 4096" \ From 1bef2266e5109c4d0a4d3fd1962662c161274062 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Thu, 2 Feb 2017 13:08:37 +0000 Subject: [PATCH 6/8] Clarify fix for #673 in Changelog Clarified fix, and added credit. --- ChangeLog | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index a404c56da..4b7536c33 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,8 +3,9 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS x.x.x branch released xxxx-xx-xx Bugfix - * Fix redefinition of macro ssl_set_bio to undefined symbol - mbedtls_ssl_set_bio_timeout in compat-1.3.h. #673 + * Fix the redefinition of macro ssl_set_bio to an undefined symbol + mbedtls_ssl_set_bio_timeout in compat-1.3.h, by removing it. + Found by omlib-lin. #673 = mbed TLS 2.4.1 branch released 2016-12-13 From d9440b15e6a0a89739f009b7b15ca6c6471c0b3a Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Thu, 2 Feb 2017 16:17:37 +0000 Subject: [PATCH 7/8] Add credit to Changelog for #562 --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 351275b48..2d26a4f13 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,7 +8,7 @@ Bugfix Found by omlib-lin. #673 * Fix unused variable/function compilation warnings in pem.c, x509_crt.c and x509_csr.c that are reported when building mbed TLS with a config.h that - does not define MBEDTLS_PEM_PARSE_C. #562 + does not define MBEDTLS_PEM_PARSE_C. Found by omnium21. #562 = mbed TLS 2.4.1 branch released 2016-12-13 From ee75b9b417de9ae93745cd7c428c4400ca7689e1 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Fri, 3 Feb 2017 00:21:28 +0000 Subject: [PATCH 8/8] Add clarification to the TLS renegotiation period Expanded details on use of mbedtls_ssl_conf_renegotiation_period() --- include/mbedtls/ssl.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 8042693d0..42c9779c6 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -2194,8 +2194,14 @@ void mbedtls_ssl_conf_renegotiation_enforced( mbedtls_ssl_config *conf, int max_ * Lower values can be used to enforce policies such as "keys * must be refreshed every N packets with cipher X". * - * \note When the transport is set to MBEDTLS_SSL_TRANSPORT_DATAGRAM, - * the maximum renegotiation period is 2^48 - 1. + * The renegotiation period can be disabled by setting + * conf->disable_renegotiation to + * MBEDTLS_SSL_RENEGOTIATION_DISABLED. + * + * \note When the configured transport is + * MBEDTLS_SSL_TRANSPORT_DATAGRAM the maximum renegotiation + * period is 2^48 - 1, and for MBEDTLS_SSL_TRANSPORT_STREAM, + * the maximum renegotiation period is 2^64 - 1. * * \param conf SSL configuration * \param period The threshold value: a big-endian 64-bit number.