diff --git a/library/ssl_msg.c b/library/ssl_msg.c index abb5a5696..87d64788b 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -275,6 +275,7 @@ exit: /* Forward declarations for functions related to message buffering. */ static void ssl_buffering_free_slot(mbedtls_ssl_context *ssl, uint8_t slot); +static void ssl_buffering_shift_slots(mbedtls_ssl_context *ssl, unsigned shift); static void ssl_free_buffered_record(mbedtls_ssl_context *ssl); MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_load_buffered_message(mbedtls_ssl_context *ssl); @@ -2985,8 +2986,12 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) * expected `message_seq` for the incoming and outgoing * handshake messages. */ - ssl->handshake->in_msg_seq = recv_msg_seq; - ssl->handshake->out_msg_seq = recv_msg_seq; + if ((ssl->handshake->in_msg_seq == 0) && (recv_msg_seq > 0)) { + MBEDTLS_SSL_DEBUG_MSG(3, ("shift slots by %u", recv_msg_seq)); + ssl_buffering_shift_slots(ssl, recv_msg_seq); + ssl->handshake->in_msg_seq = recv_msg_seq; + ssl->handshake->out_msg_seq = recv_msg_seq; + } /* Epoch should be 0 for initial handshakes */ if (ssl->in_ctr[0] != 0 || ssl->in_ctr[1] != 0) { @@ -2996,6 +3001,7 @@ int mbedtls_ssl_prepare_handshake_record(mbedtls_ssl_context *ssl) memcpy(&ssl->cur_out_ctr[2], ssl->in_ctr + 2, sizeof(ssl->cur_out_ctr) - 2); + } else if (mbedtls_ssl_is_handshake_over(ssl) == 1) { /* In case of a post-handshake ClientHello that initiates a * renegotiation check that the handshake message sequence @@ -3180,28 +3186,10 @@ int mbedtls_ssl_update_handshake_status(mbedtls_ssl_context *ssl) #if defined(MBEDTLS_SSL_PROTO_DTLS) if (ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && ssl->handshake != NULL) { - unsigned offset; - mbedtls_ssl_hs_buffer *hs_buf; /* Increment handshake sequence number */ hs->in_msg_seq++; - - /* - * Clear up handshake buffering and reassembly structure. - */ - - /* Free first entry */ - ssl_buffering_free_slot(ssl, 0); - - /* Shift all other entries */ - for (offset = 0, hs_buf = &hs->buffering.hs[0]; - offset + 1 < MBEDTLS_SSL_MAX_BUFFERED_HS; - offset++, hs_buf++) { - *hs_buf = *(hs_buf + 1); - } - - /* Create a fresh last entry */ - memset(hs_buf, 0, sizeof(mbedtls_ssl_hs_buffer)); + ssl_buffering_shift_slots(ssl, 1); } #endif return 0; @@ -3618,7 +3606,7 @@ static int ssl_parse_record_header(mbedtls_ssl_context const *ssl, ( "datagram of length %u too small to hold DTLS record header of length %u", (unsigned) len, - (unsigned) (rec_hdr_len_len + rec_hdr_len_len))); + (unsigned) (rec_hdr_len_offset + rec_hdr_len_len))); return MBEDTLS_ERR_SSL_INVALID_RECORD; } @@ -4750,6 +4738,31 @@ static int ssl_get_next_record(mbedtls_ssl_context *ssl) ret = MBEDTLS_ERR_SSL_UNEXPECTED_RECORD; } +#if defined(MBEDTLS_SSL_SRV_C) + /* + * In DTLS, invalid records are usually ignored because it is easy + * for an attacker to inject UDP datagrams, and we do not want such + * packets to disrupt the entire connection. + * + * However, when expecting the ClientHello, we reject invalid or + * unexpected records. This avoids waiting for further records + * before receiving at least one valid message. Such records could + * be leftover messages from a previous connection, accidental + * input, or part of a DoS attempt. + * + * Since no valid message has been received yet, immediately + * closing the connection does not result in any loss. + */ + if ((ssl->conf->endpoint == MBEDTLS_SSL_IS_SERVER) && + (ssl->state == MBEDTLS_SSL_CLIENT_HELLO) +#if defined(MBEDTLS_SSL_RENEGOTIATION) + && (ssl->renego_status == MBEDTLS_SSL_INITIAL_HANDSHAKE) +#endif + ) { + return ret; + } +#endif /* MBEDTLS_SSL_SRV_C */ + if (ret == MBEDTLS_ERR_SSL_UNEXPECTED_RECORD) { #if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) /* Reset in pointers to default state for TLS/DTLS records, @@ -6134,6 +6147,42 @@ static void ssl_buffering_free_slot(mbedtls_ssl_context *ssl, } } +/* + * Shift the buffering slots to the left by `shift` positions. + * After the operation, slot i contains the previous slot i + shift. + */ +static void ssl_buffering_shift_slots(mbedtls_ssl_context *ssl, + unsigned shift) +{ + mbedtls_ssl_handshake_params * const hs = ssl->handshake; + unsigned offset; + + if (shift == 0) { + return; + } + + if (shift >= MBEDTLS_SSL_MAX_BUFFERED_HS) { + shift = MBEDTLS_SSL_MAX_BUFFERED_HS; + } + + /* Free discarded entries */ + for (offset = 0; offset < shift; offset++) { + ssl_buffering_free_slot(ssl, offset); + } + + /* Shift remaining entries left */ + for (offset = 0; offset + shift < MBEDTLS_SSL_MAX_BUFFERED_HS; offset++) { + hs->buffering.hs[offset] = hs->buffering.hs[offset + shift]; + } + + /* Reset the remaining entries at the end. Some may already have been + * cleared by the loop freeing the discarded entries, but resetting all + * of them is simpler and avoids tracking which ones were already handled. + */ + for (; offset < MBEDTLS_SSL_MAX_BUFFERED_HS; offset++) { + memset(&hs->buffering.hs[offset], 0, sizeof(hs->buffering.hs[offset])); + } +} #endif /* MBEDTLS_SSL_PROTO_DTLS */ /* diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 94e61a8ac..26ba8590a 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -879,6 +879,39 @@ static int ssl_parse_client_hello(mbedtls_ssl_context *ssl) */ if ((ret = mbedtls_ssl_read_record(ssl, 0)) != 0) { MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_ssl_read_record ", ret); + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + /* + * In the case of an alert message corresponding to the termination of + * a previous connection, `ssl_parse_record_header()` and then + * `mbedtls_ssl_read_record()` may return + * MBEDTLS_ERR_SSL_UNEXPECTED_RECORD because of a non zero epoch. + * + * Historically, the library has returned + * MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE in this situation. + * The sample program dtls_server.c relies on this behavior + * (see + * https://github.com/Mbed-TLS/mbedtls/blob/d5e35a376bee23fad0b17f2e3e94a32ce4017c64/programs/ssl/dtls_server.c#L295), + * and user applications may rely on it as well. + * + * For compatibility, map MBEDTLS_ERR_SSL_UNEXPECTED_RECORD + * to MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE here. + * + * MBEDTLS_ERR_SSL_UNEXPECTED_RECORD does not appear to be + * used to detect a specific error condition, so this mapping + * should not remove any meaningful distinction. + */ + if ((ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) +#if defined(MBEDTLS_SSL_RENEGOTIATION) + && (ssl->renego_status == MBEDTLS_SSL_INITIAL_HANDSHAKE) +#endif + ) { + if (ret == MBEDTLS_ERR_SSL_UNEXPECTED_RECORD) { + ret = MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE; + } + } +#endif /* MBEDTLS_SSL_PROTO_DTLS */ + return ret; } diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index f2e8eff47..79cbad877 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -4131,55 +4131,7 @@ close_notify: } while (ret == MBEDTLS_ERR_SSL_WANT_WRITE); ret = 0; - /* - * In the DTLS case, attempt to read a possible response to the close - * notification. This avoids reconnecting to the same client when we - * reset and later receive its close-notification response during - * step 3 (waiting for a client to connect). - * - * Stop waiting for the response if the connection has already ended. - * - * The waiting loop below relies on mbedtls_ssl_read() returning regularly - * in order to keep the total waiting time approximately bounded to 1s. If - * no read timeout is configured (see the read_timeout option), or if the - * configured timeout is close to or larger than 1s, the total waiting time - * may exceed 1s by a significant margin. - */ -#if defined(MBEDTLS_SSL_PROTO_DTLS) && defined(MBEDTLS_HAVE_TIME) - if (opt.transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM) { - mbedtls_ms_time_t start = mbedtls_ms_time(); - for (;;) { - ret = mbedtls_ssl_read(&ssl, buf, opt.buffer_size); - /* - * mbedtls_ssl_read() returned some data or timed out, loop if we - * have not spent already too much time, quite arbitrarily 1s. - */ - if ((ret > 0) || (ret == MBEDTLS_ERR_SSL_TIMEOUT)) { - if ((mbedtls_ms_time() - start) < 1000) { - continue; - } - } - - if (ret == MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY) { - mbedtls_printf(" done, received client close notification.\n"); - } else { - /* ret = 0, silent transport EOF or ret < 0 except - * MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY. Note that we do not - * handle specifically the non-fatal error codes like - * MBEDTLS_ERR_SSL_WANT_READ as we do not really expect them - * here. - */ - mbedtls_printf(" done\n"); - } - break; - } - ret = 0; - } else -#endif /* MBEDTLS_SSL_PROTO_DTLS && MBEDTLS_HAVE_TIME */ - { - mbedtls_printf(" done\n"); - } - fflush(stdout); + mbedtls_printf(" done\n"); #if defined(MBEDTLS_SSL_CACHE_C) if (opt.cache_remove > 0) { diff --git a/tests/compat.sh b/tests/compat.sh index 3f44c984f..2b6f45412 100755 --- a/tests/compat.sh +++ b/tests/compat.sh @@ -557,7 +557,6 @@ setup_arguments() # with OpenSSL 1.0.1h, -www, -WWW and -HTTP break DTLS handshakes if is_dtls "$MODE"; then O_SERVER_ARGS="$O_SERVER_ARGS" - M_SERVER_ARGS="$M_SERVER_ARGS read_timeout=1000" else O_SERVER_ARGS="$O_SERVER_ARGS -www" fi diff --git a/tests/scripts/analyze_outcomes.py b/tests/scripts/analyze_outcomes.py index 2bd4bd816..b6f18c5b8 100755 --- a/tests/scripts/analyze_outcomes.py +++ b/tests/scripts/analyze_outcomes.py @@ -50,6 +50,15 @@ class CoverageTask(outcome_analysis.CoverageTask): # TLS doesn't use restartable ECDH yet. # https://github.com/Mbed-TLS/mbedtls/issues/7294 re.compile(r'EC restart:.*no USE_PSA.*'), + # The following test fails intermittently on the CI with a frequency + # that significantly impacts CI throughput. They are thus disabled + # for the time being. See + # https://github.com/Mbed-TLS/mbedtls/issues/10652 for more + # information. + 'DTLS proxy: 3d, openssl client, fragmentation', + 'DTLS proxy: 3d, openssl client, fragmentation, nbio', + 'DTLS proxy: 3d, gnutls client, fragmentation', + 'DTLS proxy: 3d, gnutls client, fragmentation, nbio=2', ], 'test_suite_config.mbedtls_boolean': [ # Missing coverage of test configurations. diff --git a/tests/scripts/components-configuration-tls.sh b/tests/scripts/components-configuration-tls.sh index d017eef18..5a77c4def 100644 --- a/tests/scripts/components-configuration-tls.sh +++ b/tests/scripts/components-configuration-tls.sh @@ -165,7 +165,6 @@ component_test_tls1_2_ccm_psk_dtls () { msg "build: configs/config-ccm-psk-dtls1_2.h" MBEDTLS_CONFIG="configs/config-ccm-psk-dtls1_2.h" CRYPTO_CONFIG="configs/crypto-config-ccm-psk-tls1_2.h" - tf-psa-crypto/scripts/config.py -f "$CRYPTO_CONFIG" set MBEDTLS_HAVE_TIME CC=$ASAN_CC cmake -DMBEDTLS_CONFIG_FILE="$MBEDTLS_CONFIG" -DTF_PSA_CRYPTO_CONFIG_FILE="$CRYPTO_CONFIG" -D CMAKE_BUILD_TYPE:String=Asan . make diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index a999c94f5..4d0b6f608 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -9978,7 +9978,7 @@ run_test "DTLS reassembly: no fragmentation (gnutls client)" \ "$G_NEXT_CLI -u --mtu 2048 --insecure 127.0.0.1" \ 0 \ -S "found fragmented DTLS handshake message" \ - -S "error" + -s "HTTP/1.0 200 OK" requires_gnutls requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 @@ -9988,7 +9988,7 @@ run_test "DTLS reassembly: some fragmentation (gnutls client)" \ 0 \ -s "found fragmented DTLS handshake message" \ -s "Certificate handshake message has been buffered and reassembled" \ - -S "error" + -s "HTTP/1.0 200 OK" # Set the MTU to 128 bytes. The minimum size of a DTLS 1.2 record # containing a ClientHello handshake message is 69 bytes, without any cookie, @@ -10003,7 +10003,7 @@ run_test "DTLS reassembly: more fragmentation (gnutls client)" \ "$G_NEXT_CLI -u --mtu 103 --insecure 127.0.0.1" \ 0 \ -s "ClientHello handshake message has been buffered and reassembled" \ - -S "error" + -s "HTTP/1.0 200 OK" requires_gnutls requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 @@ -10012,7 +10012,7 @@ run_test "DTLS reassembly: more fragmentation, nbio (gnutls client)" \ "$G_NEXT_CLI -u --mtu 103 --insecure 127.0.0.1" \ 0 \ -s "ClientHello handshake message has been buffered and reassembled" \ - -S "error" + -s "HTTP/1.0 200 OK" # No fragmentation and renegotiation tests with GnuTLS client as the feature # does not work properly. @@ -10053,7 +10053,7 @@ run_test "DTLS reassembly: no fragmentation (openssl client)" \ "$O_NEXT_CLI -dtls -mtu 2048 -cert $DATA_FILES_PATH/server5.crt -key $DATA_FILES_PATH/server5.key" \ 0 \ -S "found fragmented DTLS handshake message" \ - -S "error" + -s "HTTP/1.0 200 OK" # Minimum possible MTU for OpenSSL server: 256 bytes. # We expect the client Certificate handshake message to be fragmented and @@ -10068,7 +10068,7 @@ run_test "DTLS reassembly: some fragmentation (openssl client)" \ 0 \ -s "found fragmented DTLS handshake message" \ -s "Certificate handshake message has been buffered and reassembled" \ - -S "error" + -s "HTTP/1.0 200 OK" requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 run_test "DTLS reassembly: fragmentation, nbio (openssl client)" \ @@ -10077,7 +10077,7 @@ run_test "DTLS reassembly: fragmentation, nbio (openssl client)" \ 0 \ -s "found fragmented DTLS handshake message" \ -s "Certificate handshake message has been buffered and reassembled" \ - -S "error" + -s "HTTP/1.0 200 OK" # Tests for sending fragmented handshake messages with DTLS # @@ -12169,6 +12169,10 @@ run_test "DTLS proxy: 3d, openssl client" \ 0 \ -s "HTTP/1.0 200 OK" +# The following test fails intermittently on the CI with a frequency that +# significantly impacts CI throughput. Disable it for the time being. +# See https://github.com/Mbed-TLS/mbedtls/issues/10652 for more information. +skip_next_test requires_openssl_next client_needs_more_time 8 not_with_valgrind # risk of non-mbedtls peer timing out @@ -12182,6 +12186,10 @@ run_test "DTLS proxy: 3d, openssl client, fragmentation" \ -s "found fragmented DTLS handshake message" \ -s "Certificate handshake message has been buffered and reassembled" +# The following test fails intermittently on the CI with a frequency that +# significantly impacts CI throughput. Disable it for the time being. +# See https://github.com/Mbed-TLS/mbedtls/issues/10652 for more information. +skip_next_test requires_openssl_next client_needs_more_time 8 not_with_valgrind # risk of non-mbedtls peer timing out @@ -12250,6 +12258,11 @@ run_test "DTLS proxy: 3d, gnutls client" \ # fragmentation to remain the case across GnuTLS version updates. Avoid using a # smaller MTU, as the smaller the MTU, the more likely the handshake is to fail # in this very unreliable connection emulation. + +# The following test fails intermittently on the CI with a frequency that +# significantly impacts CI throughput. Disable it for the time being. +# See https://github.com/Mbed-TLS/mbedtls/issues/10652 for more information. +skip_next_test requires_gnutls client_needs_more_time 8 not_with_valgrind # risk of non-mbedtls peer timing out @@ -12262,6 +12275,10 @@ run_test "DTLS proxy: 3d, gnutls client, fragmentation" \ -s "HTTP/1.0 200 OK" \ -s "ClientHello handshake message has been buffered and reassembled" +# The following test fails intermittently on the CI with a frequency that +# significantly impacts CI throughput. Disable it for the time being. +# See https://github.com/Mbed-TLS/mbedtls/issues/10652 for more information. +skip_next_test requires_gnutls client_needs_more_time 8 not_with_valgrind # risk of non-mbedtls peer timing out