Merge 'iotssl-1099-incorrect-renego-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.
This commit is contained in:
Simon Butcher
commit
1d4db5b7b5
5 changed files with 55 additions and 12 deletions
|
|
@ -9,6 +9,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. 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
|
||||
|
||||
|
|
|
|||
|
|
@ -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] );
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -63,6 +63,8 @@ int main( void )
|
|||
#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 @@ 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=<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 @@ 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
|
||||
|
|
|
|||
|
|
@ -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" \
|
||||
|
|
|
|||
Loading…
Reference in a new issue