Merge pull request #3132 from mpg/fix-reconnect

Fix issues in handling of client reconnecting from the same port
This commit is contained in:
Gilles Peskine 2020-04-02 19:21:01 +02:00 committed by GitHub
commit 3124164f81
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 98 additions and 16 deletions

View file

@ -6,6 +6,14 @@ New deprecations
* Deprecate MBEDTLS_SSL_HW_RECORD_ACCEL that enables function hooks in the
SSL module for hardware acceleration of individual records.
Security
* Fix issue in DTLS handling of new associations with the same parameters
(RFC 6347 section 4.2.8): an attacker able to send forged UDP packets to
the server could cause it to drop established associations with
legitimate clients, resulting in a Denial of Service. This could only
happen when MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE was enabled in config.h
(which it is by default).
Bugfix
* Fix compilation failure when both MBEDTLS_SSL_PROTO_DTLS and
MBEDTLS_SSL_HW_RECORD_ACCEL are enabled.

View file

@ -3197,16 +3197,17 @@ static int ssl_check_dtls_clihlo_cookie(
* that looks like a ClientHello.
*
* - if the input looks like a ClientHello without cookies,
* send back HelloVerifyRequest, then
* return MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED
* send back HelloVerifyRequest, then return 0
* - if the input looks like a ClientHello with a valid cookie,
* reset the session of the current context, and
* return MBEDTLS_ERR_SSL_CLIENT_RECONNECT
* - if anything goes wrong, return a specific error code
*
* mbedtls_ssl_read_record() will ignore the record if anything else than
* MBEDTLS_ERR_SSL_CLIENT_RECONNECT or 0 is returned, although this function
* cannot not return 0.
* This function is called (through ssl_check_client_reconnect()) when an
* unexpected record is found in ssl_get_next_record(), which will discard the
* record if we return 0, and bubble up the return value otherwise (this
* includes the case of MBEDTLS_ERR_SSL_CLIENT_RECONNECT and of unexpected
* errors, and is the right thing to do in both cases).
*/
static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl )
{
@ -3218,6 +3219,8 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl )
{
/* If we can't use cookies to verify reachability of the peer,
* drop the record. */
MBEDTLS_SSL_DEBUG_MSG( 1, ( "no cookie callbacks, "
"can't check reconnect validity" ) );
return( 0 );
}
@ -3233,16 +3236,23 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl )
if( ret == MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED )
{
int send_ret;
MBEDTLS_SSL_DEBUG_MSG( 1, ( "sending HelloVerifyRequest" ) );
MBEDTLS_SSL_DEBUG_BUF( 4, "output record sent to network",
ssl->out_buf, len );
/* Don't check write errors as we can't do anything here.
* If the error is permanent we'll catch it later,
* if it's not, then hopefully it'll work next time. */
(void) ssl->f_send( ssl->p_bio, ssl->out_buf, len );
ret = 0;
send_ret = ssl->f_send( ssl->p_bio, ssl->out_buf, len );
MBEDTLS_SSL_DEBUG_RET( 2, "ssl->f_send", send_ret );
(void) send_ret;
return( 0 );
}
if( ret == 0 )
{
/* Got a valid cookie, partially reset context */
MBEDTLS_SSL_DEBUG_MSG( 1, ( "cookie is valid, resetting context" ) );
if( ( ret = mbedtls_ssl_session_reset_int( ssl, 1 ) ) != 0 )
{
MBEDTLS_SSL_DEBUG_RET( 1, "reset", ret );
@ -4415,6 +4425,7 @@ static int ssl_get_next_record( mbedtls_ssl_context *ssl )
ssl->in_msglen = rec.data_len;
ret = ssl_check_client_reconnect( ssl );
MBEDTLS_SSL_DEBUG_RET( 2, "ssl_check_client_reconnect", ret );
if( ret != 0 )
return( ret );
#endif

View file

@ -133,6 +133,7 @@ int main( void )
" modifying CID in first instance of the packet.\n" \
" protect_hvr=0/1 default: 0 (don't protect HelloVerifyRequest)\n" \
" protect_len=%%d default: (don't protect packets of this size)\n" \
" inject_clihlo=0/1 default: 0 (don't inject fake ClientHello)\n" \
"\n" \
" seed=%%d default: (use current time)\n" \
USAGE_PACK \
@ -166,6 +167,7 @@ static struct options
unsigned bad_cid; /* inject corrupted CID record */
int protect_hvr; /* never drop or delay HelloVerifyRequest */
int protect_len; /* never drop/delay packet of the given size*/
int inject_clihlo; /* inject fake ClientHello after handshake */
unsigned pack; /* merge packets into single datagram for
* at most \c merge milliseconds if > 0 */
unsigned int seed; /* seed for "random" events */
@ -314,6 +316,12 @@ static void get_options( int argc, char *argv[] )
if( opt.protect_len < 0 )
exit_usage( p, q );
}
else if( strcmp( p, "inject_clihlo" ) == 0 )
{
opt.inject_clihlo = atoi( q );
if( opt.inject_clihlo < 0 || opt.inject_clihlo > 1 )
exit_usage( p, q );
}
else if( strcmp( p, "seed" ) == 0 )
{
opt.seed = atoi( q );
@ -523,11 +531,41 @@ void print_packet( const packet *p, const char *why )
fflush( stdout );
}
/*
* In order to test the server's behaviour when receiving a ClientHello after
* the connection is established (this could be a hard reset from the client,
* but the server must not drop the existing connection before establishing
* client reachability, see RFC 6347 Section 4.2.8), we memorize the first
* ClientHello we see (which can't have a cookie), then replay it after the
* first ApplicationData record - then we're done.
*
* This is controlled by the inject_clihlo option.
*
* We want an explicit state and a place to store the packet.
*/
typedef enum {
ICH_INIT, /* haven't seen the first ClientHello yet */
ICH_CACHED, /* cached the initial ClientHello */
ICH_INJECTED, /* ClientHello already injected, done */
} inject_clihlo_state_t;
static inject_clihlo_state_t inject_clihlo_state;
static packet initial_clihlo;
int send_packet( const packet *p, const char *why )
{
int ret;
mbedtls_net_context *dst = p->dst;
/* save initial ClientHello? */
if( opt.inject_clihlo != 0 &&
inject_clihlo_state == ICH_INIT &&
strcmp( p->type, "ClientHello" ) == 0 )
{
memcpy( &initial_clihlo, p, sizeof( packet ) );
inject_clihlo_state = ICH_CACHED;
}
/* insert corrupted CID record? */
if( opt.bad_cid != 0 &&
strcmp( p->type, "CID" ) == 0 &&
@ -592,6 +630,23 @@ int send_packet( const packet *p, const char *why )
}
}
/* Inject ClientHello after first ApplicationData */
if( opt.inject_clihlo != 0 &&
inject_clihlo_state == ICH_CACHED &&
strcmp( p->type, "ApplicationData" ) == 0 )
{
print_packet( &initial_clihlo, "injected" );
if( ( ret = dispatch_data( dst, initial_clihlo.buf,
initial_clihlo.len ) ) <= 0 )
{
mbedtls_printf( " ! dispatch returned %d\n", ret );
return( ret );
}
inject_clihlo_state = ICH_INJECTED;
}
return( 0 );
}

View file

@ -7279,8 +7279,8 @@ run_test "DTLS cookie: enabled, nbio" \
not_with_valgrind # spurious resend
run_test "DTLS client reconnect from same port: reference" \
"$P_SRV dtls=1 exchanges=2 read_timeout=1000" \
"$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000" \
"$P_SRV dtls=1 exchanges=2 read_timeout=20000 hs_timeout=10000-20000" \
"$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=10000-20000" \
0 \
-C "resend" \
-S "The operation timed out" \
@ -7288,8 +7288,8 @@ run_test "DTLS client reconnect from same port: reference" \
not_with_valgrind # spurious resend
run_test "DTLS client reconnect from same port: reconnect" \
"$P_SRV dtls=1 exchanges=2 read_timeout=1000" \
"$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000 reconnect_hard=1" \
"$P_SRV dtls=1 exchanges=2 read_timeout=20000 hs_timeout=10000-20000" \
"$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=10000-20000 reconnect_hard=1" \
0 \
-C "resend" \
-S "The operation timed out" \
@ -7318,6 +7318,14 @@ run_test "DTLS client reconnect from same port: no cookies" \
-s "The operation timed out" \
-S "Client initiated reconnection from same port"
run_test "DTLS client reconnect from same port: attacker-injected" \
-p "$P_PXY inject_clihlo=1" \
"$P_SRV dtls=1 exchanges=2 debug_level=1" \
"$P_CLI dtls=1 exchanges=2" \
0 \
-s "possible client reconnect from the same port" \
-S "Client initiated reconnection from same port"
# Tests for various cases of client authentication with DTLS
# (focused on handshake flows and message parsing)
@ -8387,8 +8395,8 @@ run_test "DTLS fragmenting: 3d, openssl client, DTLS 1.0" \
not_with_valgrind # spurious resend due to timeout
run_test "DTLS proxy: reference" \
-p "$P_PXY" \
"$P_SRV dtls=1 debug_level=2" \
"$P_CLI dtls=1 debug_level=2" \
"$P_SRV dtls=1 debug_level=2 hs_timeout=10000-20000" \
"$P_CLI dtls=1 debug_level=2 hs_timeout=10000-20000" \
0 \
-C "replayed record" \
-S "replayed record" \
@ -8405,8 +8413,8 @@ run_test "DTLS proxy: reference" \
not_with_valgrind # spurious resend due to timeout
run_test "DTLS proxy: duplicate every packet" \
-p "$P_PXY duplicate=1" \
"$P_SRV dtls=1 dgram_packing=0 debug_level=2" \
"$P_CLI dtls=1 dgram_packing=0 debug_level=2" \
"$P_SRV dtls=1 dgram_packing=0 debug_level=2 hs_timeout=10000-20000" \
"$P_CLI dtls=1 dgram_packing=0 debug_level=2 hs_timeout=10000-20000" \
0 \
-c "replayed record" \
-s "replayed record" \