aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorManuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>2020-04-01 09:58:39 +0200
committerAndrzej Kurek <andrzej.kurek@arm.com>2020-05-21 10:12:25 -0400
commit731d7c0dcc2b6aab7fb899a7ab9d53e675dc42ef (patch)
tree2db5493bf3cc8c0ee8b4393adfc5fccdd0edc9f3
parent825ebd483fd9c3fe150e9e0bbd07c0a381d7eab8 (diff)
downloadmbed-tls-731d7c0dcc2b6aab7fb899a7ab9d53e675dc42ef.tar.gz
Fix lack of cookie check on hard reconnect
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 <manuel.pegourie-gonnard@arm.com>
-rw-r--r--library/ssl_tls.c16
-rwxr-xr-xtests/ssl-opt.sh6
2 files changed, 16 insertions, 6 deletions
diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index fea3b8a49..bfcfcbbc6 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -5235,16 +5235,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 )
{
@@ -5256,6 +5257,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 );
}
@@ -5281,7 +5284,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl )
send_ret = mbedtls_ssl_get_send( ssl )( ssl->p_bio, ssl->out_buf, len );
MBEDTLS_SSL_DEBUG_RET( 2, "mbedtls_ssl_get_send", send_ret );
(void) send_ret;
- ret = 0;
+ return( 0 );
}
if( ret == 0 )
@@ -6465,6 +6468,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
diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh
index e73daf144..b2113103e 100755
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -7090,6 +7090,7 @@ run_test "DTLS cookie: enabled, nbio" \
not_with_valgrind # spurious resend
requires_config_disabled MBEDTLS_SSL_CONF_READ_TIMEOUT
+requires_config_enabled MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
run_test "DTLS client reconnect from same port: reference" \
"$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" \
@@ -7100,6 +7101,7 @@ run_test "DTLS client reconnect from same port: reference" \
not_with_valgrind # spurious resend
requires_config_disabled MBEDTLS_SSL_CONF_READ_TIMEOUT
+requires_config_enabled MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
run_test "DTLS client reconnect from same port: reconnect" \
"$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" \
@@ -7110,6 +7112,7 @@ run_test "DTLS client reconnect from same port: reconnect" \
not_with_valgrind # server/client too slow to respond in time (next test has higher timeouts)
requires_config_disabled MBEDTLS_SSL_CONF_READ_TIMEOUT
+requires_config_enabled MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
run_test "DTLS client reconnect from same port: reconnect, nbio, no valgrind" \
"$P_SRV dtls=1 exchanges=2 read_timeout=1000 nbio=2" \
"$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-1000 reconnect_hard=1" \
@@ -7119,6 +7122,7 @@ run_test "DTLS client reconnect from same port: reconnect, nbio, no valgrind"
only_with_valgrind # Only with valgrind, do previous test but with higher read_timeout and hs_timeout
requires_config_disabled MBEDTLS_SSL_CONF_READ_TIMEOUT
+requires_config_enabled MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
run_test "DTLS client reconnect from same port: reconnect, nbio, valgrind" \
"$P_SRV dtls=1 exchanges=2 read_timeout=2000 nbio=2 hs_timeout=1500-6000" \
"$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=1500-3000 reconnect_hard=1" \
@@ -7127,6 +7131,7 @@ run_test "DTLS client reconnect from same port: reconnect, nbio, valgrind" \
-s "Client initiated reconnection from same port"
requires_config_disabled MBEDTLS_SSL_CONF_READ_TIMEOUT
+requires_config_enabled MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
run_test "DTLS client reconnect from same port: no cookies" \
"$P_SRV dtls=1 exchanges=2 read_timeout=1000 cookies=0" \
"$P_CLI dtls=1 exchanges=2 debug_level=2 hs_timeout=500-8000 reconnect_hard=1" \
@@ -7134,6 +7139,7 @@ run_test "DTLS client reconnect from same port: no cookies" \
-s "The operation timed out" \
-S "Client initiated reconnection from same port"
+requires_config_enabled MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
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" \