From 824655c8379d87a3116d258e6fd65c06073b0b40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 11 Mar 2020 12:51:42 +0100 Subject: [PATCH 1/5] Fix lack of cookie check on hard reconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Section 4.2.8 of RFC 6347 describes how to handle the case of a DTLS client establishing a new connection using the same UDP quartet as an already active connection, which we implement under the compile option MBEDTLS_SSL_DLTS_CLIENT_PORT_REUSE. Relevant excerpts: [the server] MUST NOT destroy the existing association until the client has demonstrated reachability either by completing a cookie exchange or by completing a complete handshake including delivering a verifiable Finished message. [...] The reachability requirement prevents off-path/blind attackers from destroying associations merely by sending forged ClientHellos. Our code chooses to use a cookie exchange for establishing reachability, but unfortunately that check was effectively removed in a recent refactoring, which changed what value ssl_handle_possible_reconnect() needs to return in order for ssl_get_next_record() (introduced in that refactoring) to take the proper action. Unfortunately, in addition to changing the value, the refactoring also changed a return statement to an assignment to the ret variable, causing the function to reach the code for a valid cookie, which immediately destroys the existing association, effectively bypassing the cookie verification. This commit fixes that by immediately returning after sending a HelloVerifyRequest when a ClientHello without a valid cookie is found. It also updates the description of the function to reflect the new return value convention (the refactoring updated the code but not the documentation). The commit that changed the return value convention (and introduced the bug) is 2fddd3765ea998bb9f40b52dc1baaf843b9889bf, whose commit message explains the change. Note: this bug also indirectly caused the ssl-opt.sh test case "DTLS client reconnect from same port: reconnect" to occasionally fail due to a race condition between the reception of the ClientHello carrying a valid cookie and the closure of the connection by the server after noticing the ClientHello didn't carry a valid cookie after it incorrectly destroyed the previous connection, that could cause that ClientHello to be invisible to the server (if that message reaches the server just before it does `net_close()`). A welcome side effect of this commit is to remove that race condition, as the new connection will immediately start with a ClientHello carrying a valid cookie in the SSL input buffer, so the server will not call `net_close()` and not risk discarding a better ClientHello that arrived in the meantime. Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog | 8 ++++++++ library/ssl_msg.c | 13 +++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index bcceebb7d..917c521cc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -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. diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 18fa55574..a0009d956 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -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 ) { @@ -3237,7 +3238,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) * 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; + return( 0 ); } if( ret == 0 ) From baad2de6d81417234e3db7b4b66e2c4ca03444da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 13 Mar 2020 11:11:02 +0100 Subject: [PATCH 2/5] Add negative test for hard reconnect cookie check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server must check client reachability (we chose to do that by checking a cookie) before destroying the existing association (RFC 6347 section 4.2.8). Let's make sure we do, by having a proxy-in-the-middle inject a ClientHello - the server should notice, but not destroy the connection. Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/udp_proxy.c | 54 +++++++++++++++++++++++++++++++++++++++ tests/ssl-opt.sh | 8 ++++++ 2 files changed, 62 insertions(+) diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index 979910e6b..ae2cabee4 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -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,40 @@ 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. + */ +static 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; + +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 +629,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 ); } diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 35f742f67..23efedfdd 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -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) From b6929891d69033d34eed9c4f8fb69b59fb34ba88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 9 Sep 2019 11:14:37 +0200 Subject: [PATCH 3/5] Adjust timeout of tests with "no resend" assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are currently 4 tests in ssl-opt.sh with either -C "resend" or -S "resend", that is, asserting that no retransmission will occur. They sometimes fail on loaded CI machines as one side doesn't send a message fast enough, causing the other side to retransmit, causing the test to fail. (For the "reconnect" test there was an other issue causing random failures, fixed in a previous commit, but even after that fix the test would still sometimes randomly fail, even if much more rarely.) While it's a hard problem to fix in a general and perfect way, in practice the probability of failures can be drastically reduced by making the timeout values much larger. For some tests, where retransmissions are actually expected, this would have the negative effect of increasing the average running time of the test, as each side would wait for longer before it starts retransmission, so we have a trade-off between average running time and probability of spurious failures. But for tests where retransmission is not expected, there is no such trade-off as the expected running time of the test (assuming the code is correct most of the time) is not impacted by the timeout value. So the only negative effect of increasing the timeout value is on the worst-case running time on the test, which is much less important, as test should only fail quite rarely. This commit addresses the easy case of tests that don't expect retransmission by increasing the value of their timeout range to 10s-20s. This value corresponds to the value used for tests that assert `-S "autoreduction"` which are in the same case and where the current value seems acceptable so far. It also represents an increase, compared to the values before this commit, of a factor 20 for the "reconnect" tests which were frequently observed to fail in the CI, and of a factor 10 for the first two "DTLS proxy" tests, which were observed to fail much less frequently, so hopefully the new values are enough to reduce the probability of spurious failures to an acceptable level. Signed-off-by: Manuel Pégourié-Gonnard --- tests/ssl-opt.sh | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 23efedfdd..9b6eee152 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -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" \ @@ -8395,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" \ @@ -8413,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" \ From f4563b4c8b0f806593a8f13638d17b34af083f7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 30 Mar 2020 12:46:21 +0200 Subject: [PATCH 4/5] Fix some style issues in udp_proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- programs/test/udp_proxy.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/programs/test/udp_proxy.c b/programs/test/udp_proxy.c index ae2cabee4..7447571f4 100644 --- a/programs/test/udp_proxy.c +++ b/programs/test/udp_proxy.c @@ -543,12 +543,13 @@ void print_packet( const packet *p, const char *why ) * * We want an explicit state and a place to store the packet. */ -static 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; +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 ) @@ -558,11 +559,11 @@ int send_packet( const packet *p, const char *why ) /* save initial ClientHello? */ if( opt.inject_clihlo != 0 && - inject_clihlo_state == ich_init && + inject_clihlo_state == ICH_INIT && strcmp( p->type, "ClientHello" ) == 0 ) { memcpy( &initial_clihlo, p, sizeof( packet ) ); - inject_clihlo_state = ich_cached; + inject_clihlo_state = ICH_CACHED; } /* insert corrupted CID record? */ @@ -631,7 +632,7 @@ int send_packet( const packet *p, const char *why ) /* Inject ClientHello after first ApplicationData */ if( opt.inject_clihlo != 0 && - inject_clihlo_state == ich_cached && + inject_clihlo_state == ICH_CACHED && strcmp( p->type, "ApplicationData" ) == 0 ) { print_packet( &initial_clihlo, "injected" ); @@ -643,7 +644,7 @@ int send_packet( const packet *p, const char *why ) return( ret ); } - inject_clihlo_state = ich_injected; + inject_clihlo_state = ICH_INJECTED; } return( 0 ); From 243d70f2a50cebd03f0d0328ed9196d6c813ecdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 31 Mar 2020 12:07:47 +0200 Subject: [PATCH 5/5] Improve debug logging of client hard reconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current logging was sub-standard, in particular there was no trace whatsoever of the HelloVerifyRequest being sent. Now it's being logged with the usual levels: 4 for full content, 2 return of f_send, 1 decision about sending it (or taking other branches in the same function) because that's the same level as state changes in the handshake, and also same as the "possible client reconnect" message" to which it's the logical continuation (what are we doing about it?). Signed-off-by: Manuel Pégourié-Gonnard --- library/ssl_msg.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index a0009d956..428ace5ba 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3219,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 ); } @@ -3234,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 ); + 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 ); @@ -4416,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