From c94f7352fae9b0828b10b3570ad18653a6c87292 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 10 May 2017 16:37:56 +0200 Subject: [PATCH] Ignore failures when sending fatal alerts In many places in TLS handling, some code detects a fatal error, sends a fatal alert message, and returns to the caller. If sending the alert fails, then return the error that triggered the alert, rather than overriding the return status. This effectively causes alert sending failures to be ignored. Formerly the code was inconsistently sometimes doing one, sometimes the other. In general ignoring the alert is the right thing: what matters to the caller is the original error. A typical alert failure is that the connection is already closed. One case which remains not handled correctly is if the alert remains in the output buffer (WANT_WRITE). Then it won't be sent, or will be truncated. We'd need to either delay the application error or record the write buffering notice; to be done later. --- library/ssl_cli.c | 47 +++++++++++++++++++------------------- library/ssl_srv.c | 57 +++++++++++++++++------------------------------ library/ssl_tls.c | 11 ++------- 3 files changed, 46 insertions(+), 69 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index ddb1f9f81..d3169d2a4 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1057,8 +1057,6 @@ static int ssl_parse_renegotiation_info( mbedtls_ssl_context *ssl, const unsigned char *buf, size_t len ) { - int ret; - #if defined(MBEDTLS_SSL_RENEGOTIATION) if( ssl->renego_status != MBEDTLS_SSL_INITIAL_HANDSHAKE ) { @@ -1071,10 +1069,8 @@ static int ssl_parse_renegotiation_info( mbedtls_ssl_context *ssl, ssl->peer_verify_data, ssl->verify_data_len ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching renegotiation info" ) ); - - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } } @@ -1084,10 +1080,8 @@ static int ssl_parse_renegotiation_info( mbedtls_ssl_context *ssl, if( len != 1 || buf[0] != 0x00 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-zero length renegotiation info" ) ); - - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -1111,7 +1105,8 @@ static int ssl_parse_max_fragment_length_ext( mbedtls_ssl_context *ssl, buf[0] != ssl->conf->mfl_code ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching max fragment length extension" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -1128,7 +1123,8 @@ static int ssl_parse_truncated_hmac_ext( mbedtls_ssl_context *ssl, len != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching truncated HMAC extension" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -1150,7 +1146,8 @@ static int ssl_parse_encrypt_then_mac_ext( mbedtls_ssl_context *ssl, len != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching encrypt-then-MAC extension" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -1172,7 +1169,8 @@ static int ssl_parse_extended_ms_ext( mbedtls_ssl_context *ssl, len != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching extended master secret extension" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -1193,7 +1191,8 @@ static int ssl_parse_session_ticket_ext( mbedtls_ssl_context *ssl, len != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching session ticket extension" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -1244,7 +1243,8 @@ static int ssl_parse_supported_point_formats_ext( mbedtls_ssl_context *ssl, } MBEDTLS_SSL_DEBUG_MSG( 1, ( "no point format in common" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } #endif /* MBEDTLS_ECDH_C || MBEDTLS_ECDSA_C || @@ -1273,8 +1273,8 @@ static int ssl_parse_ecjpake_kkpp( mbedtls_ssl_context *ssl, buf, len ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ecjpake_read_round_one", ret ); - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( ret ); } @@ -1293,7 +1293,8 @@ static int ssl_parse_alpn_ext( mbedtls_ssl_context *ssl, if( ssl->conf->alpn_list == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching ALPN extension" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -1343,7 +1344,8 @@ static int ssl_parse_alpn_ext( mbedtls_ssl_context *ssl, } MBEDTLS_SSL_DEBUG_MSG( 1, ( "ALPN extension: no matching protocol" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } #endif /* MBEDTLS_SSL_ALPN */ @@ -1910,9 +1912,8 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) if( handshake_failure == 1 ) { - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 451370c6a..89afbce5a 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -152,8 +152,6 @@ static int ssl_parse_renegotiation_info( mbedtls_ssl_context *ssl, const unsigned char *buf, size_t len ) { - int ret; - #if defined(MBEDTLS_SSL_RENEGOTIATION) if( ssl->renego_status != MBEDTLS_SSL_INITIAL_HANDSHAKE ) { @@ -164,10 +162,8 @@ static int ssl_parse_renegotiation_info( mbedtls_ssl_context *ssl, ssl->verify_data_len ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-matching renegotiation info" ) ); - - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } } @@ -177,10 +173,8 @@ static int ssl_parse_renegotiation_info( mbedtls_ssl_context *ssl, if( len != 1 || buf[0] != 0x0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "non-zero length renegotiation info" ) ); - - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } @@ -857,10 +851,8 @@ static int ssl_parse_client_hello_v2( mbedtls_ssl_context *ssl ) if( ssl->renego_status != MBEDTLS_SSL_INITIAL_HANDSHAKE ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "client hello v2 illegal for renegotiation" ) ); - - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } #endif /* MBEDTLS_SSL_RENEGOTIATION */ @@ -1006,9 +998,8 @@ static int ssl_parse_client_hello_v2( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 1, ( "received RENEGOTIATION SCSV " "during renegotiation" ) ); - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } #endif /* MBEDTLS_SSL_RENEGOTIATION */ @@ -1092,10 +1083,8 @@ have_ciphersuite_v2: ssl->conf->allow_legacy_renegotiation == MBEDTLS_SSL_LEGACY_BREAK_HANDSHAKE ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "legacy renegotiation, breaking off handshake" ) ); - - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } @@ -1794,10 +1783,8 @@ read_record_header: if( ssl->renego_status == MBEDTLS_SSL_RENEGOTIATION_IN_PROGRESS ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "received RENEGOTIATION SCSV during renegotiation" ) ); - - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } #endif @@ -1841,9 +1828,8 @@ read_record_header: if( handshake_failure == 1 ) { - if( ( ret = mbedtls_ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) - return( ret ); - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } @@ -1881,13 +1867,15 @@ read_record_header: { MBEDTLS_SSL_DEBUG_MSG( 1, ( "got ciphersuites in common, " "but none of them usable" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_NO_USABLE_CIPHERSUITE ); } else { MBEDTLS_SSL_DEBUG_MSG( 1, ( "got no ciphersuites in common" ) ); - mbedtls_ssl_send_fatal_handshake_failure( ssl ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_HANDSHAKE_FAILURE ); return( MBEDTLS_ERR_SSL_NO_CIPHER_CHOSEN ); } @@ -3314,13 +3302,8 @@ static int ssl_parse_client_psk_identity( mbedtls_ssl_context *ssl, unsigned cha if( ret == MBEDTLS_ERR_SSL_UNKNOWN_IDENTITY ) { MBEDTLS_SSL_DEBUG_BUF( 3, "Unknown PSK identity", *p, n ); - if( ( ret = mbedtls_ssl_send_alert_message( ssl, - MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_UNKNOWN_PSK_IDENTITY ) ) != 0 ) - { - return( ret ); - } - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_UNKNOWN_PSK_IDENTITY ); return( MBEDTLS_ERR_SSL_UNKNOWN_IDENTITY ); } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 096b11148..713cb1055 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3473,7 +3473,6 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) */ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) { - int ret; int major_ver, minor_ver; MBEDTLS_SSL_DEBUG_BUF( 4, "input record header", ssl->in_hdr, mbedtls_ssl_hdr_len( ssl ) ); @@ -3494,14 +3493,8 @@ static int ssl_parse_record_header( mbedtls_ssl_context *ssl ) ssl->in_msgtype != MBEDTLS_SSL_MSG_APPLICATION_DATA ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "unknown record type" ) ); - - if( ( ret = mbedtls_ssl_send_alert_message( ssl, - MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ) ) != 0 ) - { - return( ret ); - } - + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_UNEXPECTED_MESSAGE ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); }