From d0f6fa7bdc1a2c6539165ed044b9c85a109afaf1 Mon Sep 17 00:00:00 2001 From: Paul Bakker Date: Mon, 17 Sep 2012 09:18:12 +0000 Subject: [PATCH] - Sending of handshake_failures during renegotiation added - Handle two legacy modes differently: SSL_LEGACY_BREAK_HANDSHAKE and SSL_LEGACY_NO_RENEGOTIATION --- include/polarssl/ssl.h | 30 ++++++++++++++---- library/ssl_cli.c | 50 ++++++++++++++++++++++++------ library/ssl_srv.c | 63 ++++++++++++++++++++++++++------------ library/ssl_tls.c | 37 +++++++++++++++++++--- programs/ssl/ssl_client2.c | 2 +- 5 files changed, 141 insertions(+), 41 deletions(-) diff --git a/include/polarssl/ssl.h b/include/polarssl/ssl.h index 3d6e42c80..dc2619ad0 100644 --- a/include/polarssl/ssl.h +++ b/include/polarssl/ssl.h @@ -122,8 +122,9 @@ #define SSL_RENEGOTIATION_ENABLED 0 #define SSL_RENEGOTIATION_DISABLED 1 -#define SSL_NO_LEGACY_RENEGOTIATION 0 -#define SSL_ALLOW_LEGACY_RENEGOTIATION 1 +#define SSL_LEGACY_NO_RENEGOTIATION 0 +#define SSL_LEGACY_ALLOW_RENEGOTIATION 1 +#define SSL_LEGACY_BREAK_HANDSHAKE 2 #define SSL_MAX_CONTENT_LEN 16384 @@ -758,10 +759,25 @@ void ssl_set_renegotiation( ssl_context *ssl, int renegotiation ); /** * \brief Prevent or allow legacy renegotiation. - * (Default: SSL_NO_LEGACY_RENEGOTIATION) - * Allowing legacy renegotiation makes the connection - * vulnerable to specific man in the middle attacks. - * (See RFC 5746) + * (Default: SSL_LEGACY_NO_RENEGOTIATION) + * + * SSL_LEGACY_NO_RENEGOTIATION allows connections to + * be established even if the peer does not support + * secure renegotiation, but does not allow renegotiation + * to take place if not secure. + * (Interoperable and secure option) + * + * SSL_LEGACY_ALLOW_RENEGOTIATION allows renegotiations + * with non-upgraded peers. Allowing legacy renegotiation + * makes the connection vulnerable to specific man in the + * middle attacks. (See RFC 5746) + * (Most interoperable and least secure option) + * + * SSL_LEGACY_BREAK_HANDSHAKE breaks off connections + * if peer does not support secure renegotiation. Results + * in interoperability issues with non-upgraded peers + * that do not support renegotiation altogether. + * (Most secure option, interoperability issues) * * \param ssl SSL context * \param allow_legacy Prevent or allow (SSL_NO_LEGACY_RENEGOTIATION or @@ -914,6 +930,8 @@ int ssl_handshake_client( ssl_context *ssl ); int ssl_handshake_server( ssl_context *ssl ); void ssl_handshake_wrapup( ssl_context *ssl ); +int ssl_send_fatal_handshake_failure( ssl_context *ssl ); + int ssl_derive_keys( ssl_context *ssl ); int ssl_read_record( ssl_context *ssl ); diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 9b821100b..3d5008b6b 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -328,12 +328,17 @@ static int ssl_parse_renegotiation_info( ssl_context *ssl, unsigned char *buf, size_t len ) { + int ret; + if( ssl->renegotiation == SSL_INITIAL_HANDSHAKE ) { if( len != 1 || buf[0] != 0x0 ) { SSL_DEBUG_MSG( 1, ( "non-zero length renegotiated connection field" ) ); - /* TODO: Send handshake failure alert */ + + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + return( POLARSSL_ERR_SSL_BAD_HS_SERVER_HELLO ); } @@ -348,7 +353,10 @@ static int ssl_parse_renegotiation_info( ssl_context *ssl, ssl->peer_verify_data, ssl->verify_data_len ) != 0 ) { SSL_DEBUG_MSG( 1, ( "non-matching renegotiated connection field" ) ); - /* TODO: Send handshake failure alert */ + + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + return( POLARSSL_ERR_SSL_BAD_HS_SERVER_HELLO ); } } @@ -366,6 +374,7 @@ static int ssl_parse_server_hello( ssl_context *ssl ) size_t ext_len = 0; unsigned char *buf, *ext; int renegotiation_info_seen = 0; + int handshake_failure = 0; SSL_DEBUG_MSG( 2, ( "=> parse server hello" ) ); @@ -563,18 +572,39 @@ static int ssl_parse_server_hello( ssl_context *ssl ) /* * Renegotiation security checks */ - if( ssl->renegotiation == SSL_RENEGOTIATION && - ssl->secure_renegotiation == SSL_SECURE_RENEGOTIATION && - renegotiation_info_seen == 0 ) + if( ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && + ssl->allow_legacy_renegotiation == SSL_LEGACY_BREAK_HANDSHAKE ) { - SSL_DEBUG_MSG( 1, ( "renegotiation_info extension missing" ) ); - return( POLARSSL_ERR_SSL_BAD_HS_SERVER_HELLO ); + SSL_DEBUG_MSG( 1, ( "legacy renegotiation, breaking off handshake" ) ); + handshake_failure = 1; + } + else if( ssl->renegotiation == SSL_RENEGOTIATION && + ssl->secure_renegotiation == SSL_SECURE_RENEGOTIATION && + renegotiation_info_seen == 0 ) + { + SSL_DEBUG_MSG( 1, ( "renegotiation_info extension missing (secure)" ) ); + handshake_failure = 1; } - - if( !ssl->allow_legacy_renegotiation && - ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION ) + else if( ssl->renegotiation == SSL_RENEGOTIATION && + ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && + ssl->allow_legacy_renegotiation == SSL_LEGACY_NO_RENEGOTIATION ) { SSL_DEBUG_MSG( 1, ( "legacy renegotiation not allowed" ) ); + handshake_failure = 1; + } + else if( ssl->renegotiation == SSL_RENEGOTIATION && + ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && + renegotiation_info_seen == 1 ) + { + SSL_DEBUG_MSG( 1, ( "renegotiation_info extension present (legacy)" ) ); + handshake_failure = 1; + } + + if( handshake_failure == 1 ) + { + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + return( POLARSSL_ERR_SSL_BAD_HS_SERVER_HELLO ); } diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 742b9126e..209e5bdd6 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -42,12 +42,17 @@ static int ssl_parse_renegotiation_info( ssl_context *ssl, unsigned char *buf, size_t len ) { + int ret; + if( ssl->renegotiation == SSL_INITIAL_HANDSHAKE ) { if( len != 1 || buf[0] != 0x0 ) { SSL_DEBUG_MSG( 1, ( "non-zero length renegotiated connection field" ) ); - /* TODO: Send handshake failure alert */ + + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_HELLO ); } @@ -60,7 +65,10 @@ static int ssl_parse_renegotiation_info( ssl_context *ssl, memcmp( buf + 1, ssl->peer_verify_data, ssl->verify_data_len ) != 0 ) { SSL_DEBUG_MSG( 1, ( "non-matching renegotiated connection field" ) ); - /* TODO: Send handshake failure alert */ + + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_HELLO ); } } @@ -77,7 +85,8 @@ static int ssl_parse_client_hello( ssl_context *ssl ) unsigned int comp_len; unsigned int ext_len = 0; unsigned char *buf, *p, *ext; - int renegotiation_info_seen; + int renegotiation_info_seen = 0; + int handshake_failure = 0; SSL_DEBUG_MSG( 2, ( "=> parse client hello" ) ); @@ -277,6 +286,10 @@ static int ssl_parse_client_hello( ssl_context *ssl ) if( ssl->renegotiation == SSL_RENEGOTIATION ) { SSL_DEBUG_MSG( 1, ( "received RENEGOTIATION SCSV during renegotiation" ) ); + + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_HELLO ); } ssl->secure_renegotiation = SSL_SECURE_RENEGOTIATION; @@ -306,7 +319,6 @@ have_ciphersuite: ssl_optimize_checksum( ssl, ssl->session_negotiate->ciphersuite ); ext = buf + 44 + sess_len + ciph_len + comp_len; - renegotiation_info_seen = 0; while( ext_len ) { @@ -349,26 +361,39 @@ have_ciphersuite: /* * Renegotiation security checks */ - if( ssl->renegotiation == SSL_RENEGOTIATION && - ssl->secure_renegotiation == SSL_SECURE_RENEGOTIATION && - renegotiation_info_seen == 0 ) + if( ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && + ssl->allow_legacy_renegotiation == SSL_LEGACY_BREAK_HANDSHAKE ) + { + SSL_DEBUG_MSG( 1, ( "legacy renegotiation, breaking off handshake" ) ); + handshake_failure = 1; + } + else if( ssl->renegotiation == SSL_RENEGOTIATION && + ssl->secure_renegotiation == SSL_SECURE_RENEGOTIATION && + renegotiation_info_seen == 0 ) { SSL_DEBUG_MSG( 1, ( "renegotiation_info extension missing (secure)" ) ); - return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_HELLO ); + handshake_failure = 1; } - - if( ssl->renegotiation == SSL_RENEGOTIATION && - ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && - renegotiation_info_seen == 1 ) - { - SSL_DEBUG_MSG( 1, ( "renegotiation_info extension present (legacy)" ) ); - return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_HELLO ); - } - - if( !ssl->allow_legacy_renegotiation && - ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION ) + else if( ssl->renegotiation == SSL_RENEGOTIATION && + ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && + ssl->allow_legacy_renegotiation == SSL_LEGACY_NO_RENEGOTIATION ) { SSL_DEBUG_MSG( 1, ( "legacy renegotiation not allowed" ) ); + handshake_failure = 1; + } + else if( ssl->renegotiation == SSL_RENEGOTIATION && + ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && + renegotiation_info_seen == 1 ) + { + SSL_DEBUG_MSG( 1, ( "renegotiation_info extension present (legacy)" ) ); + handshake_failure = 1; + } + + if( handshake_failure == 1 ) + { + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_HELLO ); } diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d47ec9f40..7e638cd7d 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2020,6 +2020,20 @@ int ssl_read_record( ssl_context *ssl ) return( 0 ); } +int ssl_send_fatal_handshake_failure( ssl_context *ssl ) +{ + int ret; + + if( ( ret = ssl_send_alert_message( ssl, + SSL_ALERT_LEVEL_FATAL, + SSL_ALERT_MSG_HANDSHAKE_FAILURE ) ) != 0 ) + { + return( ret ); + } + + return( 0 ); +} + int ssl_send_alert_message( ssl_context *ssl, unsigned char level, unsigned char message ) @@ -3513,15 +3527,28 @@ int ssl_read( ssl_context *ssl, unsigned char *buf, size_t len ) return( POLARSSL_ERR_SSL_UNEXPECTED_MESSAGE ); } - if( ssl->disable_renegotiation == SSL_RENEGOTIATION_DISABLED ) + if( ssl->disable_renegotiation == SSL_RENEGOTIATION_DISABLED || + ( ssl->secure_renegotiation == SSL_LEGACY_RENEGOTIATION && + ssl->allow_legacy_renegotiation == SSL_LEGACY_NO_RENEGOTIATION ) ) { SSL_DEBUG_MSG( 3, ( "ignoring renegotiation, sending alert" ) ); - if( ( ret = ssl_send_alert_message( ssl, - SSL_ALERT_LEVEL_WARNING, - SSL_ALERT_MSG_NO_RENEGOTIATION ) ) != 0 ) + if( ssl->minor_ver == SSL_MINOR_VERSION_0 ) { - return( ret ); + /* + * SSLv3 does not have a "no_renegotiation" alert + */ + if( ( ret = ssl_send_fatal_handshake_failure( ssl ) ) != 0 ) + return( ret ); + } + else + { + if( ( ret = ssl_send_alert_message( ssl, + SSL_ALERT_LEVEL_WARNING, + SSL_ALERT_MSG_NO_RENEGOTIATION ) ) != 0 ) + { + return( ret ); + } } } else diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index df5274a13..0c962f210 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -51,7 +51,7 @@ #define DFL_KEY_FILE "" #define DFL_FORCE_CIPHER 0 #define DFL_RENEGOTIATION SSL_RENEGOTIATION_ENABLED -#define DFL_ALLOW_LEGACY SSL_NO_LEGACY_RENEGOTIATION +#define DFL_ALLOW_LEGACY SSL_LEGACY_NO_RENEGOTIATION #define GET_REQUEST "GET %s HTTP/1.0\r\n\r\n"