Merge 'development' branch
Backport of pull request #729.
diff --git a/ChangeLog b/ChangeLog
index 2d26a4f..fbc24cf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -9,6 +9,11 @@
* 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. 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.4.1 branch released 2016-12-13
diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h
index 1a6c9cc..42c9779 100644
--- a/include/mbedtls/ssl.h
+++ b/include/mbedtls/ssl.h
@@ -2183,7 +2183,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
@@ -2194,9 +2194,17 @@
* Lower values can be used to enforce policies such as "keys
* must be refreshed every N packets with cipher X".
*
+ * 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.
- * 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 121c135..abad0b3 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -6482,6 +6482,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 )
@@ -6489,8 +6493,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 );
}
@@ -7231,8 +7239,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)
diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c
index 18bda59..d98b669 100644
--- a/programs/ssl/ssl_server2.c
+++ b/programs/ssl/ssl_server2.c
@@ -63,6 +63,8 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <stdint.h>
+#include <inttypes.h>
#if !defined(_WIN32)
#include <signal.h>
@@ -113,7 +115,7 @@
#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 @@
" 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 @@
" force_ciphersuite=<name> 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 @@
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 @@
}
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 @@
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 57155b8..41fbc3d 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -1601,6 +1601,19 @@
-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" \