diff --git a/ChangeLog b/ChangeLog index eb729a3a0..9fcebc7d3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,6 +8,11 @@ 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 + * 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 * Fixed multiple buffer overreads in mbedtls_pem_read_buffer() when parsing the input string in PEM format to extract the different components. Found by Eyal Itkin. diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 1a6c9cc60..8042693d0 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)