From 3f1d5cb324ecc901e7491940a461e31765efbcb7 Mon Sep 17 00:00:00 2001 From: Mohammad Azim Khan Date: Wed, 18 Apr 2018 19:35:00 +0100 Subject: [PATCH 1/4] Same ciphersuite validation in server and client hello --- ChangeLog | 2 ++ library/ssl_cli.c | 80 ++++++++++++++++++++++++++++++----------------- 2 files changed, 53 insertions(+), 29 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5084c48ec4..9ce80400b5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -35,6 +35,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 * Improve testing in configurations that omit certain hashes or diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 1899886b0c..282f6b0557 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -632,6 +632,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; @@ -784,25 +823,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 - MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello, add ciphersuite: %2d", ciphersuites[i] ) ); @@ -1507,18 +1530,8 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, chosen ciphersuite: %d", 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" ) ); - return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); - } - + /* Perform cipher suite validation in same way as in ssl_write_client_hello. + */ i = 0; while( 1 ) { @@ -1535,6 +1548,15 @@ 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" ) ); + 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 302be2fce46811a6f94cb7303b082b0e19c6aef6 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 25 Apr 2018 05:06:07 -0400 Subject: [PATCH 2/4] 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 282f6b0557..483743dad1 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -637,17 +637,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) @@ -823,7 +827,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: %2d", @@ -1530,7 +1536,8 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 3, ( "server hello, chosen ciphersuite: %d", 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 ) @@ -1549,7 +1556,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" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); From 128bcbea1a9fb59085fc122382a4363281c366bb Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 25 Apr 2018 05:25:30 -0400 Subject: [PATCH 3/4] Changelog entry --- ChangeLog | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9ce80400b5..8635fb9c9a 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 * Add missing dependencies in test suites that led to build failures @@ -35,8 +40,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 * Improve testing in configurations that omit certain hashes or From 32f5cc6dd4ba0612ee2f7cdafddb854903db5ec8 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Thu, 26 Apr 2018 10:43:28 +0100 Subject: [PATCH 4/4] ssl_cli: Fix all.sh test failure for ECJPAKE typo When the "same ciphersuite validation" was backported to 2.1, we introduced the use of irrelevant defines in ssl_cli.c. all.sh catches these as "Likely typos". Remove the code for ECJPAKE, a feature that doesn't exist in 2.1, from ssl_cli to fix this test failure. ****************************************************************** * test/build: declared and exported names * Thu Apr 26 08:23:19 UTC 2018 ****************************************************************** 1175 macros 143 enum-consts 942 identifiers 771 exported-symbols Exported symbols declared in header: PASS Names of actual-macros: PASS Names of enum-consts: PASS Names of identifiers: PASS Likely typos: FAIL MBEDTLS_KEY_EXCHANGE_ECJPAKE MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED FAILED --- library/ssl_cli.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 483743dad1..2d1dcf868c 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -666,12 +666,6 @@ static int ssl_validate_ciphersuite( const mbedtls_ssl_ciphersuite_t * suite_inf 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 ); }