From 428f28d8d7db85e0bec428b43200b02be97ba3c3 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Thu, 15 Dec 2016 17:01:16 +0000 Subject: [PATCH] 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 92d7977e0..edb32ae89 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.1.6 branch released 2016-10-17 Security diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index fa7fbda28..fa9d48cdc 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1946,7 +1946,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 @@ -1957,9 +1957,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 d44264208..0c88d2ea2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6379,6 +6379,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 ) @@ -6386,8 +6390,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 ); } @@ -7119,8 +7127,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)