From 39ae8cd2077d2e9e7d00161162ad8d3fb45dba27 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 | 33 ++++++++++++++++++++++----------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/ChangeLog b/ChangeLog index 1b6a3542d..da0755ad5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -17,6 +17,13 @@ Bugfix that triggered the alert. * In SSLv3, if refusing a renegotiation attempt, don't process any further data. + * 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. Changes * Send fatal alerts in many more cases instead of dropping the connection. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 1e5f8e493..ff951a536 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4472,14 +4472,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" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_BAD_CERT ); - return( MBEDTLS_ERR_SSL_CA_CHAIN_REQUIRED ); - } - /* * Main check: verify certificate */ @@ -4508,6 +4500,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; @@ -4516,8 +4510,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)" ) ); @@ -4525,8 +4519,24 @@ 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; + } if( ret != 0 ) { @@ -4558,6 +4568,7 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, alert ); } + } MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); From e6706e62d8296d5d9630c0c6edec278f6cc02698 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 ff951a536..208500a66 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4569,6 +4569,17 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) alert ); } +#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 212f47a6c..9a75c7f0e 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -98,6 +98,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 @@ -178,6 +179,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" \ @@ -260,6 +272,7 @@ int main( void ) USAGE_FALLBACK \ USAGE_EMS \ USAGE_ETM \ + USAGE_CURVES \ USAGE_RECSPLIT \ USAGE_DHMLEN \ "\n" \ @@ -313,6 +326,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 */ @@ -428,6 +442,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) @@ -524,6 +543,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; @@ -680,6 +700,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 ) ) @@ -937,6 +959,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 ) { @@ -1226,6 +1306,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 05827106c..cbc654e90 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -134,6 +134,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 @@ -311,6 +312,17 @@ int main( void ) #define USAGE_ECJPAKE "" #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" \ @@ -347,6 +359,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" \ @@ -415,6 +428,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? */ @@ -871,6 +885,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 @@ -982,6 +1000,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; @@ -1060,6 +1079,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 ) @@ -1419,6 +1440,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 ) { @@ -1870,6 +1949,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 b57edd7fe..688baa7ef 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1837,6 +1837,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" \