From 0acbd7df03f0018607b2085e588712afd2d9dfbc 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 4be7ec4c5..98da62639 100644 --- a/ChangeLog +++ b/ChangeLog @@ -39,6 +39,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 * Support cmake build where Mbed TLS is a subproject. Fix diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 0d8836406..d34946eae 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 660809654411873626143d5106478730fa3cc909 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 d34946eae..a7aaaff12 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 ef30d96739b42d53523a10a81c3fdd65d9665240 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 25 Apr 2018 05:29:00 -0400 Subject: [PATCH 3/3] Changelog entry --- ChangeLog | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 98da62639..77e1b4a03 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. Bugfix * Fix spurious uninitialized variable warning in cmac.c. Fix independently @@ -39,8 +44,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 * Support cmake build where Mbed TLS is a subproject. Fix