From 846ba473af0af5cc4aafdc23ed1862a9d1df4da3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 8 Jan 2015 13:54:38 +0100 Subject: [PATCH 1/5] Minor refactoring --- library/ssl_srv.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 01b0aca20..a385c8939 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -809,14 +809,13 @@ static int ssl_pick_cert( ssl_context *ssl, } #if defined(POLARSSL_ECDSA_C) - if( pk_alg == POLARSSL_PK_ECDSA ) - { - if( ssl_key_matches_curves( cur->key, ssl->handshake->curves ) ) - break; - } - else + if( pk_alg == POLARSSL_PK_ECDSA && + ! ssl_key_matches_curves( cur->key, ssl->handshake->curves ) ) + continue; #endif - break; + + /* If we get there, we got a winner */ + break; } if( cur == NULL ) From 6458e3b743ec7c15fad483d1b643709627a4bb2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 8 Jan 2015 14:16:56 +0100 Subject: [PATCH 2/5] Some more refactoring/tuning. --- library/ssl_srv.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index a385c8939..211ab5d47 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -749,11 +749,11 @@ static int ssl_parse_alpn_ext( ssl_context *ssl, #if defined(POLARSSL_X509_CRT_PARSE_C) /* - * Return 1 if the given EC key uses the given curve, 0 otherwise + * Return 0 if the given key uses one of the acceptable curves, -1 otherwise */ #if defined(POLARSSL_ECDSA_C) -static int ssl_key_matches_curves( pk_context *pk, - const ecp_curve_info **curves ) +static int ssl_check_key_curve( pk_context *pk, + const ecp_curve_info **curves ) { const ecp_curve_info **crv = curves; ecp_group_id grp_id = pk_ec( *pk )->grp.id; @@ -761,11 +761,11 @@ static int ssl_key_matches_curves( pk_context *pk, while( *crv != NULL ) { if( (*crv)->grp_id == grp_id ) - return( 1 ); + return( 0 ); crv++; } - return( 0 ); + return( -1 ); } #endif /* POLARSSL_ECDSA_C */ @@ -810,7 +810,7 @@ static int ssl_pick_cert( ssl_context *ssl, #if defined(POLARSSL_ECDSA_C) if( pk_alg == POLARSSL_PK_ECDSA && - ! ssl_key_matches_curves( cur->key, ssl->handshake->curves ) ) + ssl_check_key_curve( cur->key, ssl->handshake->curves ) != 0 ) continue; #endif @@ -838,8 +838,8 @@ static int ssl_ciphersuite_match( ssl_context *ssl, int suite_id, suite_info = ssl_ciphersuite_from_id( suite_id ); if( suite_info == NULL ) { - SSL_DEBUG_MSG( 1, ( "ciphersuite info for %04x not found", suite_id ) ); - return( POLARSSL_ERR_SSL_BAD_INPUT_DATA ); + SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); } if( suite_info->min_minor_ver > ssl->minor_ver || From df331a55d2d71a9e1aed123beaf374943f97bcc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 8 Jan 2015 16:43:07 +0100 Subject: [PATCH 3/5] Prefer SHA-1 certificates for pre-1.2 clients --- ChangeLog | 6 ++++++ library/ssl_srv.c | 31 ++++++++++++++++++++++----- tests/ssl-opt.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index fd83b9e60..b38d56f27 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ PolarSSL ChangeLog (Sorted per branch, date) += PolarSSL 1.3.10 released ??? + +Features + * Certificate selection based on signature hash, prefering SHA-1 over SHA-2 + for pre-1.2 clients when multiple certificates are available. + = PolarSSL 1.3.9 released 2014-10-20 Security * Lowest common hash was selected from signature_algorithms extension in diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 211ab5d47..04608ee98 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -776,7 +776,7 @@ static int ssl_check_key_curve( pk_context *pk, static int ssl_pick_cert( ssl_context *ssl, const ssl_ciphersuite_t * ciphersuite_info ) { - ssl_key_cert *cur, *list; + ssl_key_cert *cur, *list, *fallback = NULL; pk_type_t pk_alg = ssl_get_ciphersuite_sig_pk_alg( ciphersuite_info ); #if defined(POLARSSL_SSL_SERVER_NAME_INDICATION) @@ -814,15 +814,36 @@ static int ssl_pick_cert( ssl_context *ssl, continue; #endif + /* + * Try to select a SHA-1 certificate for pre-1.2 clients, but still + * present them a SHA-higher cert rather than failing if it's the only + * one we got that satisfies the other conditions. + */ + if( ssl->minor_ver < SSL_MINOR_VERSION_3 && + cur->cert->sig_md != POLARSSL_MD_SHA1 ) + { + if( fallback == NULL ) + fallback = cur; + continue; + } + /* If we get there, we got a winner */ break; } - if( cur == NULL ) - return( -1 ); + if( cur != NULL ) + { + ssl->handshake->key_cert = cur; + return( 0 ); + } - ssl->handshake->key_cert = cur; - return( 0 ); + if( fallback != NULL ) + { + ssl->handshake->key_cert = fallback; + return( 0 ); + } + + return( -1 ); } #endif /* POLARSSL_X509_CRT_PARSE_C */ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index f94808d2c..2cf4b6e70 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -956,6 +956,60 @@ run_test "Authentication: client no cert, ssl3" \ -C "! ssl_handshake returned" \ -S "X509 - Certificate verification failed" +# Tests for certificate selection based on SHA verson + +run_test "Certificate hash: client TLS 1.2 -> SHA-2" \ + "$P_SRV crt_file=data_files/server5.crt \ + key_file=data_files/server5.key \ + crt_file2=data_files/server5-sha1.crt \ + key_file2=data_files/server5.key" \ + "$P_CLI force_version=tls1_2" \ + 0 \ + -c "signed using.*ECDSA with SHA256" \ + -C "signed using.*ECDSA with SHA1" + +run_test "Certificate hash: client TLS 1.1 -> SHA-1" \ + "$P_SRV crt_file=data_files/server5.crt \ + key_file=data_files/server5.key \ + crt_file2=data_files/server5-sha1.crt \ + key_file2=data_files/server5.key" \ + "$P_CLI force_version=tls1_1" \ + 0 \ + -C "signed using.*ECDSA with SHA256" \ + -c "signed using.*ECDSA with SHA1" + +run_test "Certificate hash: client TLS 1.0 -> SHA-1" \ + "$P_SRV crt_file=data_files/server5.crt \ + key_file=data_files/server5.key \ + crt_file2=data_files/server5-sha1.crt \ + key_file2=data_files/server5.key" \ + "$P_CLI force_version=tls1" \ + 0 \ + -C "signed using.*ECDSA with SHA256" \ + -c "signed using.*ECDSA with SHA1" + +run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 1)" \ + "$P_SRV crt_file=data_files/server5.crt \ + key_file=data_files/server5.key \ + crt_file2=data_files/server6.crt \ + key_file2=data_files/server6.key" \ + "$P_CLI force_version=tls1_1" \ + 0 \ + -c "serial number.*09" \ + -c "signed using.*ECDSA with SHA256" \ + -C "signed using.*ECDSA with SHA1" + +run_test "Certificate hash: client TLS 1.1, no SHA-1 -> SHA-2 (order 2)" \ + "$P_SRV crt_file=data_files/server6.crt \ + key_file=data_files/server6.key \ + crt_file2=data_files/server5.crt \ + key_file2=data_files/server5.key" \ + "$P_CLI force_version=tls1_1" \ + 0 \ + -c "serial number.*0A" \ + -c "signed using.*ECDSA with SHA256" \ + -C "signed using.*ECDSA with SHA1" + # tests for SNI run_test "SNI: no SNI callback" \ From f01768c55e60d8fa7366e8e7c6f5c71f327b1e6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 8 Jan 2015 17:06:16 +0100 Subject: [PATCH 4/5] Specific error for suites in common but none good --- ChangeLog | 5 +++++ include/polarssl/error.h | 2 +- include/polarssl/ssl.h | 1 + library/error.c | 2 ++ library/ssl_srv.c | 43 ++++++++++++++++++++++++++++++---------- tests/ssl-opt.sh | 14 ++++++------- 6 files changed, 48 insertions(+), 19 deletions(-) diff --git a/ChangeLog b/ChangeLog index b38d56f27..efaadfb42 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,6 +6,11 @@ Features * Certificate selection based on signature hash, prefering SHA-1 over SHA-2 for pre-1.2 clients when multiple certificates are available. +Changes + * A specific error is now returned when there are ciphersuites in common + but none of them is usable due to external factors such as no certificate + with a suitable (extended)KeyUsage or curvem or no PSK set. + = PolarSSL 1.3.9 released 2014-10-20 Security * Lowest common hash was selected from signature_algorithms extension in diff --git a/include/polarssl/error.h b/include/polarssl/error.h index 7ce2828b2..6a1143f46 100644 --- a/include/polarssl/error.h +++ b/include/polarssl/error.h @@ -91,7 +91,7 @@ * ECP 4 8 (Started from top) * MD 5 4 * CIPHER 6 6 - * SSL 6 10 (Started from top) + * SSL 6 11 (Started from top) * SSL 7 31 * * Module dependent error code (5 bits 0x.00.-0x.F8.) diff --git a/include/polarssl/ssl.h b/include/polarssl/ssl.h index 194e94471..1e9fb39c7 100644 --- a/include/polarssl/ssl.h +++ b/include/polarssl/ssl.h @@ -146,6 +146,7 @@ #define POLARSSL_ERR_SSL_INTERNAL_ERROR -0x6C00 /**< Internal error (eg, unexpected failure in lower-level module) */ #define POLARSSL_ERR_SSL_COUNTER_WRAPPING -0x6B80 /**< A counter would wrap (eg, too many messages exchanged). */ #define POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO -0x6B00 /**< Unexpected message at ServerHello in renegotiation. */ +#define POLARSSL_ERR_SSL_NO_USABLE_CIPHERSUITE -0x6A80 /**< None of the common ciphersuites is usable (eg, no suitable certificate) */ /* * Various constants diff --git a/library/error.c b/library/error.c index 73504d450..00956360b 100644 --- a/library/error.c +++ b/library/error.c @@ -452,6 +452,8 @@ void polarssl_strerror( int ret, char *buf, size_t buflen ) snprintf( buf, buflen, "SSL - A counter would wrap (eg, too many messages exchanged)" ); if( use_ret == -(POLARSSL_ERR_SSL_WAITING_SERVER_HELLO_RENEGO) ) snprintf( buf, buflen, "SSL - Unexpected message at ServerHello in renegotiation" ); + if( use_ret == -(POLARSSL_ERR_SSL_NO_USABLE_CIPHERSUITE) ) + snprintf( buf, buflen, "SSL - None of the common ciphersuites is usable (eg, no suitable certificate)" ); #endif /* POLARSSL_SSL_TLS_C */ #if defined(POLARSSL_X509_USE_C) || defined(POLARSSL_X509_CREATE_C) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 04608ee98..384207a39 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -903,7 +903,7 @@ static int ssl_ciphersuite_match( ssl_context *ssl, int suite_id, #if defined(POLARSSL_SSL_SRV_SUPPORT_SSLV2_CLIENT_HELLO) static int ssl_parse_client_hello_v2( ssl_context *ssl ) { - int ret; + int ret, got_common_suite; unsigned int i, j; size_t n; unsigned int ciph_len, sess_len, chal_len; @@ -1072,6 +1072,7 @@ static int ssl_parse_client_hello_v2( ssl_context *ssl ) } } + got_common_suite = 0; ciphersuites = ssl->ciphersuite_list[ssl->minor_ver]; ciphersuite_info = NULL; #if defined(POLARSSL_SSL_SRV_RESPECT_CLIENT_PREFERENCE) @@ -1089,6 +1090,8 @@ static int ssl_parse_client_hello_v2( ssl_context *ssl ) p[2] != ( ( ciphersuites[i] ) & 0xFF ) ) continue; + got_common_suite = 1; + if( ( ret = ssl_ciphersuite_match( ssl, ciphersuites[i], &ciphersuite_info ) ) != 0 ) return( ret ); @@ -1098,9 +1101,17 @@ static int ssl_parse_client_hello_v2( ssl_context *ssl ) } } - SSL_DEBUG_MSG( 1, ( "got no ciphersuites in common" ) ); - - return( POLARSSL_ERR_SSL_NO_CIPHER_CHOSEN ); + if( got_common_suite ) + { + SSL_DEBUG_MSG( 1, ( "got ciphersuites in common, " + "but none of them usable" ) ); + return( POLARSSL_ERR_SSL_NO_USABLE_CIPHERSUITE ); + } + else + { + SSL_DEBUG_MSG( 1, ( "got no ciphersuites in common" ) ); + return( POLARSSL_ERR_SSL_NO_CIPHER_CHOSEN ); + } have_ciphersuite_v2: ssl->session_negotiate->ciphersuite = ciphersuites[i]; @@ -1132,7 +1143,7 @@ have_ciphersuite_v2: static int ssl_parse_client_hello( ssl_context *ssl ) { - int ret; + int ret, got_common_suite; unsigned int i, j; size_t n; unsigned int ciph_len, sess_len; @@ -1552,6 +1563,7 @@ static int ssl_parse_client_hello( ssl_context *ssl ) * (At the end because we need information from the EC-based extensions * and certificate from the SNI callback triggered by the SNI extension.) */ + got_common_suite = 0; ciphersuites = ssl->ciphersuite_list[ssl->minor_ver]; ciphersuite_info = NULL; #if defined(POLARSSL_SSL_SRV_RESPECT_CLIENT_PREFERENCE) @@ -1568,6 +1580,8 @@ static int ssl_parse_client_hello( ssl_context *ssl ) p[1] != ( ( ciphersuites[i] ) & 0xFF ) ) continue; + got_common_suite = 1; + if( ( ret = ssl_ciphersuite_match( ssl, ciphersuites[i], &ciphersuite_info ) ) != 0 ) return( ret ); @@ -1577,12 +1591,19 @@ static int ssl_parse_client_hello( ssl_context *ssl ) } } - SSL_DEBUG_MSG( 1, ( "got no ciphersuites in common" ) ); - - if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - - return( POLARSSL_ERR_SSL_NO_CIPHER_CHOSEN ); + if( got_common_suite ) + { + SSL_DEBUG_MSG( 1, ( "got ciphersuites in common, " + "but none of them usable" ) ); + ssl_send_fatal_handshake_failure( ssl ); + return( POLARSSL_ERR_SSL_NO_USABLE_CIPHERSUITE ); + } + else + { + SSL_DEBUG_MSG( 1, ( "got no ciphersuites in common" ) ); + ssl_send_fatal_handshake_failure( ssl ); + return( POLARSSL_ERR_SSL_NO_CIPHER_CHOSEN ); + } have_ciphersuite: ssl->session_negotiate->ciphersuite = ciphersuites[i]; diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 2cf4b6e70..a37ea9383 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1562,7 +1562,7 @@ run_test "PSK callback: psk, no callback" \ "$P_CLI force_ciphersuite=TLS-PSK-WITH-AES-128-CBC-SHA \ psk_identity=foo psk=abc123" \ 0 \ - -S "SSL - The server has no ciphersuites in common" \ + -S "SSL - None of the common ciphersuites is usable" \ -S "SSL - Unknown identity received" \ -S "SSL - Verification of the message MAC failed" @@ -1571,7 +1571,7 @@ run_test "PSK callback: no psk, no callback" \ "$P_CLI force_ciphersuite=TLS-PSK-WITH-AES-128-CBC-SHA \ psk_identity=foo psk=abc123" \ 1 \ - -s "SSL - The server has no ciphersuites in common" \ + -s "SSL - None of the common ciphersuites is usable" \ -S "SSL - Unknown identity received" \ -S "SSL - Verification of the message MAC failed" @@ -1580,7 +1580,7 @@ run_test "PSK callback: callback overrides other settings" \ "$P_CLI force_ciphersuite=TLS-PSK-WITH-AES-128-CBC-SHA \ psk_identity=foo psk=abc123" \ 1 \ - -S "SSL - The server has no ciphersuites in common" \ + -S "SSL - None of the common ciphersuites is usable" \ -s "SSL - Unknown identity received" \ -S "SSL - Verification of the message MAC failed" @@ -1589,7 +1589,7 @@ run_test "PSK callback: first id matches" \ "$P_CLI force_ciphersuite=TLS-PSK-WITH-AES-128-CBC-SHA \ psk_identity=abc psk=dead" \ 0 \ - -S "SSL - The server has no ciphersuites in common" \ + -S "SSL - None of the common ciphersuites is usable" \ -S "SSL - Unknown identity received" \ -S "SSL - Verification of the message MAC failed" @@ -1598,7 +1598,7 @@ run_test "PSK callback: second id matches" \ "$P_CLI force_ciphersuite=TLS-PSK-WITH-AES-128-CBC-SHA \ psk_identity=def psk=beef" \ 0 \ - -S "SSL - The server has no ciphersuites in common" \ + -S "SSL - None of the common ciphersuites is usable" \ -S "SSL - Unknown identity received" \ -S "SSL - Verification of the message MAC failed" @@ -1607,7 +1607,7 @@ run_test "PSK callback: no match" \ "$P_CLI force_ciphersuite=TLS-PSK-WITH-AES-128-CBC-SHA \ psk_identity=ghi psk=beef" \ 1 \ - -S "SSL - The server has no ciphersuites in common" \ + -S "SSL - None of the common ciphersuites is usable" \ -s "SSL - Unknown identity received" \ -S "SSL - Verification of the message MAC failed" @@ -1616,7 +1616,7 @@ run_test "PSK callback: wrong key" \ "$P_CLI force_ciphersuite=TLS-PSK-WITH-AES-128-CBC-SHA \ psk_identity=abc psk=beef" \ 1 \ - -S "SSL - The server has no ciphersuites in common" \ + -S "SSL - None of the common ciphersuites is usable" \ -S "SSL - Unknown identity received" \ -s "SSL - Verification of the message MAC failed" From 6f303ce19e667355c0d8af1081d61d796391b1d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 8 Jan 2015 17:07:18 +0100 Subject: [PATCH 5/5] Fix portability issue in script If there was a reason for hardcoding the path, it should have been documented. --- scripts/generate_errors.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/generate_errors.pl b/scripts/generate_errors.pl index 0ee992d45..b9a8e9c0f 100755 --- a/scripts/generate_errors.pl +++ b/scripts/generate_errors.pl @@ -46,7 +46,7 @@ close(FORMAT_FILE); $/ = $line_separator; -open(GREP, "/bin/grep \"define POLARSSL_ERR_\" $include_dir/* |") || die("Failure when calling grep: $!"); +open(GREP, "grep \"define POLARSSL_ERR_\" $include_dir/* |") || die("Failure when calling grep: $!"); my $ll_old_define = ""; my $hl_old_define = "";