From 888c2fde60737adc5250baff3f4d4e23084c99d2 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 11 May 2017 11:12:40 +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 SSL_VERIFY_REQUIRED and SSL_VERIFY_OPTIONAL be in the following relationship: Mode == SSL_VERIFY_REQUIRED <=> Mode == 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 | 6 ++++++ include/polarssl/x509.h | 2 ++ library/ssl_tls.c | 38 +++++++++++++++++++++++++++++++------- library/x509_crt.c | 1 + 4 files changed, 40 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index a64de5dd1..d33897c8a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -15,6 +15,12 @@ Bugfix * Wipe stack buffers in RSA private key operations (rsa_rsaes_pkcs1_v15_decrypt(), rsa_rsaes_oaep_decrypt). Found by Laurent Simon. + * Accept empty trusted CA chain in authentication mode + SSL_VERIFY_OPTIONAL. Fixes #864. Found by jethrogb. + * Fix implementation of ssl_parse_certificate + to not annihilate fatal errors in authentication mode + SSL_VERIFY_OPTIONAL and to reflect bad EC curves + within verification result. = mbed TLS 1.3.19 branch released 2017-03-08 diff --git a/include/polarssl/x509.h b/include/polarssl/x509.h index cd01539b9..adaacfff1 100644 --- a/include/polarssl/x509.h +++ b/include/polarssl/x509.h @@ -97,6 +97,8 @@ #define BADCERT_KEY_USAGE 0x0800 /**< Usage does not match the keyUsage extension. */ #define BADCERT_EXT_KEY_USAGE 0x1000 /**< Usage does not match the extendedKeyUsage extension. */ #define BADCERT_NS_CERT_TYPE 0x2000 /**< Usage does not match the nsCertType extension. */ +#define BADCERT_BAD_KEY 0x10000 /**< Bad key (e.g. unsupported elliptic curve in use) */ + /* \} name */ /* \} addtogroup x509_module */ diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 7cf968d92..577922978 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2841,12 +2841,6 @@ int ssl_parse_certificate( ssl_context *ssl ) if( ssl->authmode != SSL_VERIFY_NONE ) { - if( ssl->ca_chain == NULL ) - { - SSL_DEBUG_MSG( 1, ( "got no CA chain" ) ); - return( POLARSSL_ERR_SSL_CA_CHAIN_REQUIRED ); - } - /* * Main check: verify certificate */ @@ -2872,6 +2866,8 @@ int ssl_parse_certificate( ssl_context *ssl ) if( pk_can_do( pk, POLARSSL_PK_ECKEY ) && ! ssl_curve_is_acceptable( ssl, pk_ec( *pk )->grp.id ) ) { + ssl->session_negotiate->verify_result |= BADCERT_BAD_KEY; + SSL_DEBUG_MSG( 1, ( "bad certificate (EC key curve)" ) ); if( ret == 0 ) ret = POLARSSL_ERR_SSL_BAD_HS_CERTIFICATE; @@ -2889,8 +2885,36 @@ int ssl_parse_certificate( ssl_context *ssl ) ret = POLARSSL_ERR_SSL_BAD_HS_CERTIFICATE; } - if( ssl->authmode != SSL_VERIFY_REQUIRED ) + /* x509_crt_verify_with_profile is supposed to report a + * verification failure through POLARSSL_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( ssl->authmode == SSL_VERIFY_OPTIONAL && + ( ret == POLARSSL_ERR_X509_CERT_VERIFY_FAILED || + ret == POLARSSL_ERR_SSL_BAD_HS_CERTIFICATE ) ) + { ret = 0; + } + + if( ssl->ca_chain == NULL && ssl->authmode == SSL_VERIFY_REQUIRED ) + { + SSL_DEBUG_MSG( 1, ( "got no CA chain" ) ); + ret = POLARSSL_ERR_SSL_CA_CHAIN_REQUIRED; + } + +#if defined(POLARSSL_DEBUG_C) + if( ssl->session_negotiate->verify_result != 0 ) + { + SSL_DEBUG_MSG( 3, ( "! Certificate verification flags %x", + ssl->session_negotiate->verify_result ) ); + } + else + { + SSL_DEBUG_MSG( 3, ( "Certificate verification flags clear" ) ); + } +#endif /* POLARSSL_DEBUG_C */ } SSL_DEBUG_MSG( 2, ( "<= parse certificate" ) ); diff --git a/library/x509_crt.c b/library/x509_crt.c index a3517f64f..16a29b5b3 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1403,6 +1403,7 @@ static const struct x509_crt_verify_string x509_crt_verify_strings[] = { { BADCERT_KEY_USAGE, "Usage does not match the keyUsage extension" }, { BADCERT_EXT_KEY_USAGE, "Usage does not match the extendedKeyUsage extension" }, { BADCERT_NS_CERT_TYPE, "Usage does not match the nsCertType extension" }, + { BADCERT_BAD_KEY, "The certificate uses an invalid key (e.g. unsupported elliptic curve)" }, { 0, NULL } }; From 6fd6d248ae5d76f8ad68a0c96228481ed60f5e02 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 25 May 2017 17:51:31 +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. --- programs/ssl/ssl_client2.c | 88 ++++++++++++++++++++++++++++++++++++++ programs/ssl/ssl_server2.c | 87 +++++++++++++++++++++++++++++++++++++ tests/ssl-opt.sh | 50 +++++++++++++++++++++- 3 files changed, 224 insertions(+), 1 deletion(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index cdadf59a4..e30224492 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -92,6 +92,7 @@ #define DFL_RECO_DELAY 0 #define DFL_TICKETS SSL_SESSION_TICKETS_ENABLED #define DFL_ALPN_STRING NULL +#define DFL_CURVES NULL #define DFL_FALLBACK -1 #define DFL_EXTENDED_MS -1 #define DFL_ETM -1 @@ -169,6 +170,17 @@ #define USAGE_ALPN "" #endif /* POLARSSL_SSL_ALPN */ +#if defined(POLARSSL_SSL_SET_CURVES) +#define USAGE_CURVES \ + " curves=a,b,c,d default: \"default\" (library default)\n" \ + " example: \"secp521r1,brainpoolP512r1\"\n" \ + " - use \"none\" for empty list\n" \ + " - see ecp_curve_list()\n" \ + " for acceptable curve names\n" +#else +#define USAGE_CURVES "" +#endif /* POLARSSL_SSL_SET_CURVES */ + #if defined(POLARSSL_SSL_FALLBACK_SCSV) #define USAGE_FALLBACK \ " fallback=0/1 default: (library default: off)\n" @@ -226,6 +238,7 @@ USAGE_MAX_FRAG_LEN \ USAGE_TRUNC_HMAC \ USAGE_ALPN \ + USAGE_CURVES \ USAGE_FALLBACK \ USAGE_EMS \ USAGE_ETM \ @@ -285,6 +298,7 @@ struct options int reconnect; /* attempt to resume session */ int reco_delay; /* delay in seconds before resuming session */ int tickets; /* enable / disable session tickets */ + const char *curves; /* list of supported elliptic curves */ const char *alpn_string; /* ALPN supported protocols */ int fallback; /* is this a fallback connection? */ int extended_ms; /* negotiate extended master secret? */ @@ -373,6 +387,11 @@ int main( int argc, char *argv[] ) #if defined(POLARSSL_SSL_ALPN) const char *alpn_list[10]; #endif +#if defined(POLARSSL_SSL_SET_CURVES) + ecp_group_id curve_list[20]; + const ecp_curve_info *curve_cur; +#endif + const char *pers = "ssl_client2"; entropy_context entropy; @@ -453,6 +472,7 @@ int main( int argc, char *argv[] ) opt.reco_delay = DFL_RECO_DELAY; opt.tickets = DFL_TICKETS; opt.alpn_string = DFL_ALPN_STRING; + opt.curves = DFL_CURVES; opt.fallback = DFL_FALLBACK; opt.extended_ms = DFL_EXTENDED_MS; opt.etm = DFL_ETM; @@ -584,6 +604,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 ) ) @@ -775,6 +797,64 @@ int main( int argc, char *argv[] ) } #endif /* POLARSSL_KEY_EXCHANGE__SOME__PSK_ENABLED */ +#if defined(POLARSSL_SSL_SET_CURVES) + if( opt.curves != NULL ) + { + p = (char *) opt.curves; + i = 0; + + if( strcmp( p, "none" ) == 0 ) + { + curve_list[0] = POLARSSL_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 = ecp_curve_info_from_name( q ) ) != NULL ) + { + curve_list[i++] = curve_cur->grp_id; + } + else + { + polarssl_printf( "unknown curve %s\n", q ); + polarssl_printf( "supported curves: " ); + for( curve_cur = ecp_curve_list(); + curve_cur->grp_id != POLARSSL_ECP_DP_NONE; + curve_cur++ ) + { + polarssl_printf( "%s ", curve_cur->name ); + } + polarssl_printf( "\n" ); + goto exit; + } + } + + polarssl_printf( "Number of curves: %d\n", i ); + + if( i == (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1 + && *p != '\0' ) + { + polarssl_printf( "curves list too long, maximum %zu", + (size_t) ( sizeof( curve_list ) / sizeof( *curve_list ) - 1 ) ); + goto exit; + } + + curve_list[i] = POLARSSL_ECP_DP_NONE; + } + } +#endif /* POLARSSL_SSL_SET_CURVES */ + #if defined(POLARSSL_SSL_ALPN) if( opt.alpn_string != NULL ) { @@ -996,6 +1076,14 @@ int main( int argc, char *argv[] ) } #endif +#if defined(POLARSSL_SSL_SET_CURVES) + if( opt.curves != NULL && + strcmp( opt.curves, "default" ) != 0 ) + { + ssl_set_curves( &ssl, curve_list ); + } +#endif + ssl_set_rng( &ssl, ctr_drbg_random, &ctr_drbg ); ssl_set_dbg( &ssl, my_debug, stdout ); diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 095fabd49..95438f906 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -106,6 +106,7 @@ #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_EXTENDED_MS -1 #define DFL_ETM -1 @@ -215,6 +216,17 @@ #define USAGE_ALPN "" #endif /* POLARSSL_SSL_ALPN */ +#if defined(POLARSSL_SSL_SET_CURVES) +#define USAGE_CURVES \ + " curves=a,b,c,d default: \"default\" (library default)\n" \ + " example: \"secp521r1,brainpoolP512r1\"\n" \ + " - use \"none\" for empty list\n" \ + " - see ecp_curve_list()\n" \ + " for acceptable curve names\n" +#else +#define USAGE_CURVES "" +#endif /* POLARSSL_SSL_SET_CURVES */ + #if defined(POLARSSL_SSL_EXTENDED_MASTER_SECRET) #define USAGE_EMS \ " extended_ms=0/1 default: (library default: on)\n" @@ -263,6 +275,7 @@ USAGE_MAX_FRAG_LEN \ USAGE_TRUNC_HMAC \ USAGE_ALPN \ + USAGE_CURVES \ USAGE_EMS \ USAGE_ETM \ "\n" \ @@ -327,6 +340,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? */ @@ -685,6 +699,10 @@ int main( int argc, char *argv[] ) #if defined(POLARSSL_SSL_ALPN) const char *alpn_list[10]; #endif +#if defined(POLARSSL_SSL_SET_CURVES) + ecp_group_id curve_list[20]; + const ecp_curve_info *curve_cur; +#endif #if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) unsigned char alloc_buf[100000]; #endif @@ -780,6 +798,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.extended_ms = DFL_EXTENDED_MS; opt.etm = DFL_ETM; @@ -987,6 +1006,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 ) ) @@ -1118,6 +1139,64 @@ int main( int argc, char *argv[] ) } #endif /* POLARSSL_KEY_EXCHANGE__SOME__PSK_ENABLED */ +#if defined(POLARSSL_SSL_SET_CURVES) + if( opt.curves != NULL ) + { + p = (char *) opt.curves; + i = 0; + + if( strcmp( p, "none" ) == 0 ) + { + curve_list[0] = POLARSSL_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 = ecp_curve_info_from_name( q ) ) != NULL ) + { + curve_list[i++] = curve_cur->grp_id; + } + else + { + polarssl_printf( "unknown curve %s\n", q ); + polarssl_printf( "supported curves: " ); + for( curve_cur = ecp_curve_list(); + curve_cur->grp_id != POLARSSL_ECP_DP_NONE; + curve_cur++ ) + { + polarssl_printf( "%s ", curve_cur->name ); + } + polarssl_printf( "\n" ); + goto exit; + } + } + + polarssl_printf( "Number of curves: %d\n", i ); + + if( i == (int) ( sizeof( curve_list ) / sizeof( *curve_list ) ) - 1 + && *p != '\0' ) + { + polarssl_printf( "curves list too long, maximum %zu", + (size_t) ( sizeof( curve_list ) / sizeof( *curve_list ) - 1 ) ); + goto exit; + } + + curve_list[i] = POLARSSL_ECP_DP_NONE; + } + } +#endif /* POLARSSL_SSL_SET_CURVES */ + #if defined(POLARSSL_SSL_ALPN) if( opt.alpn_string != NULL ) { @@ -1397,6 +1476,14 @@ int main( int argc, char *argv[] ) } #endif +#if defined(POLARSSL_SSL_SET_CURVES) + if( opt.curves != NULL && + strcmp( opt.curves, "default" ) != 0 ) + { + ssl_set_curves( &ssl, curve_list ); + } +#endif + ssl_set_rng( &ssl, ctr_drbg_random, &ctr_drbg ); ssl_set_dbg( &ssl, my_debug, stdout ); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 7bcc41215..6e1964646 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1432,6 +1432,54 @@ run_test "Authentication: server badcert, client optional" \ -C "! 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 "! 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 "! 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 POLARSSL_SSL_SET_CURVES +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 POLARSSL_SSL_SET_CURVES +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" \ @@ -2420,7 +2468,7 @@ run_test "Small packet TLS 1.2 AEAD shorter tag" \ # A test for extensions in SSLv3 -requires_config_enabled MBEDTLS_SSL_PROTO_SSL3 +requires_config_enabled POLARSSL_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" \