From a3929bac1e35e635d4a86e681f54a1d369e3e4d7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 8 May 2017 16:31:14 +0100 Subject: [PATCH 1/2] Fix implementation of VERIFY_OPTIONAL verification mode This commit changes the behaviour of mbedtls_ssl_parse_certificate to make the two authentication modes MBEDTLS_SSL_VERIFY_REQUIRED and MBEDTLS_SSL_VERIFY_OPTIONAL be in the following relationship: Mode == MBEDTLS_SSL_VERIFY_REQUIRED <=> Mode == MBEDTLS_SSL_VERIFY_OPTIONAL + check verify result Also, it changes the behaviour to perform the certificate chain verification even if the trusted CA chain is empty. Previously, the function failed in this case, even when using optional verification, which was brought up in #864. --- ChangeLog | 7 +++++++ library/ssl_tls.c | 31 ++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/ChangeLog b/ChangeLog index fc7e2ebc0..c8519e485 100644 --- a/ChangeLog +++ b/ChangeLog @@ -23,6 +23,13 @@ Bugfix * Fixed issue in mutexes to failing to initialise. #667 * Fix insufficient support for signature-hash-algorithm extension, resulting in compatibility problems with Chrome. Found by hfloyrd. #823 + * Accept empty trusted CA chain in authentication mode + MBEDTLS_SSL_VERIFY_OPTIONAL. + Fixes #864. Found by jethrogb. + * Fix implementation of mbedtls_ssl_parse_certificate + to not annihilate fatal errors in authentication mode + MBEDTLS_SSL_VERIFY_OPTIONAL and to reflect bad EC curves + within verification result. = mbed TLS 2.1.7 branch released 2017-03-08 diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bcefe954e..5d22d02fc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4378,12 +4378,6 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) ca_crl = ssl->conf->ca_crl; } - if( ca_chain == NULL ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "got no CA chain" ) ); - return( MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED ); - } - /* * Main check: verify certificate */ @@ -4412,6 +4406,8 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) if( mbedtls_pk_can_do( pk, MBEDTLS_PK_ECKEY ) && mbedtls_ssl_check_curve( ssl, mbedtls_pk_ec( *pk )->grp.id ) != 0 ) { + ssl->session_negotiate->verify_result |= MBEDTLS_X509_BADCERT_BAD_KEY; + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (EC key curve)" ) ); if( ret == 0 ) ret = MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE; @@ -4420,8 +4416,8 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) #endif /* MBEDTLS_ECP_C */ if( mbedtls_ssl_check_cert_usage( ssl->session_negotiate->peer_cert, - ciphersuite_info, - ! ssl->conf->endpoint, + ciphersuite_info, + ! ssl->conf->endpoint, &ssl->session_negotiate->verify_result ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (usage extensions)" ) ); @@ -4429,8 +4425,25 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) ret = MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE; } - if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL ) + /* mbedtls_x509_crt_verify_with_profile is supposed to report a + * verification failure through MBEDTLS_ERR_X509_CERT_VERIFY_FAILED, + * with details encoded in the verification flags. All other kinds + * of error codes, including those from the user provided f_vrfy + * functions, are treated as fatal and lead to a failure of + * ssl_parse_certificate even if verification was optional. */ + if( authmode == MBEDTLS_SSL_VERIFY_OPTIONAL && + ( ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED || + ret == MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ) ) + { ret = 0; + } + + if( ca_chain == NULL && authmode == MBEDTLS_SSL_VERIFY_REQUIRED ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "got no CA chain" ) ); + ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; + } + } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); From 61c0c7041852e3bdd981a06e083b5ae134d778a4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 15 May 2017 16:05:15 +0100 Subject: [PATCH 2/2] Add tests for missing CA chains and bad curves. This commit adds four tests to tests/ssl-opt.sh: (1) & (2): Check behaviour of optional/required verification when the trusted CA chain is empty. (3) & (4): Check behaviour of optional/required verification when the client receives a server certificate with an unsupported curve. --- library/ssl_tls.c | 11 +++++ programs/ssl/ssl_client2.c | 88 ++++++++++++++++++++++++++++++++++++++ programs/ssl/ssl_server2.c | 87 +++++++++++++++++++++++++++++++++++++ tests/ssl-opt.sh | 48 +++++++++++++++++++++ 4 files changed, 234 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 5d22d02fc..22c3f9936 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4444,6 +4444,17 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) ret = MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED; } +#if defined(MBEDTLS_DEBUG_C) + if( ssl->session_negotiate->verify_result != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 3, ( "! Certificate verification flags %x", + ssl->session_negotiate->verify_result ) ); + } + else + { + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Certificate verification flags clear" ) ); + } +#endif /* MBEDTLS_DEBUG_C */ } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 27284fc5a..64b8f5f13 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -94,6 +94,7 @@ int main( void ) #define DFL_RECONNECT_HARD 0 #define DFL_TICKETS MBEDTLS_SSL_SESSION_TICKETS_ENABLED #define DFL_ALPN_STRING NULL +#define DFL_CURVES NULL #define DFL_TRANSPORT MBEDTLS_SSL_TRANSPORT_STREAM #define DFL_HS_TO_MIN 0 #define DFL_HS_TO_MAX 0 @@ -174,6 +175,17 @@ int main( void ) #define USAGE_ALPN "" #endif /* MBEDTLS_SSL_ALPN */ +#if defined(MBEDTLS_ECP_C) +#define USAGE_CURVES \ + " curves=a,b,c,d default: \"default\" (library default)\n" \ + " example: \"secp521r1,brainpoolP512r1\"\n" \ + " - use \"none\" for empty list\n" \ + " - see mbedtls_ecp_curve_list()\n" \ + " for acceptable curve names\n" +#else +#define USAGE_CURVES "" +#endif + #if defined(MBEDTLS_SSL_PROTO_DTLS) #define USAGE_DTLS \ " dtls=%%d default: 0 (TLS)\n" \ @@ -248,6 +260,7 @@ int main( void ) USAGE_FALLBACK \ USAGE_EMS \ USAGE_ETM \ + USAGE_CURVES \ USAGE_RECSPLIT \ USAGE_DHMLEN \ "\n" \ @@ -300,6 +313,7 @@ struct options int reco_delay; /* delay in seconds before resuming session */ int reconnect_hard; /* unexpectedly reconnect from the same port */ int tickets; /* enable / disable session tickets */ + const char *curves; /* list of supported elliptic curves */ const char *alpn_string; /* ALPN supported protocols */ int transport; /* TLS or DTLS? */ uint32_t hs_to_min; /* Initial value of DTLS handshake timer */ @@ -415,6 +429,11 @@ int main( int argc, char *argv[] ) #if defined(MBEDTLS_SSL_ALPN) const char *alpn_list[10]; #endif +#if defined(MBEDTLS_ECP_C) + mbedtls_ecp_group_id curve_list[20]; + const mbedtls_ecp_curve_info *curve_cur; +#endif + const char *pers = "ssl_client2"; #if defined(MBEDTLS_X509_CRT_PARSE_C) @@ -510,6 +529,7 @@ int main( int argc, char *argv[] ) opt.reconnect_hard = DFL_RECONNECT_HARD; opt.tickets = DFL_TICKETS; opt.alpn_string = DFL_ALPN_STRING; + opt.curves = DFL_CURVES; opt.transport = DFL_TRANSPORT; opt.hs_to_min = DFL_HS_TO_MIN; opt.hs_to_max = DFL_HS_TO_MAX; @@ -664,6 +684,8 @@ int main( int argc, char *argv[] ) default: goto usage; } } + else if( strcmp( p, "curves" ) == 0 ) + opt.curves = q; else if( strcmp( p, "etm" ) == 0 ) { switch( atoi( q ) ) @@ -921,6 +943,64 @@ int main( int argc, char *argv[] ) } #endif /* MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED */ +#if defined(MBEDTLS_ECP_C) + if( opt.curves != NULL ) + { + p = (char *) opt.curves; + i = 0; + + if( strcmp( p, "none" ) == 0 ) + { + curve_list[0] = MBEDTLS_ECP_DP_NONE; + } + else if( strcmp( p, "default" ) != 0 ) + { + /* Leave room for a final NULL in curve list */ + while( i < (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1 + && *p != '\0' ) + { + q = p; + + /* Terminate the current string */ + while( *p != ',' && *p != '\0' ) + p++; + if( *p == ',' ) + *p++ = '\0'; + + if( ( curve_cur = mbedtls_ecp_curve_info_from_name( q ) ) != NULL ) + { + curve_list[i++] = curve_cur->grp_id; + } + else + { + mbedtls_printf( "unknown curve %s\n", q ); + mbedtls_printf( "supported curves: " ); + for( curve_cur = mbedtls_ecp_curve_list(); + curve_cur->grp_id != MBEDTLS_ECP_DP_NONE; + curve_cur++ ) + { + mbedtls_printf( "%s ", curve_cur->name ); + } + mbedtls_printf( "\n" ); + goto exit; + } + } + + mbedtls_printf("Number of curves: %d\n", i ); + + if( i == (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1 + && *p != '\0' ) + { + mbedtls_printf( "curves list too long, maximum %zu", + (size_t) ( sizeof( curve_list ) / sizeof( *curve_list ) - 1 ) ); + goto exit; + } + + curve_list[i] = MBEDTLS_ECP_DP_NONE; + } + } +#endif /* MBEDTLS_ECP_C */ + #if defined(MBEDTLS_SSL_ALPN) if( opt.alpn_string != NULL ) { @@ -1210,6 +1290,14 @@ int main( int argc, char *argv[] ) } #endif +#if defined(MBEDTLS_ECP_C) + if( opt.curves != NULL && + strcmp( opt.curves, "default" ) != 0 ) + { + mbedtls_ssl_conf_curves( &conf, curve_list ); + } +#endif + #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED) if( ( ret = mbedtls_ssl_conf_psk( &conf, psk, psk_len, (const unsigned char *) opt.psk_identity, diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 602da475a..f2db91fd5 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -129,6 +129,7 @@ int main( void ) #define DFL_CACHE_TIMEOUT -1 #define DFL_SNI NULL #define DFL_ALPN_STRING NULL +#define DFL_CURVES NULL #define DFL_DHM_FILE NULL #define DFL_TRANSPORT MBEDTLS_SSL_TRANSPORT_STREAM #define DFL_COOKIES 1 @@ -299,6 +300,17 @@ int main( void ) #define USAGE_RENEGO "" #endif +#if defined(MBEDTLS_ECP_C) +#define USAGE_CURVES \ + " curves=a,b,c,d default: \"default\" (library default)\n" \ + " example: \"secp521r1,brainpoolP512r1\"\n" \ + " - use \"none\" for empty list\n" \ + " - see mbedtls_ecp_curve_list()\n" \ + " for acceptable curve names\n" +#else +#define USAGE_CURVES "" +#endif + #define USAGE \ "\n usage: ssl_server2 param=<>...\n" \ "\n acceptable parameters:\n" \ @@ -332,6 +344,7 @@ int main( void ) USAGE_ALPN \ USAGE_EMS \ USAGE_ETM \ + USAGE_CURVES \ "\n" \ " arc4=%%d default: (library default: 0)\n" \ " allow_sha1=%%d default: 0\n" \ @@ -398,6 +411,7 @@ struct options int cache_max; /* max number of session cache entries */ int cache_timeout; /* expiration delay of session cache entries */ char *sni; /* string describing sni information */ + const char *curves; /* list of supported elliptic curves */ const char *alpn_string; /* ALPN supported protocols */ const char *dhm_file; /* the file with the DH parameters */ int extended_ms; /* allow negotiation of extended MS? */ @@ -854,6 +868,10 @@ int main( int argc, char *argv[] ) #if defined(SNI_OPTION) sni_entry *sni_info = NULL; #endif +#if defined(MBEDTLS_ECP_C) + mbedtls_ecp_group_id curve_list[20]; + const mbedtls_ecp_curve_info * curve_cur; +#endif #if defined(MBEDTLS_SSL_ALPN) const char *alpn_list[10]; #endif @@ -963,6 +981,7 @@ int main( int argc, char *argv[] ) opt.cache_timeout = DFL_CACHE_TIMEOUT; opt.sni = DFL_SNI; opt.alpn_string = DFL_ALPN_STRING; + opt.curves = DFL_CURVES; opt.dhm_file = DFL_DHM_FILE; opt.transport = DFL_TRANSPORT; opt.cookies = DFL_COOKIES; @@ -1039,6 +1058,8 @@ int main( int argc, char *argv[] ) } opt.force_ciphersuite[1] = 0; } + else if( strcmp( p, "curves" ) == 0 ) + opt.curves = q; else if( strcmp( p, "version_suites" ) == 0 ) opt.version_suites = q; else if( strcmp( p, "renegotiation" ) == 0 ) @@ -1392,6 +1413,64 @@ int main( int argc, char *argv[] ) } #endif /* MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED */ +#if defined(MBEDTLS_ECP_C) + if( opt.curves != NULL ) + { + p = (char *) opt.curves; + i = 0; + + if( strcmp( p, "none" ) == 0 ) + { + curve_list[0] = MBEDTLS_ECP_DP_NONE; + } + else if( strcmp( p, "default" ) != 0 ) + { + /* Leave room for a final NULL in curve list */ + while( i < (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1 + && *p != '\0' ) + { + q = p; + + /* Terminate the current string */ + while( *p != ',' && *p != '\0' ) + p++; + if( *p == ',' ) + *p++ = '\0'; + + if( ( curve_cur = mbedtls_ecp_curve_info_from_name( q ) ) != NULL ) + { + curve_list[i++] = curve_cur->grp_id; + } + else + { + mbedtls_printf( "unknown curve %s\n", q ); + mbedtls_printf( "supported curves: " ); + for( curve_cur = mbedtls_ecp_curve_list(); + curve_cur->grp_id != MBEDTLS_ECP_DP_NONE; + curve_cur++ ) + { + mbedtls_printf( "%s ", curve_cur->name ); + } + mbedtls_printf( "\n" ); + goto exit; + } + } + + mbedtls_printf("Number of curves: %d\n", i ); + + if( i == (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1 + && *p != '\0' ) + { + mbedtls_printf( "curves list too long, maximum %zu", + (size_t) ( sizeof( curve_list ) / sizeof( *curve_list ) - 1 ) ); + goto exit; + } + + curve_list[i] = MBEDTLS_ECP_DP_NONE; + } + } +#endif /* MBEDTLS_ECP_C */ + #if defined(MBEDTLS_SSL_ALPN) if( opt.alpn_string != NULL ) { @@ -1840,6 +1919,14 @@ int main( int argc, char *argv[] ) mbedtls_ssl_conf_sni( &conf, sni_callback, sni_info ); #endif +#if defined(MBEDTLS_ECP_C) + if( opt.curves != NULL && + strcmp( opt.curves, "default" ) != 0 ) + { + mbedtls_ssl_conf_curves( &conf, curve_list ); + } +#endif + #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED) if( strlen( opt.psk ) != 0 && strlen( opt.psk_identity ) != 0 ) { diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 156785ee4..2ca1490ee 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1763,6 +1763,54 @@ run_test "Authentication: server badcert, client optional" \ -C "! mbedtls_ssl_handshake returned" \ -C "X509 - Certificate verification failed" +run_test "Authentication: server goodcert, client optional, no trusted CA" \ + "$P_SRV" \ + "$P_CLI debug_level=3 auth_mode=optional ca_file=none ca_path=none" \ + 0 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -c "! Certificate verification flags"\ + -C "! mbedtls_ssl_handshake returned" \ + -C "X509 - Certificate verification failed" \ + -C "SSL - No CA Chain is set, but required to operate" + +run_test "Authentication: server goodcert, client required, no trusted CA" \ + "$P_SRV" \ + "$P_CLI debug_level=3 auth_mode=required ca_file=none ca_path=none" \ + 1 \ + -c "x509_verify_cert() returned" \ + -c "! The certificate is not correctly signed by the trusted CA" \ + -c "! Certificate verification flags"\ + -c "! mbedtls_ssl_handshake returned" \ + -c "SSL - No CA Chain is set, but required to operate" + +# The purpose of the next two tests is to test the client's behaviour when receiving a server +# certificate with an unsupported elliptic curve. This should usually not happen because +# the client informs the server about the supported curves - it does, though, in the +# corner case of a static ECDH suite, because the server doesn't check the curve on that +# occasion (to be fixed). If that bug's fixed, the test needs to be altered to use a +# different means to have the server ignoring the client's supported curve list. + +requires_config_enabled MBEDTLS_ECP_C +run_test "Authentication: server ECDH p256v1, client required, p256v1 unsupported" \ + "$P_SRV debug_level=1 key_file=data_files/server5.key \ + crt_file=data_files/server5.ku-ka.crt" \ + "$P_CLI debug_level=3 auth_mode=required curves=secp521r1" \ + 1 \ + -c "bad certificate (EC key curve)"\ + -c "! Certificate verification flags"\ + -C "bad server certificate (ECDH curve)" # Expect failure at earlier verification stage + +requires_config_enabled MBEDTLS_ECP_C +run_test "Authentication: server ECDH p256v1, client optional, p256v1 unsupported" \ + "$P_SRV debug_level=1 key_file=data_files/server5.key \ + crt_file=data_files/server5.ku-ka.crt" \ + "$P_CLI debug_level=3 auth_mode=optional curves=secp521r1" \ + 1 \ + -c "bad certificate (EC key curve)"\ + -c "! Certificate verification flags"\ + -c "bad server certificate (ECDH curve)" # Expect failure only at ECDH params check + run_test "Authentication: server badcert, client none" \ "$P_SRV crt_file=data_files/server5-badsign.crt \ key_file=data_files/server5.key" \