From 1d3b508b825f0f4fc2e27694fde6aa1c56184f34 Mon Sep 17 00:00:00 2001 From: Mohammad Azim Khan Date: Wed, 18 Apr 2018 19:35:00 +0100 Subject: [PATCH 1/3] Same ciphersuite validation in server and client hello --- ChangeLog | 2 ++ library/ssl_cli.c | 92 +++++++++++++++++++++++++++-------------------- 2 files changed, 55 insertions(+), 39 deletions(-) diff --git a/ChangeLog b/ChangeLog index 3e5dd6808..100551972 100644 --- a/ChangeLog +++ b/ChangeLog @@ -64,6 +64,8 @@ Bugfix * Fix buffer length assertions in the ssl_parse_certificate_request() function which leads to a potential one byte overread of the message buffer. + * Fix cipher suite validation in ssl_parse_server_hello() by performing same + checks as performed in ssl_write_client_hello(). Changes * Remove some redundant code in bignum.c. Contributed by Alexey Skalozub. diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 7cde5b113..efcf48bc0 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -717,6 +717,45 @@ static int ssl_generate_random( mbedtls_ssl_context *ssl ) return( 0 ); } +/** + * \brief Validate cipher suite against config in SSL context. + * + * \param suite_info cipher suite to validate + * \param ssl SSL context + * + * \return 0 if valid, else 1 + */ +static int ssl_validate_ciphersuite( const mbedtls_ssl_ciphersuite_t * suite_info, + const mbedtls_ssl_context * ssl ) +{ + if( suite_info == NULL ) + return( 1 ); + + if( suite_info->min_minor_ver > ssl->conf->max_minor_ver || + suite_info->max_minor_ver < ssl->conf->min_minor_ver ) + return( 1 ); + +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + ( suite_info->flags & MBEDTLS_CIPHERSUITE_NODTLS ) ) + return( 1 ); +#endif + +#if defined(MBEDTLS_ARC4_C) + if( ssl->conf->arc4_disabled == MBEDTLS_SSL_ARC4_DISABLED && + suite_info->cipher == MBEDTLS_CIPHER_ARC4_128 ) + return( 1 ); +#endif + +#if defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) + if( suite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE && + mbedtls_ecjpake_check( &ssl->handshake->ecjpake_ctx ) != 0 ) + return( 1 ); +#endif + + return( 0 ); +} + static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) { int ret; @@ -869,31 +908,9 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) { ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( ciphersuites[i] ); - if( ciphersuite_info == NULL ) + if( ssl_validate_ciphersuite( ciphersuite_info, ssl ) != 0 ) continue; - if( ciphersuite_info->min_minor_ver > ssl->conf->max_minor_ver || - ciphersuite_info->max_minor_ver < ssl->conf->min_minor_ver ) - continue; - -#if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && - ( ciphersuite_info->flags & MBEDTLS_CIPHERSUITE_NODTLS ) ) - continue; -#endif - -#if defined(MBEDTLS_ARC4_C) - if( ssl->conf->arc4_disabled == MBEDTLS_SSL_ARC4_DISABLED && - ciphersuite_info->cipher == MBEDTLS_CIPHER_ARC4_128 ) - continue; -#endif - -#if defined(MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED) - if( ciphersuite_info->key_exchange == MBEDTLS_KEY_EXCHANGE_ECJPAKE && - mbedtls_ecjpake_check( &ssl->handshake->ecjpake_ctx ) != 0 ) - continue; -#endif - MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello, add ciphersuite: %04x", ciphersuites[i] ) ); @@ -1690,22 +1707,8 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, chosen ciphersuite: %04x", i ) ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, compress alg.: %d", buf[37 + n] ) ); - suite_info = mbedtls_ssl_ciphersuite_from_id( ssl->session_negotiate->ciphersuite ); - if( suite_info == NULL -#if defined(MBEDTLS_ARC4_C) - || ( ssl->conf->arc4_disabled && - suite_info->cipher == MBEDTLS_CIPHER_ARC4_128 ) -#endif - ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello message" ) ); - mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER ); - return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); - } - - MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, chosen ciphersuite: %s", suite_info->name ) ); - + /* Perform cipher suite validation in same way as in ssl_write_client_hello. + */ i = 0; while( 1 ) { @@ -1724,6 +1727,17 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) } } + suite_info = mbedtls_ssl_ciphersuite_from_id( ssl->session_negotiate->ciphersuite ); + if( ssl_validate_ciphersuite( suite_info, ssl ) != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_ILLEGAL_PARAMETER ); + return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); + } + + MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, chosen ciphersuite: %s", suite_info->name ) ); + if( comp != MBEDTLS_SSL_COMPRESS_NULL #if defined(MBEDTLS_ZLIB_SUPPORT) && comp != MBEDTLS_SSL_COMPRESS_DEFLATE From 03bac448db441d66612e2a2ed2c5e2f1ec2b04b8 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 25 Apr 2018 05:06:07 -0400 Subject: [PATCH 2/3] Change accepted ciphersuite versions when parsing server hello Accept only ciphersuites for version chosen by the server --- library/ssl_cli.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index efcf48bc0..f4dc02aba 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -722,17 +722,21 @@ static int ssl_generate_random( mbedtls_ssl_context *ssl ) * * \param suite_info cipher suite to validate * \param ssl SSL context + * \param min_minor_ver Minimal minor version to accept a cipher suite + * \param max_minor_ver Maximal minor version to accept a cipher suite * * \return 0 if valid, else 1 */ static int ssl_validate_ciphersuite( const mbedtls_ssl_ciphersuite_t * suite_info, - const mbedtls_ssl_context * ssl ) + const mbedtls_ssl_context * ssl, + int min_minor_ver, int max_minor_ver ) { + (void) ssl; if( suite_info == NULL ) return( 1 ); - if( suite_info->min_minor_ver > ssl->conf->max_minor_ver || - suite_info->max_minor_ver < ssl->conf->min_minor_ver ) + if( suite_info->min_minor_ver > max_minor_ver || + suite_info->max_minor_ver < min_minor_ver ) return( 1 ); #if defined(MBEDTLS_SSL_PROTO_DTLS) @@ -908,7 +912,9 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) { ciphersuite_info = mbedtls_ssl_ciphersuite_from_id( ciphersuites[i] ); - if( ssl_validate_ciphersuite( ciphersuite_info, ssl ) != 0 ) + if( ssl_validate_ciphersuite( ciphersuite_info, ssl, + ssl->conf->min_minor_ver, + ssl->conf->max_minor_ver ) != 0 ) continue; MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello, add ciphersuite: %04x", @@ -1707,7 +1713,8 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, chosen ciphersuite: %04x", i ) ); MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, compress alg.: %d", buf[37 + n] ) ); - /* Perform cipher suite validation in same way as in ssl_write_client_hello. + /* + * Perform cipher suite validation in same way as in ssl_write_client_hello. */ i = 0; while( 1 ) @@ -1728,7 +1735,7 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) } suite_info = mbedtls_ssl_ciphersuite_from_id( ssl->session_negotiate->ciphersuite ); - if( ssl_validate_ciphersuite( suite_info, ssl ) != 0 ) + if( ssl_validate_ciphersuite( suite_info, ssl, ssl->minor_ver, ssl->minor_ver ) != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello message" ) ); mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, From b7a18c049863bcddcc74321a0d32467216f844cd Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 25 Apr 2018 05:25:30 -0400 Subject: [PATCH 3/3] Changelog entry --- ChangeLog | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 100551972..a0810d1a7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -15,6 +15,11 @@ Security where an optional signature algorithms list is expected in the cases of the signature algorithms section being too short. In the debug builds the overread data is printed to the standard output. + * Fix a client-side bug in the validation of the server's ciphersuite choice + potentially leading to the client accepting a ciphersuite it didn't offer + or one that cannot be used with the (D)TLS version chosen by the server. + This may lead to corruption of internal data structures for some + configurations. Features * Add option MBEDTLS_AES_FEWER_TABLES to dynamically compute 3/4 of the AES tables @@ -64,8 +69,6 @@ Bugfix * Fix buffer length assertions in the ssl_parse_certificate_request() function which leads to a potential one byte overread of the message buffer. - * Fix cipher suite validation in ssl_parse_server_hello() by performing same - checks as performed in ssl_write_client_hello(). Changes * Remove some redundant code in bignum.c. Contributed by Alexey Skalozub.