From 6eb066ed56ab0b6b34e7fd9b1903160423770599 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Thu, 19 May 2016 22:12:18 +0100 Subject: [PATCH 1/5] Fixes RC4 config dependencies in tests in ssl-opt.h Adds dependencies on MBEDTLS_REMOVE_ARC4_CIPHERSUITES for tests that require RC4 to be disabled (the default config). --- tests/ssl-opt.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 8f49e9661..103809bd1 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -567,12 +567,14 @@ run_test "Default, DTLS" \ # Tests for rc4 option +requires_config_enabled MBEDTLS_REMOVE_ARC4_CIPHERSUITES run_test "RC4: server disabled, client enabled" \ "$P_SRV" \ "$P_CLI force_ciphersuite=TLS-RSA-WITH-RC4-128-SHA" \ 1 \ -s "SSL - The server has no ciphersuites in common" +requires_config_enabled MBEDTLS_REMOVE_ARC4_CIPHERSUITES run_test "RC4: server half, client enabled" \ "$P_SRV arc4=1" \ "$P_CLI force_ciphersuite=TLS-RSA-WITH-RC4-128-SHA" \ From 3b224ffd25d6d27d2cce8f00039bca4396eaa23e Mon Sep 17 00:00:00 2001 From: Paul Bakker Date: Fri, 13 May 2016 10:33:25 +0100 Subject: [PATCH 2/5] Split test into valgrind and no-valgrind version Running valgrind on: "DTLS client reconnect from same port: reconnect, nbio" results in timeouts. New version added that runs only under valgrind. Original only runs when valgrind is not used --- tests/ssl-opt.sh | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 103809bd1..0a2ff2518 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -130,6 +130,13 @@ not_with_valgrind() { fi } +# skip the next test if valgrind is NOT in use +only_with_valgrind() { + if [ "$MEMCHECK" -eq 0 ]; then + SKIP_NEXT="YES" + fi +} + # multiply the client timeout delay by the given factor for the next test needs_more_time() { CLI_DELAY_FACTOR=$1 @@ -2958,13 +2965,22 @@ run_test "DTLS client reconnect from same port: reconnect" \ -S "The operation timed out" \ -s "Client initiated reconnection from same port" -run_test "DTLS client reconnect from same port: reconnect, nbio" \ +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" \ From b700c46750b07dff545a9acddaf416a2c454b8a3 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 6 May 2016 13:48:23 +0100 Subject: [PATCH 3/5] Add a test for SSLv3 with extensions, server side This test verifies if the server parses or sends extensions when the protocol is SSLv3. --- tests/ssl-opt.sh | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 0a2ff2518..d184d8565 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -724,7 +724,7 @@ run_test "Encrypt then MAC: client enabled, server SSLv3" \ "$P_CLI debug_level=3 min_version=ssl3" \ 0 \ -c "client hello, adding encrypt_then_mac extension" \ - -s "found encrypt then mac extension" \ + -S "found encrypt then mac extension" \ -S "server hello, adding encrypt then mac extension" \ -C "found encrypt_then_mac extension" \ -C "using encrypt then mac" \ @@ -783,7 +783,7 @@ run_test "Extended Master Secret: client enabled, server SSLv3" \ "$P_CLI debug_level=3 min_version=ssl3" \ 0 \ -c "client hello, adding extended_master_secret extension" \ - -s "found extended master secret extension" \ + -S "found extended master secret extension" \ -S "server hello, adding extended master secret extension" \ -C "found extended_master_secret extension" \ -C "using extended master secret" \ @@ -2765,6 +2765,16 @@ run_test "Small packet TLS 1.2 AEAD shorter tag" \ 0 \ -s "Read from client: 1 bytes read" +# A test for extensions in SSLv3 + +requires_config_enabled MBEDTLS_SSL_PROTO_SSL3 +run_test "SSLv3 with extensions, server side" \ + "$P_SRV min_version=ssl3 debug_level=3" \ + "$P_CLI force_version=ssl3 tickets=1 max_frag_len=4096 alpn=abc,1234" \ + 0 \ + -S "dumping 'client hello extensions'" \ + -S "server hello, total extension length:" + # Test for large packets requires_config_enabled MBEDTLS_SSL_PROTO_SSL3 From 83f26052bf0068baff08d90204ffff57d20f85e7 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 23 May 2016 14:27:02 +0100 Subject: [PATCH 4/5] Fix non compliance SSLv3 in server extension handling. The server code parses the client hello extensions even when the protocol is SSLv3 and this behaviour is non compliant with rfc6101. Also the server sends extensions in the server hello and omitting them may prevent interoperability problems. --- ChangeLog | 2 ++ library/ssl_srv.c | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/ChangeLog b/ChangeLog index 01563a422..cd0d67f6f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -20,6 +20,8 @@ Changes * On ARM platforms, when compiling with -O0 with GCC, Clang or armcc5, don't use the optimized assembly for bignum multiplication. This removes the need to pass -fomit-frame-pointer to avoid a build error with -O0. + * Fix non-compliance server extension handling. Extensions for SSLv3 are now + ignored, as required by RFC6101. = mbed TLS 2.1.4 released 2016-01-05 diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 938ca7a2f..f61c38be7 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -1460,6 +1460,12 @@ read_record_header: ssl->session_negotiate->compression = MBEDTLS_SSL_COMPRESS_NULL; #endif + /* Do not parse the extensions if the protocol is SSLv3 */ +#if defined(MBEDTLS_SSL_PROTO_SSL3) + if( ( ssl->major_ver != 3 ) || ( ssl->minor_ver != 0 ) ) + { +#endif + /* * Check the extension length */ @@ -1633,8 +1639,13 @@ read_record_header: MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } + } +#if defined(MBEDTLS_SSL_PROTO_SSL3) + } +#endif + #if defined(MBEDTLS_SSL_FALLBACK_SCSV) for( i = 0, p = buf + 41 + sess_len; i < ciph_len; i += 2, p += 2 ) { @@ -2259,6 +2270,12 @@ static int ssl_write_server_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, compress alg.: 0x%02X", ssl->session_negotiate->compression ) ); + /* Do not write the extensions if the protocol is SSLv3 */ +#if defined(MBEDTLS_SSL_PROTO_SSL3) + if( ( ssl->major_ver != 3 ) || ( ssl->minor_ver != 0 ) ) + { +#endif + /* * First write extensions, then the total length */ @@ -2309,6 +2326,10 @@ static int ssl_write_server_hello( mbedtls_ssl_context *ssl ) p += ext_len; } +#if defined(MBEDTLS_SSL_PROTO_SSL3) + } +#endif + ssl->out_msglen = p - buf; ssl->out_msgtype = MBEDTLS_SSL_MSG_HANDSHAKE; ssl->out_msg[0] = MBEDTLS_SSL_HS_SERVER_HELLO; From 60e2b2fe22818ca90206ccbd23b8da492c57cc64 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Mon, 23 May 2016 16:24:52 +0100 Subject: [PATCH 5/5] Fix whitespace and formatting in ssl_srv.c --- library/ssl_srv.c | 246 +++++++++++++++++++++++----------------------- 1 file changed, 121 insertions(+), 125 deletions(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index f61c38be7..59608eaca 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -1465,183 +1465,179 @@ read_record_header: if( ( ssl->major_ver != 3 ) || ( ssl->minor_ver != 0 ) ) { #endif - - /* - * Check the extension length - */ - ext_offset = comp_offset + 1 + comp_len; - if( msg_len > ext_offset ) - { - if( msg_len < ext_offset + 2 ) + /* + * Check the extension length + */ + ext_offset = comp_offset + 1 + comp_len; + if( msg_len > ext_offset ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + if( msg_len < ext_offset + 2 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } + + ext_len = ( buf[ext_offset + 0] << 8 ) + | ( buf[ext_offset + 1] ); + + if( ( ext_len > 0 && ext_len < 4 ) || + msg_len != ext_offset + 2 + ext_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } } + else + ext_len = 0; - ext_len = ( buf[ext_offset + 0] << 8 ) - | ( buf[ext_offset + 1] ); + ext = buf + ext_offset + 2; + MBEDTLS_SSL_DEBUG_BUF( 3, "client hello extensions", ext, ext_len ); - if( ( ext_len > 0 && ext_len < 4 ) || - msg_len != ext_offset + 2 + ext_len ) + while( ext_len != 0 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); - } - } - else - ext_len = 0; + unsigned int ext_id = ( ( ext[0] << 8 ) + | ( ext[1] ) ); + unsigned int ext_size = ( ( ext[2] << 8 ) + | ( ext[3] ) ); - ext = buf + ext_offset + 2; - MBEDTLS_SSL_DEBUG_BUF( 3, "client hello extensions", ext, ext_len ); - - while( ext_len != 0 ) - { - unsigned int ext_id = ( ( ext[0] << 8 ) - | ( ext[1] ) ); - unsigned int ext_size = ( ( ext[2] << 8 ) - | ( ext[3] ) ); - - if( ext_size + 4 > ext_len ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); - } - switch( ext_id ) - { + if( ext_size + 4 > ext_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } + switch( ext_id ) + { #if defined(MBEDTLS_SSL_SERVER_NAME_INDICATION) - case MBEDTLS_TLS_EXT_SERVERNAME: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found ServerName extension" ) ); - if( ssl->conf->f_sni == NULL ) - break; + case MBEDTLS_TLS_EXT_SERVERNAME: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found ServerName extension" ) ); + if( ssl->conf->f_sni == NULL ) + break; - ret = ssl_parse_servername_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_servername_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_SERVER_NAME_INDICATION */ - case MBEDTLS_TLS_EXT_RENEGOTIATION_INFO: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found renegotiation extension" ) ); + case MBEDTLS_TLS_EXT_RENEGOTIATION_INFO: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found renegotiation extension" ) ); #if defined(MBEDTLS_SSL_RENEGOTIATION) - renegotiation_info_seen = 1; + renegotiation_info_seen = 1; #endif - ret = ssl_parse_renegotiation_info( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_renegotiation_info( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #if defined(MBEDTLS_SSL_PROTO_TLS1_2) && \ defined(MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED) - case MBEDTLS_TLS_EXT_SIG_ALG: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found signature_algorithms extension" ) ); + case MBEDTLS_TLS_EXT_SIG_ALG: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found signature_algorithms extension" ) ); #if defined(MBEDTLS_SSL_RENEGOTIATION) - if( ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) - break; + if( ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) + break; #endif - ret = ssl_parse_signature_algorithms_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_signature_algorithms_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_PROTO_TLS1_2 && MBEDTLS_KEY_EXCHANGE__WITH_CERT__ENABLED */ #if defined(MBEDTLS_ECDH_C) || defined(MBEDTLS_ECDSA_C) - case MBEDTLS_TLS_EXT_SUPPORTED_ELLIPTIC_CURVES: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported elliptic curves extension" ) ); + case MBEDTLS_TLS_EXT_SUPPORTED_ELLIPTIC_CURVES: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported elliptic curves extension" ) ); + ret = ssl_parse_supported_elliptic_curves( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; - ret = ssl_parse_supported_elliptic_curves( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + case MBEDTLS_TLS_EXT_SUPPORTED_POINT_FORMATS: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported point formats extension" ) ); + ssl->handshake->cli_exts |= MBEDTLS_TLS_EXT_SUPPORTED_POINT_FORMATS_PRESENT; - case MBEDTLS_TLS_EXT_SUPPORTED_POINT_FORMATS: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found supported point formats extension" ) ); - ssl->handshake->cli_exts |= MBEDTLS_TLS_EXT_SUPPORTED_POINT_FORMATS_PRESENT; - - ret = ssl_parse_supported_point_formats( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_supported_point_formats( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_ECDH_C || MBEDTLS_ECDSA_C */ #if defined(MBEDTLS_SSL_MAX_FRAGMENT_LENGTH) - case MBEDTLS_TLS_EXT_MAX_FRAGMENT_LENGTH: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found max fragment length extension" ) ); + case MBEDTLS_TLS_EXT_MAX_FRAGMENT_LENGTH: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found max fragment length extension" ) ); - ret = ssl_parse_max_fragment_length_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_max_fragment_length_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_MAX_FRAGMENT_LENGTH */ #if defined(MBEDTLS_SSL_TRUNCATED_HMAC) - case MBEDTLS_TLS_EXT_TRUNCATED_HMAC: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found truncated hmac extension" ) ); + case MBEDTLS_TLS_EXT_TRUNCATED_HMAC: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found truncated hmac extension" ) ); - ret = ssl_parse_truncated_hmac_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_truncated_hmac_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_TRUNCATED_HMAC */ #if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC) - case MBEDTLS_TLS_EXT_ENCRYPT_THEN_MAC: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found encrypt then mac extension" ) ); + case MBEDTLS_TLS_EXT_ENCRYPT_THEN_MAC: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found encrypt then mac extension" ) ); - ret = ssl_parse_encrypt_then_mac_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_encrypt_then_mac_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_ENCRYPT_THEN_MAC */ #if defined(MBEDTLS_SSL_EXTENDED_MASTER_SECRET) - case MBEDTLS_TLS_EXT_EXTENDED_MASTER_SECRET: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found extended master secret extension" ) ); + case MBEDTLS_TLS_EXT_EXTENDED_MASTER_SECRET: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found extended master secret extension" ) ); - ret = ssl_parse_extended_ms_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_extended_ms_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_EXTENDED_MASTER_SECRET */ #if defined(MBEDTLS_SSL_SESSION_TICKETS) - case MBEDTLS_TLS_EXT_SESSION_TICKET: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found session ticket extension" ) ); + case MBEDTLS_TLS_EXT_SESSION_TICKET: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found session ticket extension" ) ); - ret = ssl_parse_session_ticket_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_session_ticket_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_SESSION_TICKETS */ #if defined(MBEDTLS_SSL_ALPN) - case MBEDTLS_TLS_EXT_ALPN: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "found alpn extension" ) ); + case MBEDTLS_TLS_EXT_ALPN: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "found alpn extension" ) ); - ret = ssl_parse_alpn_ext( ssl, ext + 4, ext_size ); - if( ret != 0 ) - return( ret ); - break; + ret = ssl_parse_alpn_ext( ssl, ext + 4, ext_size ); + if( ret != 0 ) + return( ret ); + break; #endif /* MBEDTLS_SSL_SESSION_TICKETS */ - default: - MBEDTLS_SSL_DEBUG_MSG( 3, ( "unknown extension found: %d (ignoring)", - ext_id ) ); + default: + MBEDTLS_SSL_DEBUG_MSG( 3, ( "unknown extension found: %d (ignoring)", + ext_id ) ); + } + + ext_len -= 4 + ext_size; + ext += 4 + ext_size; + + if( ext_len > 0 && ext_len < 4 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } } - - ext_len -= 4 + ext_size; - ext += 4 + ext_size; - - if( ext_len > 0 && ext_len < 4 ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); - } - - } - #if defined(MBEDTLS_SSL_PROTO_SSL3) } #endif