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.
diff --git a/ChangeLog b/ChangeLog
index cdd8257..4578146 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -9,6 +9,11 @@
* Fix unused variable/function compilation warnings in pem.c and x509_csr.c
that are reported when building mbed TLS with a config.h that does not
define MBEDTLS_PEM_PARSE_C. Found by omnium21. #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
= mbed TLS 2.1.6 branch released 2016-10-17
diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index fa7fbda..fa9d48c 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -1946,7 +1946,7 @@
/**
* \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 @@
* 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 c455625..43efe0c 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -6380,6 +6380,10 @@
*/
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 )
@@ -6387,8 +6391,12 @@
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 );
}
@@ -7120,8 +7128,8 @@
#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)