From b847d8f2a988ffac1ac76e0bdc71eb02a5eae5f3 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Wed, 14 Feb 2018 19:30:48 +0200 Subject: [PATCH 1/7] Add ecc extensions only if ecc ciphersuite is used Fix compliancy to RFC4492. ECC extensions should be included only if ec ciphersuites are used. Interoperability issue with bouncy castle. #1157 --- library/ssl_cli.c | 20 ++++++++++++++++---- library/ssl_srv.c | 8 ++++++-- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index a57d866f3..4701b13e1 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -678,6 +678,10 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) unsigned char offer_compress; const int *ciphersuites; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; +#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ + defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) + int uses_ec = 0; +#endif MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> write client hello" ) ); @@ -829,6 +833,11 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello, add ciphersuite: %2d", ciphersuites[i] ) ); +#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ + defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) + uses_ec |= mbedtls_ssl_ciphersuite_uses_ec( ciphersuite_info ); +#endif + n++; *p++ = (unsigned char)( ciphersuites[i] >> 8 ); *p++ = (unsigned char)( ciphersuites[i] ); @@ -919,11 +928,14 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) #endif #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) - ssl_write_supported_elliptic_curves_ext( ssl, p + 2 + ext_len, &olen ); - ext_len += olen; + if( uses_ec ) + { + ssl_write_supported_elliptic_curves_ext( ssl, p + 2 + ext_len, &olen ); + ext_len += olen; - ssl_write_supported_point_formats_ext( ssl, p + 2 + ext_len, &olen ); - ext_len += olen; + ssl_write_supported_point_formats_ext( ssl, p + 2 + ext_len, &olen ); + ext_len += olen; + } #endif #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 9a884f055..984b205db 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -2423,8 +2423,12 @@ static int ssl_write_server_hello( mbedtls_ssl_context *ssl ) #endif #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) - ssl_write_supported_point_formats_ext( ssl, p + 2 + ext_len, &olen ); - ext_len += olen; + if ( mbedtls_ssl_ciphersuite_uses_ec( + mbedtls_ssl_ciphersuite_from_id( ssl->session_negotiate->ciphersuite ) ) ) + { + ssl_write_supported_point_formats_ext( ssl, p + 2 + ext_len, &olen ); + ext_len += olen; + } #endif #if defined(MBEDTLS_SSL_ALPN) From 5c141d28ca47814da96d76576e9b25effdfcd4b9 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Thu, 21 Jun 2018 16:40:24 +0300 Subject: [PATCH 2/7] Add entry in ChangeLog Add an entry in the ChangeLog, describing the fix. --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0d5ae5a09..03b3cabe0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Bugfix Philippe Antoine. * Clarify documentation for mbedtls_ssl_write() to include 0 as a valid return value. Found by @davidwu2000. #839 + * Add ecc extensions only if an ecc based ciphersuite is used. + Affects interoperability with BouncyCastle and other peers. + Raised by milenamil in #1157. Changes * Change the shebang line in Perl scripts to look up perl in the PATH. From f27f8aeb19836e0e9c5b19366f4a0f6838f63c86 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Thu, 28 Jun 2018 11:09:09 +0300 Subject: [PATCH 3/7] Update ChangeLog Update ChangeLog with a less ambigous description. --- ChangeLog | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 03b3cabe0..34f08a889 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,8 +8,8 @@ Bugfix * Clarify documentation for mbedtls_ssl_write() to include 0 as a valid return value. Found by @davidwu2000. #839 * Add ecc extensions only if an ecc based ciphersuite is used. - Affects interoperability with BouncyCastle and other peers. - Raised by milenamil in #1157. + This improves compliance to RFC 4492, and as a result, solves + interoperability issues with BouncyCastle. Raised by milenamil in #1157. Changes * Change the shebang line in Perl scripts to look up perl in the PATH. From b27a1ab18f1fd5e954e21b21b6c2241329e137e0 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Thu, 28 Jun 2018 13:22:05 +0300 Subject: [PATCH 4/7] Add ECC extensions test in ssl-opts.sh Add test to verify if an ecc based extension exists or not if an ecc based ciphersuite is used or not. --- tests/ssl-opt.sh | 117 ++++++++++------------------------------------- 1 file changed, 25 insertions(+), 92 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index a8adf9bb3..bedbde1e0 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -3739,106 +3739,39 @@ run_test "Large packet TLS 1.2 AEAD shorter tag" \ -c "16384 bytes written in 1 fragments" \ -s "Read from client: 16384 bytes read" -# Tests for DTLS HelloVerifyRequest +# Tests for ECC extensions (rfc 4492) -run_test "DTLS cookie: enabled" \ - "$P_SRV dtls=1 debug_level=2" \ - "$P_CLI dtls=1 debug_level=2" \ +run_test "Force a non ECC ciphersuite in the client side" \ + "$P_SRV debug_level=3" \ + "$P_CLI debug_level=3 force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \ 0 \ - -s "cookie verification failed" \ - -s "cookie verification passed" \ - -S "cookie verification skipped" \ - -c "received hello verify request" \ - -s "hello verification requested" \ - -S "SSL - The requested feature is not available" + -C "client hello, adding supported_elliptic_curves extension" \ + -C "client hello, adding supported_point_formats extension" \ + -S "found supported elliptic curves extension" \ + -S "found supported point formats extension" -run_test "DTLS cookie: disabled" \ - "$P_SRV dtls=1 debug_level=2 cookies=0" \ - "$P_CLI dtls=1 debug_level=2" \ +run_test "Force a non ECC ciphersuite in the server side" \ + "$P_SRV debug_level=3 force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \ + "$P_CLI debug_level=3" \ 0 \ - -S "cookie verification failed" \ - -S "cookie verification passed" \ - -s "cookie verification skipped" \ - -C "received hello verify request" \ - -S "hello verification requested" \ - -S "SSL - The requested feature is not available" + -C "found supported_point_formats extension" \ + -S "server hello, supported_point_formats extension" -run_test "DTLS cookie: default (failing)" \ - "$P_SRV dtls=1 debug_level=2 cookies=-1" \ - "$P_CLI dtls=1 debug_level=2 hs_timeout=100-400" \ - 1 \ - -s "cookie verification failed" \ - -S "cookie verification passed" \ - -S "cookie verification skipped" \ - -C "received hello verify request" \ - -S "hello verification requested" \ - -s "SSL - The requested feature is not available" - -requires_ipv6 -run_test "DTLS cookie: enabled, IPv6" \ - "$P_SRV dtls=1 debug_level=2 server_addr=::1" \ - "$P_CLI dtls=1 debug_level=2 server_addr=::1" \ +run_test "Force an ECC ciphersuite in the client side" \ + "$P_SRV debug_level=3" \ + "$P_CLI debug_level=3 force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-CBC-SHA256" \ 0 \ - -s "cookie verification failed" \ - -s "cookie verification passed" \ - -S "cookie verification skipped" \ - -c "received hello verify request" \ - -s "hello verification requested" \ - -S "SSL - The requested feature is not available" + -c "client hello, adding supported_elliptic_curves extension" \ + -c "client hello, adding supported_point_formats extension" \ + -s "found supported elliptic curves extension" \ + -s "found supported point formats extension" -run_test "DTLS cookie: enabled, nbio" \ - "$P_SRV dtls=1 nbio=2 debug_level=2" \ - "$P_CLI dtls=1 nbio=2 debug_level=2" \ +run_test "Force an ECC ciphersuite in the server side" \ + "$P_SRV debug_level=3 force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-CBC-SHA256" \ + "$P_CLI debug_level=3" \ 0 \ - -s "cookie verification failed" \ - -s "cookie verification passed" \ - -S "cookie verification skipped" \ - -c "received hello verify request" \ - -s "hello verification requested" \ - -S "SSL - The requested feature is not available" - -# Tests for client reconnecting from the same port with DTLS - -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" \ - 0 \ - -C "resend" \ - -S "The operation timed out" \ - -S "Client initiated reconnection from same port" - -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" \ - 0 \ - -C "resend" \ - -S "The operation timed out" \ - -s "Client initiated reconnection from same port" - -not_with_valgrind # server/client too slow to respond in time (next test has higher timeouts) -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" \ - 0 \ - -S "The operation timed out" \ - -s "Client initiated reconnection from same port" - -only_with_valgrind # Only with valgrind, do previous test but with higher read_timeout and hs_timeout -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" \ - 0 \ - -S "The operation timed out" \ - -s "Client initiated reconnection from same port" - -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" \ - 0 \ - -s "The operation timed out" \ - -S "Client initiated reconnection from same port" + -c "found supported_point_formats extension" \ + -s "server hello, supported_point_formats extension" # Tests for various cases of client authentication with DTLS # (focused on handshake flows and message parsing) From 2eee2e63e51fb2360997f25222e87b5a606462d6 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Thu, 28 Jun 2018 16:17:00 +0300 Subject: [PATCH 5/7] Update ssl-opt.sh test to run condition 1. Update the test script to un the ECC tests only if the relevant configurations are defined in `config.h` file 2. Change the HASH of the ciphersuite from SHA1 based to SHA256 for better example --- tests/ssl-opt.sh | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index bedbde1e0..b7ad0a70b 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -3741,22 +3741,34 @@ run_test "Large packet TLS 1.2 AEAD shorter tag" \ # Tests for ECC extensions (rfc 4492) +requires_config_enabled MBEDTLS_AES_C +requires_config_enabled MBEDTLS_CIPHER_MODE_CBC +requires_config_enabled MBEDTLS_SHA256_C +requires_config_enabled MBEDTLS_KEY_EXCHANGE_RSA_ENABLED run_test "Force a non ECC ciphersuite in the client side" \ "$P_SRV debug_level=3" \ - "$P_CLI debug_level=3 force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \ + "$P_CLI debug_level=3 force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA256" \ 0 \ -C "client hello, adding supported_elliptic_curves extension" \ -C "client hello, adding supported_point_formats extension" \ -S "found supported elliptic curves extension" \ -S "found supported point formats extension" +requires_config_enabled MBEDTLS_AES_C +requires_config_enabled MBEDTLS_CIPHER_MODE_CBC +requires_config_enabled MBEDTLS_SHA256_C +requires_config_enabled MBEDTLS_KEY_EXCHANGE_RSA_ENABLED run_test "Force a non ECC ciphersuite in the server side" \ - "$P_SRV debug_level=3 force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \ + "$P_SRV debug_level=3 force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA256" \ "$P_CLI debug_level=3" \ 0 \ -C "found supported_point_formats extension" \ -S "server hello, supported_point_formats extension" +requires_config_enabled MBEDTLS_AES_C +requires_config_enabled MBEDTLS_CIPHER_MODE_CBC +requires_config_enabled MBEDTLS_SHA256_C +requires_config_enabled MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED run_test "Force an ECC ciphersuite in the client side" \ "$P_SRV debug_level=3" \ "$P_CLI debug_level=3 force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-CBC-SHA256" \ @@ -3766,6 +3778,10 @@ run_test "Force an ECC ciphersuite in the client side" \ -s "found supported elliptic curves extension" \ -s "found supported point formats extension" +requires_config_enabled MBEDTLS_AES_C +requires_config_enabled MBEDTLS_CIPHER_MODE_CBC +requires_config_enabled MBEDTLS_SHA256_C +requires_config_enabled MBEDTLS_KEY_EXCHANGE_ECDHE_ECDSA_ENABLED run_test "Force an ECC ciphersuite in the server side" \ "$P_SRV debug_level=3 force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-CBC-SHA256" \ "$P_CLI debug_level=3" \ From 6877685ac6274f2d602419ae9ff83db391287b85 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Sun, 1 Jul 2018 10:05:49 +0300 Subject: [PATCH 6/7] Restore accidentally deleted lines Restore lines that were accidentally deleted by a previous moerge conflict. --- tests/ssl-opt.sh | 101 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index b7ad0a70b..3a8ec3c70 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -3789,6 +3789,107 @@ run_test "Force an ECC ciphersuite in the server side" \ -c "found supported_point_formats extension" \ -s "server hello, supported_point_formats extension" +# Tests for DTLS HelloVerifyRequest + +run_test "DTLS cookie: enabled" \ + "$P_SRV dtls=1 debug_level=2" \ + "$P_CLI dtls=1 debug_level=2" \ + 0 \ + -s "cookie verification failed" \ + -s "cookie verification passed" \ + -S "cookie verification skipped" \ + -c "received hello verify request" \ + -s "hello verification requested" \ + -S "SSL - The requested feature is not available" + +run_test "DTLS cookie: disabled" \ + "$P_SRV dtls=1 debug_level=2 cookies=0" \ + "$P_CLI dtls=1 debug_level=2" \ + 0 \ + -S "cookie verification failed" \ + -S "cookie verification passed" \ + -s "cookie verification skipped" \ + -C "received hello verify request" \ + -S "hello verification requested" \ + -S "SSL - The requested feature is not available" + +run_test "DTLS cookie: default (failing)" \ + "$P_SRV dtls=1 debug_level=2 cookies=-1" \ + "$P_CLI dtls=1 debug_level=2 hs_timeout=100-400" \ + 1 \ + -s "cookie verification failed" \ + -S "cookie verification passed" \ + -S "cookie verification skipped" \ + -C "received hello verify request" \ + -S "hello verification requested" \ + -s "SSL - The requested feature is not available" + +requires_ipv6 +run_test "DTLS cookie: enabled, IPv6" \ + "$P_SRV dtls=1 debug_level=2 server_addr=::1" \ + "$P_CLI dtls=1 debug_level=2 server_addr=::1" \ + 0 \ + -s "cookie verification failed" \ + -s "cookie verification passed" \ + -S "cookie verification skipped" \ + -c "received hello verify request" \ + -s "hello verification requested" \ + -S "SSL - The requested feature is not available" + +run_test "DTLS cookie: enabled, nbio" \ + "$P_SRV dtls=1 nbio=2 debug_level=2" \ + "$P_CLI dtls=1 nbio=2 debug_level=2" \ + 0 \ + -s "cookie verification failed" \ + -s "cookie verification passed" \ + -S "cookie verification skipped" \ + -c "received hello verify request" \ + -s "hello verification requested" \ + -S "SSL - The requested feature is not available" + +# Tests for client reconnecting from the same port with DTLS + +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" \ + 0 \ + -C "resend" \ + -S "The operation timed out" \ + -S "Client initiated reconnection from same port" + +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" \ + 0 \ + -C "resend" \ + -S "The operation timed out" \ + -s "Client initiated reconnection from same port" + +not_with_valgrind # server/client too slow to respond in time (next test has higher timeouts) +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" \ + 0 \ + -S "The operation timed out" \ + -s "Client initiated reconnection from same port" + +only_with_valgrind # Only with valgrind, do previous test but with higher read_timeout and hs_timeout +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" \ + 0 \ + -S "The operation timed out" \ + -s "Client initiated reconnection from same port" + +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" \ + 0 \ + -s "The operation timed out" \ + -S "Client initiated reconnection from same port" + # Tests for various cases of client authentication with DTLS # (focused on handshake flows and message parsing) From 2e7b686f71dd25a8fcac58c648c1d2e6ae57113c Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Wed, 11 Jul 2018 13:37:38 +0300 Subject: [PATCH 7/7] Remove reference to ECJPAKE Remove reference to `MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED` as branch `mbedtls-2.1` doesn't have `ECJPAKE`. This definition was accidently inserted in a backport. --- library/ssl_cli.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 4701b13e1..14b7a481d 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -678,8 +678,7 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) unsigned char offer_compress; const int *ciphersuites; const mbedtls_ssl_ciphersuite_t *ciphersuite_info; -#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ - defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) +#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) int uses_ec = 0; #endif @@ -833,8 +832,7 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello, add ciphersuite: %2d", ciphersuites[i] ) ); -#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) || \ - defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) +#if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) uses_ec |= mbedtls_ssl_ciphersuite_uses_ec( ciphersuite_info ); #endif