From 615129839558690d2bca8fbdcc1ca885ee8d208e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 21 Aug 2018 09:40:07 +0200 Subject: [PATCH 1/8] Add missing requires_gnutls guards --- tests/ssl-opt.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index f1c19828b..4a6234803 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5332,6 +5332,7 @@ requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_gnutls run_test "DTLS fragmenting: gnutls server, DTLS 1.2" \ "$G_SRV -u" \ "$P_CLI dtls=1 debug_level=2 \ @@ -5346,6 +5347,7 @@ requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_1 +requires_gnutls run_test "DTLS fragmenting: gnutls server, DTLS 1.0" \ "$G_SRV -u" \ "$P_CLI dtls=1 debug_level=2 \ @@ -5362,6 +5364,7 @@ requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_2 +requires_gnutls run_test "DTLS fragmenting: gnutls client, DTLS 1.2" \ "$P_SRV dtls=1 debug_level=2 server_addr=::1 \ crt_file=data_files/server7_int-ca.crt \ @@ -5377,6 +5380,7 @@ requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C requires_config_enabled MBEDTLS_ECDSA_C requires_config_enabled MBEDTLS_SSL_PROTO_TLS1_1 +requires_gnutls run_test "DTLS fragmenting: gnutls client, DTLS 1.0" \ "$P_SRV dtls=1 debug_level=2 server_addr=::1 \ crt_file=data_files/server7_int-ca.crt \ @@ -5486,6 +5490,7 @@ run_test "DTLS fragmenting: 3d, gnutls server, DTLS 1.0" \ ## ## # gnutls-cli always tries IPv6 first, and doesn't fall back to IPv4 with DTLS ## requires_ipv6 +## requires_gnutls ## requires_config_enabled MBEDTLS_SSL_PROTO_DTLS ## requires_config_enabled MBEDTLS_RSA_C ## requires_config_enabled MBEDTLS_ECDSA_C @@ -5503,6 +5508,7 @@ run_test "DTLS fragmenting: 3d, gnutls server, DTLS 1.0" \ ## ## # gnutls-cli always tries IPv6 first, and doesn't fall back to IPv4 with DTLS ## requires_ipv6 +## requires_gnutls ## requires_config_enabled MBEDTLS_SSL_PROTO_DTLS ## requires_config_enabled MBEDTLS_RSA_C ## requires_config_enabled MBEDTLS_ECDSA_C From f2f1d40d6d96fd5f7c0973d91b5620d30a6e0913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 21 Aug 2018 09:53:22 +0200 Subject: [PATCH 2/8] Improve wording in ChangeLog and documentation --- ChangeLog | 2 +- include/mbedtls/ssl.h | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index a95cc6c59..3f144a7e9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,7 +5,7 @@ mbed TLS ChangeLog (Sorted per branch, date) Features * Add support for fragmentation of outgoing DTLS handshake messages. This is controlled by the maximum fragment length as set locally or negotiated - with the peer, as well as new per-connection MTU option, set using + with the peer, as well as by a new per-connection MTU option, set using mbedtls_ssl_set_mtu(). Bugfix diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index f563437d1..4471de507 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1392,9 +1392,11 @@ void mbedtls_ssl_set_bio( mbedtls_ssl_context *ssl, * example when a PMTU estimate becomes available from other * sources, such as lower (or higher) protocol layers. * - * \note This only controls the size of the packets we send. - * Client-side, you can request the server to use smaller - * records with \c mbedtls_ssl_conf_max_frag_len(). + * \note This setting only controls the size of the packets we send, + * and does not restrict the size of the datagrams we're + * willing to receive. Client-side, you can request the + * server to use smaller records with \c + * mbedtls_ssl_conf_max_frag_len(). * * \note If both a MTU and a maximum fragment length have been * configured (or negotiated with the peer), the resulting @@ -1402,7 +1404,8 @@ void mbedtls_ssl_set_bio( mbedtls_ssl_context *ssl, * on the record content length) is used. * * \note This can only be used to decrease the maximum size - * of datagrams sent. It cannot be used to increase the + * of datagrams (hence records, as records cannot span + * multiple datagrams) sent. It cannot be used to increase the * maximum size of records over the limit set by * #MBEDTLS_SSL_OUT_CONTENT_LEN. * From 000281e07d796576d615243b5883b243f22dc53f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 21 Aug 2018 11:20:58 +0200 Subject: [PATCH 3/8] Fix "unused parameter" warning in small configs --- library/ssl_tls.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index da21db237..faa9467e1 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7092,6 +7092,11 @@ int mbedtls_ssl_get_max_out_record_payload( const mbedtls_ssl_context *ssl ) { size_t max_len = MBEDTLS_SSL_OUT_CONTENT_LEN; +#if !defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) && \ + !defined(MBEDTLS_SSL_PROTO_DTLS) + (void) ssl; +#endif + #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) const size_t mfl = mbedtls_ssl_get_max_frag_len( ssl ); From 661103595e90529a2a3fc0af3648331f02b1af30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 21 Aug 2018 11:55:40 +0200 Subject: [PATCH 4/8] Try to further clarify documentation --- include/mbedtls/ssl.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 4471de507..35f4d320a 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1388,6 +1388,10 @@ void mbedtls_ssl_set_bio( mbedtls_ssl_context *ssl, * the maximum size datagram the DTLS layer will pass to the * \c f_send() callback set using \c mbedtls_ssl_set_bio(). * + * \note The limit on datagram size is converted to a limit on + * record payload by subtracting the current overhead of + * encapsulation and encryption/authentication if any. + * * \note This can be called at any point during the connection, for * example when a PMTU estimate becomes available from other * sources, such as lower (or higher) protocol layers. @@ -1400,14 +1404,12 @@ void mbedtls_ssl_set_bio( mbedtls_ssl_context *ssl, * * \note If both a MTU and a maximum fragment length have been * configured (or negotiated with the peer), the resulting - * lower limit (after translating the MTU setting to a limit - * on the record content length) is used. + * lower limit on record payload (see first note) is used. * * \note This can only be used to decrease the maximum size - * of datagrams (hence records, as records cannot span - * multiple datagrams) sent. It cannot be used to increase the - * maximum size of records over the limit set by - * #MBEDTLS_SSL_OUT_CONTENT_LEN. + * of datagrams (hence records, see first note) sent. It + * cannot be used to increase the maximum size of records over + * the limit set by #MBEDTLS_SSL_OUT_CONTENT_LEN. * * \note Values lower than the current record layer expansion will * result in an error when trying to send data. From 2f2d9020cd4eaab26b4159fd87e1220211e35a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 21 Aug 2018 12:17:54 +0200 Subject: [PATCH 5/8] Add delay in test to avoid race condition We previously observed random-looking failures from this test. I think they were caused by a race condition where the client tries to reconnect while the server is still closing the connection and has not yet returned to an accepting state. In that case, the server would fail to see and reply to the ClientHello, and the client would have to resend it. I believe logs of failing runs are compatible with this interpretation: - the proxy logs show the new ClientHello and the server's closing Alert are sent the same millisecond. - the client logs show the server's closing Alert is received after the new handshake has been started (discarding message from wrong epoch). The attempted fix is for the client to wait a bit before reconnecting, which should vastly enhance the probability of the server reaching its accepting state before the client tries to reconnect. The value of 1 second is arbitrary but should be more than enough even on loaded machines. The test was run locally 100 times in a row on a slightly loaded machine (an instance of all.sh running in parallel) without any failure after this fix. --- tests/ssl-opt.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 4a6234803..f811789e6 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5139,6 +5139,8 @@ run_test "DTLS fragmenting: proxy MTU, simple handshake" \ # Since we don't support reading fragmented ClientHello yet, # up the MTU to 1450 (larger than ClientHello with session ticket, # but still smaller than client's Certificate to ensure fragmentation). +# reco_delay avoids races where the client reconnects before the server has +# resumed listening, which would result in a spurious resend. not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5152,7 +5154,7 @@ run_test "DTLS fragmenting: proxy MTU, resumed handshake" \ "$P_CLI dtls=1 debug_level=2 \ crt_file=data_files/server8_int-ca2.crt \ key_file=data_files/server8.key \ - mtu=1450 reconnect=1" \ + mtu=1450 reconnect=1 reco_delay=1" \ 0 \ -S "resend" \ -C "resend" \ From 3d183cefb5bbc3e37fa033c2c85fdcde127a296c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Aug 2018 09:56:22 +0200 Subject: [PATCH 6/8] Allow client-side resend in proxy MTU tests From Hanno: When a server replies to a cookieless ClientHello with a HelloVerifyRequest, it is supposed to reset the connection and wait for a subsequent ClientHello which includes the cookie from the HelloVerifyRequest. In testing environments, it might happen that the reset of the server takes longer than for the client to replying to the HelloVerifyRequest with the ClientHello+Cookie. In this case, the ClientHello gets lost and the client will need retransmit. This may happen even if the underlying datagram transport is reliable. --- tests/ssl-opt.sh | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index f811789e6..8cf0c82a6 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5112,6 +5112,8 @@ run_test "DTLS fragmenting: both (MTU)" \ -C "error" # the proxy shouldn't drop or mess up anything, so we shouldn't need to resend +# OTOH the client might resend if the server is to slow to reset after sending +# a HelloVerifyRequest, so only check for no retransmission server-side not_with_valgrind # spurious resend due to timeout requires_config_enabled MBEDTLS_SSL_PROTO_DTLS requires_config_enabled MBEDTLS_RSA_C @@ -5128,7 +5130,6 @@ run_test "DTLS fragmenting: proxy MTU, simple handshake" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" @@ -5157,7 +5158,6 @@ run_test "DTLS fragmenting: proxy MTU, resumed handshake" \ mtu=1450 reconnect=1 reco_delay=1" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" @@ -5185,7 +5185,6 @@ run_test "DTLS fragmenting: proxy MTU, ChachaPoly renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" @@ -5214,7 +5213,6 @@ run_test "DTLS fragmenting: proxy MTU, AES-GCM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" @@ -5243,7 +5241,6 @@ run_test "DTLS fragmenting: proxy MTU, AES-CCM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" @@ -5273,7 +5270,6 @@ run_test "DTLS fragmenting: proxy MTU, AES-CBC EtM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" @@ -5302,7 +5298,6 @@ run_test "DTLS fragmenting: proxy MTU, AES-CBC non-EtM renego" \ mtu=512" \ 0 \ -S "resend" \ - -C "resend" \ -s "found fragmented DTLS handshake message" \ -c "found fragmented DTLS handshake message" \ -C "error" From c1d54b74ec756186e373a266e4cfc453225b0708 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Aug 2018 10:02:59 +0200 Subject: [PATCH 7/8] Add tests with non-blocking I/O Make sure we behave properly when f_send() or f_recv() return MBEDTLS_ERR_SSL_WANT_{WRITE,READ}. --- tests/ssl-opt.sh | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 8cf0c82a6..ec2717ad5 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5134,6 +5134,26 @@ run_test "DTLS fragmenting: proxy MTU, simple handshake" \ -c "found fragmented DTLS handshake message" \ -C "error" +not_with_valgrind # spurious resend due to timeout +requires_config_enabled MBEDTLS_SSL_PROTO_DTLS +requires_config_enabled MBEDTLS_RSA_C +requires_config_enabled MBEDTLS_ECDSA_C +run_test "DTLS fragmenting: proxy MTU, simple handshake, nbio" \ + -p "$P_PXY mtu=512" \ + "$P_SRV dtls=1 debug_level=2 auth_mode=required \ + crt_file=data_files/server7_int-ca.crt \ + key_file=data_files/server7.key \ + mtu=512 nbio=2" \ + "$P_CLI dtls=1 debug_level=2 \ + crt_file=data_files/server8_int-ca2.crt \ + key_file=data_files/server8.key \ + mtu=512 nbio=2" \ + 0 \ + -S "resend" \ + -s "found fragmented DTLS handshake message" \ + -c "found fragmented DTLS handshake message" \ + -C "error" + # This ensures things still work after session_reset(), # for example it would have caught #1941. # It also exercises the "resumed hanshake" flow. @@ -5321,6 +5341,25 @@ run_test "DTLS fragmenting: proxy MTU + 3d" \ -c "found fragmented DTLS handshake message" \ -C "error" +requires_config_enabled MBEDTLS_SSL_PROTO_DTLS +requires_config_enabled MBEDTLS_RSA_C +requires_config_enabled MBEDTLS_ECDSA_C +client_needs_more_time 2 +run_test "DTLS fragmenting: proxy MTU + 3d, nbio" \ + -p "$P_PXY mtu=512 drop=8 delay=8 duplicate=8" \ + "$P_SRV dtls=1 debug_level=2 auth_mode=required \ + crt_file=data_files/server7_int-ca.crt \ + key_file=data_files/server7.key \ + hs_timeout=250-10000 mtu=512 nbio=2" \ + "$P_CLI dtls=1 debug_level=2 \ + crt_file=data_files/server8_int-ca2.crt \ + key_file=data_files/server8.key \ + hs_timeout=250-10000 mtu=512 nbio=2" \ + 0 \ + -s "found fragmented DTLS handshake message" \ + -c "found fragmented DTLS handshake message" \ + -C "error" + # interop tests for DTLS fragmentating with reliable connection # # here and below we just want to test that the we fragment in a way that From 68ae351dbec53e8e6b5eae3ff1392952055f1a2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 22 Aug 2018 10:24:31 +0200 Subject: [PATCH 8/8] Fix some whitespace in documentation --- include/mbedtls/ssl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 35f4d320a..090660733 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -1398,7 +1398,7 @@ void mbedtls_ssl_set_bio( mbedtls_ssl_context *ssl, * * \note This setting only controls the size of the packets we send, * and does not restrict the size of the datagrams we're - * willing to receive. Client-side, you can request the + * willing to receive. Client-side, you can request the * server to use smaller records with \c * mbedtls_ssl_conf_max_frag_len(). *