From 99000142cba168b410669631a654efc23117952a Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Thu, 13 Oct 2016 17:21:01 +0100 Subject: [PATCH] Merge fix for IE Certificate Compatibility --- ChangeLog | 3 + include/mbedtls/ssl.h | 2 + include/mbedtls/ssl_internal.h | 6 ++ library/error.c | 4 ++ library/ssl_cli.c | 9 +++ library/ssl_srv.c | 63 +++++++++++++------- library/ssl_tls.c | 106 +++++++++++++++++++++++++++++---- tests/ssl-opt.sh | 18 ++++++ 8 files changed, 178 insertions(+), 33 deletions(-) diff --git a/ChangeLog b/ChangeLog index ec9219bd7..8d99bf4f0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -48,6 +48,9 @@ Bugfix ssl_parse_hello_verify_request() for DTLS. Found by Guido Vranken. * Fix check for validity of date when parsing in mbedtls_x509_get_time(). Found by subramanyam-c. #626 + * Fix compatibility issue with Internet Explorer client authentication, + where the limited hash choices prevented the client from sending its + certificate. Found by teumas. #513 Changes * Extended test coverage of special cases, and added new timing test suite. diff --git a/include/mbedtls/ssl.h b/include/mbedtls/ssl.h index 1c0513da7..ba499d2bd 100644 --- a/include/mbedtls/ssl.h +++ b/include/mbedtls/ssl.h @@ -107,6 +107,8 @@ #define MBEDTLS_ERR_SSL_TIMEOUT -0x6800 /**< The operation timed out. */ #define MBEDTLS_ERR_SSL_CLIENT_RECONNECT -0x6780 /**< The client initiated a reconnect from the same port. */ #define MBEDTLS_ERR_SSL_UNEXPECTED_RECORD -0x6700 /**< Record header looks valid but is not expected. */ +#define MBEDTLS_ERR_SSL_NON_FATAL -0x6680 /**< The alert message received indicates a non-fatal error. */ +#define MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH -0x6600 /**< Couldn't set the hash for verifying CertificateVerify */ /* * Various constants diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index d63d7d4e7..668c0f567 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -355,6 +355,11 @@ int mbedtls_ssl_send_fatal_handshake_failure( mbedtls_ssl_context *ssl ); void mbedtls_ssl_reset_checksum( mbedtls_ssl_context *ssl ); int mbedtls_ssl_derive_keys( mbedtls_ssl_context *ssl ); +int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ); +int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ); +int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ); +void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ); + int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ); int mbedtls_ssl_fetch_input( mbedtls_ssl_context *ssl, size_t nb_want ); @@ -384,6 +389,7 @@ mbedtls_pk_type_t mbedtls_ssl_pk_alg_from_sig( unsigned char sig ); mbedtls_md_type_t mbedtls_ssl_md_alg_from_hash( unsigned char hash ); unsigned char mbedtls_ssl_hash_from_md_alg( int md ); +int mbedtls_ssl_set_calc_verify_md( mbedtls_ssl_context *ssl, int md ); #if defined(MBEDTLS_ECP_C) int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id ); diff --git a/library/error.c b/library/error.c index 71d4faa70..dd2db0c45 100644 --- a/library/error.c +++ b/library/error.c @@ -435,6 +435,10 @@ void mbedtls_strerror( int ret, char *buf, size_t buflen ) mbedtls_snprintf( buf, buflen, "SSL - The client initiated a reconnect from the same port" ); if( use_ret == -(MBEDTLS_ERR_SSL_UNEXPECTED_RECORD) ) mbedtls_snprintf( buf, buflen, "SSL - Record header looks valid but is not expected" ); + if( use_ret == -(MBEDTLS_ERR_SSL_NON_FATAL) ) + mbedtls_snprintf( buf, buflen, "SSL - The alert message received indicates a non-fatal error" ); + if( use_ret == -(MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH) ) + mbedtls_snprintf( buf, buflen, "SSL - Couldn't set the hash for verifying CertificateVerify" ); #endif /* MBEDTLS_SSL_TLS_C */ #if defined(MBEDTLS_X509_USE_C) || defined(MBEDTLS_X509_CREATE_C) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 39fcd6cb1..223823b3c 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2639,6 +2639,15 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) { size_t sig_alg_len = ( ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 1 + n] << 8 ) | ( buf[mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n] ) ); +#if defined(MBEDTLS_DEBUG_C) + unsigned char* sig_alg = buf + mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n; + size_t i; + + for( i = 0; i < sig_alg_len; i += 2 ) + { + MBEDTLS_SSL_DEBUG_MSG( 3, ( "Supported Signature Algorithm found: %d,%d", sig_alg[i], sig_alg[i + 1] ) ); + } +#endif n += 2 + sig_alg_len; diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 4b0f99714..fc0d2d7b4 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -1043,7 +1043,6 @@ have_ciphersuite_v2: ssl->session_negotiate->ciphersuite = ciphersuites[i]; ssl->transform_negotiate->ciphersuite_info = ciphersuite_info; - mbedtls_ssl_optimize_checksum( ssl, ssl->transform_negotiate->ciphersuite_info ); /* * SSLv2 Client Hello relevant renegotiation security checks @@ -1840,7 +1839,6 @@ have_ciphersuite: ssl->session_negotiate->ciphersuite = ciphersuites[i]; ssl->transform_negotiate->ciphersuite_info = ciphersuite_info; - mbedtls_ssl_optimize_checksum( ssl, ssl->transform_negotiate->ciphersuite_info ); ssl->state++; @@ -2556,29 +2554,27 @@ static int ssl_write_certificate_request( mbedtls_ssl_context *ssl ) */ if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 ) { - /* - * Only use current running hash algorithm that is already required - * for requested ciphersuite. - */ - ssl->handshake->verify_sig_alg = MBEDTLS_SSL_HASH_SHA256; - - if( ssl->transform_negotiate->ciphersuite_info->mac == - MBEDTLS_MD_SHA384 ) - { - ssl->handshake->verify_sig_alg = MBEDTLS_SSL_HASH_SHA384; - } + const int *cur; /* * Supported signature algorithms */ + for( cur = ssl->conf->sig_hashes; *cur != MBEDTLS_MD_NONE; cur++ ) + { + unsigned char hash = mbedtls_ssl_hash_from_md_alg( *cur ); + + if( MBEDTLS_SSL_HASH_NONE == hash || mbedtls_ssl_set_calc_verify_md( ssl, hash ) ) + continue; + #if defined(MBEDTLS_RSA_C) - p[2 + sa_len++] = ssl->handshake->verify_sig_alg; - p[2 + sa_len++] = MBEDTLS_SSL_SIG_RSA; + p[2 + sa_len++] = hash; + p[2 + sa_len++] = MBEDTLS_SSL_SIG_RSA; #endif #if defined(MBEDTLS_ECDSA_C) - p[2 + sa_len++] = ssl->handshake->verify_sig_alg; - p[2 + sa_len++] = MBEDTLS_SSL_SIG_ECDSA; + p[2 + sa_len++] = hash; + p[2 + sa_len++] = MBEDTLS_SSL_SIG_ECDSA; #endif + } p[0] = (unsigned char)( sa_len >> 8 ); p[1] = (unsigned char)( sa_len ); @@ -3581,17 +3577,28 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) return( 0 ); } - /* Needs to be done before read_record() to exclude current message */ - ssl->handshake->calc_verify( ssl, hash ); + /* Read the message without adding it to the checksum */ + do { - if( ( ret = mbedtls_ssl_read_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_read_record_layer( ssl ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); + return( ret ); + } + + ret = mbedtls_ssl_handle_message_type( ssl ); + + } while( MBEDTLS_ERR_SSL_NON_FATAL == ret ); + + if( 0 != ret ) { - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_read_record", ret ); + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_handle_message_type" ), ret ); return( ret ); } ssl->state++; + /* Process the message contents */ if( ssl->in_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || ssl->in_msg[0] != MBEDTLS_SSL_HS_CERTIFICATE_VERIFY ) { @@ -3638,14 +3645,19 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) /* * Hash */ - if( ssl->in_msg[i] != ssl->handshake->verify_sig_alg ) + md_alg = mbedtls_ssl_md_alg_from_hash( ssl->in_msg[i] ); + + if( md_alg == MBEDTLS_MD_NONE || mbedtls_ssl_set_calc_verify_md( ssl, ssl->in_msg[i] ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "peer not adhering to requested sig_alg" " for verify message" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_VERIFY ); } - md_alg = mbedtls_ssl_md_alg_from_hash( ssl->handshake->verify_sig_alg ); +#if !defined(MBEDTLS_MD_SHA1) + if( MBEDTLS_MD_SHA1 == md_alg ) + hash_start += 16; +#endif /* Info from md_alg will be used instead */ hashlen = 0; @@ -3696,6 +3708,9 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_VERIFY ); } + /* Calculate hash and verify signature */ + ssl->handshake->calc_verify( ssl, hash ); + if( ( ret = mbedtls_pk_verify( &ssl->session_negotiate->peer_cert->pk, md_alg, hash_start, hashlen, ssl->in_msg + i, sig_len ) ) != 0 ) @@ -3704,6 +3719,8 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) return( ret ); } + mbedtls_ssl_update_handshake_status( ssl ); + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= parse certificate verify" ) ); return( ret ); diff --git a/library/ssl_tls.c b/library/ssl_tls.c index df7b73495..52723187b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3082,7 +3082,7 @@ static int ssl_reassemble_dtls_handshake( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_PROTO_DTLS */ -static int ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) +int mbedtls_ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) { if( ssl->in_msglen < mbedtls_ssl_hs_hdr_len( ssl ) ) { @@ -3164,6 +3164,12 @@ static int ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); } + return( 0 ); +} + +void mbedtls_ssl_update_handshake_status( mbedtls_ssl_context *ssl ) +{ + if( ssl->state != MBEDTLS_SSL_HANDSHAKE_OVER && ssl->handshake != NULL ) { @@ -3178,8 +3184,6 @@ static int ssl_prepare_handshake_record( mbedtls_ssl_context *ssl ) ssl->handshake->in_msg_seq++; } #endif - - return( 0 ); } /* @@ -3735,6 +3739,38 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "=> read record" ) ); + do { + + if( ( ret = mbedtls_ssl_read_record_layer( ssl ) ) != 0 ) + { + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_read_record_layer" ), ret ); + return( ret ); + } + + ret = mbedtls_ssl_handle_message_type( ssl ); + + } while( MBEDTLS_ERR_SSL_NON_FATAL == ret ); + + if( 0 != ret ) + { + MBEDTLS_SSL_DEBUG_RET( 1, ( "mbedtls_ssl_handle_message_type" ), ret ); + return( ret ); + } + + if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) + { + mbedtls_ssl_update_handshake_status( ssl ); + } + + MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= read record" ) ); + + return( 0 ); +} + +int mbedtls_ssl_read_record_layer( mbedtls_ssl_context *ssl ) +{ + int ret; + if( ssl->in_hslen != 0 && ssl->in_hslen < ssl->in_msglen ) { /* @@ -3748,9 +3784,6 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_BUF( 4, "remaining content in record", ssl->in_msg, ssl->in_msglen ); - if( ( ret = ssl_prepare_handshake_record( ssl ) ) != 0 ) - return( ret ); - return( 0 ); } @@ -3759,7 +3792,10 @@ int mbedtls_ssl_read_record( mbedtls_ssl_context *ssl ) /* * Read the record header and parse it */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) read_record_header: +#endif + if( ( ret = mbedtls_ssl_fetch_input( ssl, mbedtls_ssl_hdr_len( ssl ) ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ssl_fetch_input", ret ); @@ -3913,13 +3949,22 @@ read_record_header: } #endif + return( 0 ); +} + +int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ) +{ + int ret; + /* * Handle particular types of records */ if( ssl->in_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE ) { - if( ( ret = ssl_prepare_handshake_record( ssl ) ) != 0 ) + if( ( ret = mbedtls_ssl_prepare_handshake_record( ssl ) ) != 0 ) + { return( ret ); + } } if( ssl->in_msgtype == MBEDTLS_SSL_MSG_ALERT ) @@ -3967,11 +4012,9 @@ read_record_header: #endif /* MBEDTLS_SSL_PROTO_SSL3 && MBEDTLS_SSL_SRV_C */ /* Silently ignore: fetch new message */ - goto read_record_header; + return MBEDTLS_ERR_SSL_NON_FATAL; } - MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= read record" ) ); - return( 0 ); } @@ -7602,4 +7645,47 @@ void mbedtls_ssl_read_version( int *major, int *minor, int transport, } } +int mbedtls_ssl_set_calc_verify_md( mbedtls_ssl_context *ssl, int md ) +{ +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) + if( ssl->minor_ver != MBEDTLS_SSL_MINOR_VERSION_3 ) + return MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH; + + switch( md ) + { +#if defined(MBEDTLS_SSL_PROTO_TLS1) || defined(MBEDTLS_SSL_PROTO_TLS1_1) +#if defined(MBEDTLS_MD5_C) + case MBEDTLS_SSL_HASH_MD5: + ssl->handshake->calc_verify = ssl_calc_verify_tls; + break; +#endif +#if defined(MBEDTLS_SHA1_C) + case MBEDTLS_SSL_HASH_SHA1: + ssl->handshake->calc_verify = ssl_calc_verify_tls; + break; +#endif +#endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 */ +#if defined(MBEDTLS_SHA512_C) + case MBEDTLS_SSL_HASH_SHA384: + ssl->handshake->calc_verify = ssl_calc_verify_tls_sha384; + break; +#endif +#if defined(MBEDTLS_SHA256_C) + case MBEDTLS_SSL_HASH_SHA256: + ssl->handshake->calc_verify = ssl_calc_verify_tls_sha256; + break; +#endif + default: + return MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH; + } + + return 0; +#else /* !MBEDTLS_SSL_PROTO_TLS1_2 */ + (void) ssl; + (void) md; + + return MBEDTLS_ERR_SSL_INVALID_VERIFY_HASH; +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ +} + #endif /* MBEDTLS_SSL_TLS_C */ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 429d9cd19..09a947e65 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1725,6 +1725,24 @@ run_test "Authentication: server badcert, client none" \ -C "! mbedtls_ssl_handshake returned" \ -C "X509 - Certificate verification failed" +run_test "Authentication: client SHA256, server required" \ + "$P_SRV auth_mode=required" \ + "$P_CLI debug_level=3 crt_file=data_files/server6.crt \ + key_file=data_files/server6.key \ + force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-256-GCM-SHA384" \ + 0 \ + -c "Supported Signature Algorithm found: 4," \ + -c "Supported Signature Algorithm found: 5," + +run_test "Authentication: client SHA384, server required" \ + "$P_SRV auth_mode=required" \ + "$P_CLI debug_level=3 crt_file=data_files/server6.crt \ + key_file=data_files/server6.key \ + force_ciphersuite=TLS-ECDHE-ECDSA-WITH-AES-128-GCM-SHA256" \ + 0 \ + -c "Supported Signature Algorithm found: 4," \ + -c "Supported Signature Algorithm found: 5," + run_test "Authentication: client badcert, server required" \ "$P_SRV debug_level=3 auth_mode=required" \ "$P_CLI debug_level=3 crt_file=data_files/server5-badsign.crt \