From 364fd8bb717686367d11d38c55957251e31eb630 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 15 Feb 2022 23:53:36 +0100 Subject: [PATCH 01/11] More SSL debug messages for ClientHello parsing In particular, be verbose when checking the ClientHello cookie in a possible DTLS reconnection. Signed-off-by: Gilles Peskine --- library/ssl_msg.c | 62 ++++++++++++++++++++++++++------------ library/ssl_tls12_server.c | 10 +++++- 2 files changed, 52 insertions(+), 20 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 083c8d2e6..a77a1a821 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3139,8 +3139,8 @@ void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ) #if defined(MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE) && defined(MBEDTLS_SSL_SRV_C) /* - * Without any SSL context, check if a datagram looks like a ClientHello with - * a valid cookie, and if it doesn't, generate a HelloVerifyRequest message. + * Check if a datagram looks like a ClientHello with a valid cookie, + * and if it doesn't, generate a HelloVerifyRequest message. * Both input and output include full DTLS headers. * * - if cookie is valid, return 0 @@ -3150,9 +3150,7 @@ void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ) * - otherwise return a specific error code */ static int ssl_check_dtls_clihlo_cookie( - mbedtls_ssl_cookie_write_t *f_cookie_write, - mbedtls_ssl_cookie_check_t *f_cookie_check, - void *p_cookie, + mbedtls_ssl_context *ssl, const unsigned char *cli_id, size_t cli_id_len, const unsigned char *in, size_t in_len, unsigned char *obuf, size_t buf_len, size_t *olen ) @@ -3186,26 +3184,52 @@ static int ssl_check_dtls_clihlo_cookie( * * Minimum length is 61 bytes. */ - if( in_len < 61 || - in[0] != MBEDTLS_SSL_MSG_HANDSHAKE || + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: in_len=%u", + (unsigned) in_len ) ); + MBEDTLS_SSL_DEBUG_BUF( 4, "cli_id", cli_id, cli_id_len ); + if( in_len < 61 ) + { + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: record too short" ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + if( in[0] != MBEDTLS_SSL_MSG_HANDSHAKE || in[3] != 0 || in[4] != 0 || in[19] != 0 || in[20] != 0 || in[21] != 0 ) { + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: not a good ClientHello" ) ); + MBEDTLS_SSL_DEBUG_MSG( 4, ( " type=%u epoch=%u fragment_offset=%u", + in[0], + (unsigned) in[3] << 8 | in[4], + (unsigned) in[19] << 16 | in[20] << 8 | in[21] ) ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } sid_len = in[59]; if( sid_len > in_len - 61 ) + { + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: sid_len=%u > %u", + (unsigned) sid_len, + (unsigned) in_len - 61 ) ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + MBEDTLS_SSL_DEBUG_BUF( 4, "sid received from network", + in + 60, sid_len ); cookie_len = in[60 + sid_len]; - if( cookie_len > in_len - 60 ) + if( cookie_len > in_len - 60 ) { + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: cookie_len=%u > %u", + (unsigned) cookie_len, + (unsigned) in_len - 60 ) ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } - if( f_cookie_check( p_cookie, in + sid_len + 61, cookie_len, - cli_id, cli_id_len ) == 0 ) + MBEDTLS_SSL_DEBUG_BUF( 4, "cookie received from network", + in + sid_len + 61, cookie_len ); + if( ssl->conf->f_cookie_check( ssl->conf->p_cookie, + in + sid_len + 61, cookie_len, + cli_id, cli_id_len ) == 0 ) { - /* Valid cookie */ + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: valid" ) ); return( 0 ); } @@ -3240,8 +3264,9 @@ static int ssl_check_dtls_clihlo_cookie( /* Generate and write actual cookie */ p = obuf + 28; - if( f_cookie_write( p_cookie, - &p, obuf + buf_len, cli_id, cli_id_len ) != 0 ) + if( ssl->conf->f_cookie_write( ssl->conf->p_cookie, + &p, obuf + buf_len, + cli_id, cli_id_len ) != 0 ) { return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } @@ -3296,9 +3321,7 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) } ret = ssl_check_dtls_clihlo_cookie( - ssl->conf->f_cookie_write, - ssl->conf->f_cookie_check, - ssl->conf->p_cookie, + ssl, ssl->cli_id, ssl->cli_id_len, ssl->in_buf, ssl->in_left, ssl->out_buf, MBEDTLS_SSL_OUT_CONTENT_LEN, &len ); @@ -3481,7 +3504,6 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, /* * Parse and validate record version */ - rec->ver[0] = buf[ rec_hdr_version_offset + 0 ]; rec->ver[1] = buf[ rec_hdr_version_offset + 1 ]; tls_version = mbedtls_ssl_read_version( buf + rec_hdr_version_offset, @@ -3489,10 +3511,12 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, if( tls_version > ssl->conf->max_tls_version ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "TLS version mismatch" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "TLS version mismatch: got %u, expected max %u", + (unsigned) tls_version, + (unsigned) ssl->conf->max_tls_version) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - /* * Parse/Copy record sequence number. */ diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index a1505d16a..58d609335 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1286,7 +1286,10 @@ read_record_header: if( buf[1] != 0 || msg_len != mbedtls_ssl_hs_hdr_len( ssl ) + ( ( buf[2] << 8 ) | buf[3] ) ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != %u + %u", + (unsigned) msg_len, + (unsigned) mbedtls_ssl_hs_hdr_len( ssl ), + (unsigned) ( buf[2] << 8 ) | buf[3] ) ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } @@ -1327,6 +1330,11 @@ read_record_header: * For now we don't support fragmentation, so make sure * fragment_offset == 0 and fragment_length == length */ + MBEDTLS_SSL_DEBUG_MSG( + 4, ( "fragment_offset=%u fragment_length=%u length=%u", + (unsigned) ( ssl->in_msg[6] << 16 | ssl->in_msg[7] << 8 | ssl->in_msg[8] ), + (unsigned) ( ssl->in_msg[9] << 16 | ssl->in_msg[10] << 8 | ssl->in_msg[11] ), + (unsigned) ( ssl->in_msg[1] << 16 | ssl->in_msg[2] << 8 | ssl->in_msg[3] ) ) ); if( ssl->in_msg[6] != 0 || ssl->in_msg[7] != 0 || ssl->in_msg[8] != 0 || memcmp( ssl->in_msg + 1, ssl->in_msg + 9, 3 ) != 0 ) { From c8183cc4924e25a205db6f42150fd075cdeb83c8 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Mon, 6 Jun 2022 14:42:41 -0400 Subject: [PATCH 02/11] Add missing sid_len in calculations of cookie sizes This could lead to a potential buffer overread with small MBEDTLS_SSL_IN_CONTENT_LEN. Change the bound calculations so that it is apparent what lengths and sizes are used. Signed-off-by: Andrzej Kurek --- library/ssl_msg.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index a77a1a821..da4dbc721 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3205,7 +3205,7 @@ static int ssl_check_dtls_clihlo_cookie( } sid_len = in[59]; - if( sid_len > in_len - 61 ) + if( 59 + 1 + sid_len + 1 > in_len ) { MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: sid_len=%u > %u", (unsigned) sid_len, @@ -3216,10 +3216,11 @@ static int ssl_check_dtls_clihlo_cookie( in + 60, sid_len ); cookie_len = in[60 + sid_len]; - if( cookie_len > in_len - 60 ) { + if( 59 + 1 + sid_len + 1 + cookie_len > in_len ) + { MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: cookie_len=%u > %u", (unsigned) cookie_len, - (unsigned) in_len - 60 ) ); + (unsigned) ( in_len - sid_len - 61 ) ) ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } From cfb01948c86e5a13ca164dd69a576264ff71583e Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Mon, 6 Jun 2022 13:08:23 -0400 Subject: [PATCH 03/11] Add cookie parsing tests to test_suite_ssl Signed-off-by: Andrzej Kurek --- library/ssl_misc.h | 8 ++++++++ library/ssl_msg.c | 5 ++++- tests/suites/test_suite_ssl.data | 18 ++++++++++++++++++ tests/suites/test_suite_ssl.function | 27 +++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index c2c3cd210..1a3217c6c 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2296,4 +2296,12 @@ int mbedtls_ssl_validate_ciphersuite( int mbedtls_ssl_write_sig_alg_ext( mbedtls_ssl_context *ssl, unsigned char *buf, const unsigned char *end, size_t *out_len ); +#if defined(MBEDTLS_TEST_HOOKS) +int ssl_check_dtls_clihlo_cookie( + mbedtls_ssl_context *ssl, + const unsigned char *cli_id, size_t cli_id_len, + const unsigned char *in, size_t in_len, + unsigned char *obuf, size_t buf_len, size_t *olen ); +#endif + #endif /* ssl_misc.h */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index da4dbc721..ee6a01be5 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3149,7 +3149,10 @@ void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ) * return MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED * - otherwise return a specific error code */ -static int ssl_check_dtls_clihlo_cookie( +#if !defined(MBEDTLS_TEST_HOOKS) +static +#endif +int ssl_check_dtls_clihlo_cookie( mbedtls_ssl_context *ssl, const unsigned char *cli_id, size_t cli_id_len, const unsigned char *in, size_t in_len, diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 848a497cb..b6a46b2a0 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3366,3 +3366,21 @@ raw_key_agreement_fail:0 Raw key agreement: bad server key raw_key_agreement_fail:1 + +Cookie parsing: nominal run +cookie_parsing:"16fefd0000000000000000002F010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d00200000000000000000000000000000000000000000000000000000000000000000":MBEDTLS_ERR_SSL_INTERNAL_ERROR + +Cookie parsing: cookie_len overflow +cookie_parsing:"16fefd000000000000000000ea010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727db97b7373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737373737db963":MBEDTLS_ERR_SSL_DECODE_ERROR + +Cookie parsing: non-zero fragment offset +cookie_parsing:"16fefd00000000000000000032010000de000072000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d01730143":MBEDTLS_ERR_SSL_DECODE_ERROR + +Cookie parsing: sid_len overflow +cookie_parsing:"16fefd00000000000000000032010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF730143":MBEDTLS_ERR_SSL_DECODE_ERROR + +Cookie parsing: record too short +cookie_parsing:"16fefd0000000000000000002f010000de000072000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF":MBEDTLS_ERR_SSL_DECODE_ERROR + +Cookie parsing: one byte overread +cookie_parsing:"16fefd0000000000000000002F010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d0001":MBEDTLS_ERR_SSL_DECODE_ERROR diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 35f1638cb..51d57a057 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -5506,6 +5506,33 @@ void conf_group() } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +void cookie_parsing( data_t *cookie, int exp_ret ) +{ + mbedtls_ssl_context ssl; + mbedtls_ssl_config conf; + size_t len; + + mbedtls_ssl_init( &ssl ); + mbedtls_ssl_config_init( &conf ); + TEST_EQUAL( mbedtls_ssl_config_defaults( &conf, MBEDTLS_SSL_IS_SERVER, + MBEDTLS_SSL_TRANSPORT_DATAGRAM, + MBEDTLS_SSL_PRESET_DEFAULT ), + 0 ); + + TEST_EQUAL( mbedtls_ssl_setup( &ssl, &conf ), 0 ); + TEST_EQUAL( ssl_check_dtls_clihlo_cookie( &ssl, ssl.cli_id, ssl.cli_id_len, + cookie->x, cookie->len, + ssl.out_buf, + MBEDTLS_SSL_OUT_CONTENT_LEN, + &len ), + exp_ret ); + + mbedtls_ssl_free( &ssl ); + mbedtls_ssl_config_free( &conf ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_TIMING_C:MBEDTLS_HAVE_TIME */ void timing_final_delay_accessor( ) { From e6487ab490edbc28618270164ca0503bb02018ea Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Mon, 6 Jun 2022 14:54:58 -0400 Subject: [PATCH 04/11] Add a changelog entry for the cookie parsing bounds bug Co-authored-by: Gilles Peskine Signed-off-by: Andrzej Kurek --- ChangeLog.d/cookie_parsing_bug.txt | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 ChangeLog.d/cookie_parsing_bug.txt diff --git a/ChangeLog.d/cookie_parsing_bug.txt b/ChangeLog.d/cookie_parsing_bug.txt new file mode 100644 index 000000000..a5f5875d3 --- /dev/null +++ b/ChangeLog.d/cookie_parsing_bug.txt @@ -0,0 +1,11 @@ +Security + * Fix a buffer overread in DTLS ClientHello parsing in servers with + MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE enabled. An unauthenticated client + or a man-in-the-middle could cause a DTLS server to read up to 255 bytes + after the end of the SSL input buffer. The buffer overread only happens + when MBEDTLS_SSL_IN_CONTENT_LEN is less than a threshold that depends on + the exact configuration: 258 bytes if using mbedtls_ssl_cookie_check(), + and possibly up to 571 bytes with a custom cookie check function. + If the function provider deliberately omits these size checks, he/she + is responsible for the negative impact on his/her code. + Reported by the Cybeats PSI Team. From 078e9bcda6a0ccc355dcff9b0adf87edd1fdfdff Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 8 Jun 2022 11:47:33 -0400 Subject: [PATCH 05/11] Add the mbedtls prefix to ssl_check_dtls_clihlo_cookie Signed-off-by: Andrzej Kurek --- library/ssl_misc.h | 2 +- library/ssl_msg.c | 10 ++++------ tests/suites/test_suite_ssl.function | 11 ++++++----- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/library/ssl_misc.h b/library/ssl_misc.h index 1a3217c6c..f07f9d452 100644 --- a/library/ssl_misc.h +++ b/library/ssl_misc.h @@ -2297,7 +2297,7 @@ int mbedtls_ssl_write_sig_alg_ext( mbedtls_ssl_context *ssl, unsigned char *buf, const unsigned char *end, size_t *out_len ); #if defined(MBEDTLS_TEST_HOOKS) -int ssl_check_dtls_clihlo_cookie( +int mbedtls_ssl_check_dtls_clihlo_cookie( mbedtls_ssl_context *ssl, const unsigned char *cli_id, size_t cli_id_len, const unsigned char *in, size_t in_len, diff --git a/library/ssl_msg.c b/library/ssl_msg.c index ee6a01be5..681e431e6 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3149,10 +3149,8 @@ void mbedtls_ssl_dtls_replay_update( mbedtls_ssl_context *ssl ) * return MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED * - otherwise return a specific error code */ -#if !defined(MBEDTLS_TEST_HOOKS) -static -#endif -int ssl_check_dtls_clihlo_cookie( +MBEDTLS_STATIC_TESTABLE +int mbedtls_ssl_check_dtls_clihlo_cookie( mbedtls_ssl_context *ssl, const unsigned char *cli_id, size_t cli_id_len, const unsigned char *in, size_t in_len, @@ -3324,13 +3322,13 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) return( 0 ); } - ret = ssl_check_dtls_clihlo_cookie( + ret = mbedtls_ssl_check_dtls_clihlo_cookie( ssl, ssl->cli_id, ssl->cli_id_len, ssl->in_buf, ssl->in_left, ssl->out_buf, MBEDTLS_SSL_OUT_CONTENT_LEN, &len ); - MBEDTLS_SSL_DEBUG_RET( 2, "ssl_check_dtls_clihlo_cookie", ret ); + MBEDTLS_SSL_DEBUG_RET( 2, "mbedtls_ssl_check_dtls_clihlo_cookie", ret ); if( ret == MBEDTLS_ERR_SSL_HELLO_VERIFY_REQUIRED ) { diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 51d57a057..67880da3a 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -5521,11 +5521,12 @@ void cookie_parsing( data_t *cookie, int exp_ret ) 0 ); TEST_EQUAL( mbedtls_ssl_setup( &ssl, &conf ), 0 ); - TEST_EQUAL( ssl_check_dtls_clihlo_cookie( &ssl, ssl.cli_id, ssl.cli_id_len, - cookie->x, cookie->len, - ssl.out_buf, - MBEDTLS_SSL_OUT_CONTENT_LEN, - &len ), + TEST_EQUAL( mbedtls_ssl_check_dtls_clihlo_cookie( &ssl, ssl.cli_id, + ssl.cli_id_len, + cookie->x, cookie->len, + ssl.out_buf, + MBEDTLS_SSL_OUT_CONTENT_LEN, + &len ), exp_ret ); mbedtls_ssl_free( &ssl ); From b58cf0d172cf3d6a4a27d16ab29ecf43be7546db Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 8 Jun 2022 11:53:59 -0400 Subject: [PATCH 06/11] Split a debug message into two - for clarity Signed-off-by: Andrzej Kurek --- library/ssl_tls12_server.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 58d609335..4e0756988 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1282,9 +1282,14 @@ read_record_header: MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello v3, handshake len.: %d", ( buf[1] << 16 ) | ( buf[2] << 8 ) | buf[3] ) ); + if( buf[1] != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != 0", + (unsigned) buf[1] ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } /* We don't support fragmentation of ClientHello (yet?) */ - if( buf[1] != 0 || - msg_len != mbedtls_ssl_hs_hdr_len( ssl ) + ( ( buf[2] << 8 ) | buf[3] ) ) + if( msg_len != mbedtls_ssl_hs_hdr_len( ssl ) + ( ( buf[2] << 8 ) | buf[3] ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != %u + %u", (unsigned) msg_len, From ed4d217874f3c2d31eeb38f0f586907cfdd85b7a Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 8 Jun 2022 11:57:57 -0400 Subject: [PATCH 07/11] Add missing test dependencies for cookie parsing Signed-off-by: Andrzej Kurek --- tests/suites/test_suite_ssl.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 67880da3a..2c7ecd55b 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -5506,7 +5506,7 @@ void conf_group() } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_TEST_HOOKS */ +/* BEGIN_CASE depends_on:MBEDTLS_SSL_SRV_C:MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE:MBEDTLS_TEST_HOOKS */ void cookie_parsing( data_t *cookie, int exp_ret ) { mbedtls_ssl_context ssl; From 96d5439da5dc69419604a44a021bb77f55c3820e Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 8 Jun 2022 12:00:52 -0400 Subject: [PATCH 08/11] Fix incorrect changelog entry Signed-off-by: Andrzej Kurek --- ChangeLog.d/cookie_parsing_bug.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/ChangeLog.d/cookie_parsing_bug.txt b/ChangeLog.d/cookie_parsing_bug.txt index a5f5875d3..1c25f3952 100644 --- a/ChangeLog.d/cookie_parsing_bug.txt +++ b/ChangeLog.d/cookie_parsing_bug.txt @@ -6,6 +6,4 @@ Security when MBEDTLS_SSL_IN_CONTENT_LEN is less than a threshold that depends on the exact configuration: 258 bytes if using mbedtls_ssl_cookie_check(), and possibly up to 571 bytes with a custom cookie check function. - If the function provider deliberately omits these size checks, he/she - is responsible for the negative impact on his/her code. Reported by the Cybeats PSI Team. From cbe14ec96755121037c48c39d0bbe3e347ac63ab Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 15 Jun 2022 07:17:28 -0400 Subject: [PATCH 09/11] Improve variable extracting operations by using MBEDTLS_GET macros Signed-off-by: Andrzej Kurek --- library/ssl_msg.c | 16 +++++---- library/ssl_tls12_server.c | 66 +++++++++++++++++++++----------------- 2 files changed, 46 insertions(+), 36 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 681e431e6..5aa098ec5 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3156,7 +3156,7 @@ int mbedtls_ssl_check_dtls_clihlo_cookie( const unsigned char *in, size_t in_len, unsigned char *obuf, size_t buf_len, size_t *olen ) { - size_t sid_len, cookie_len; + size_t sid_len, cookie_len, epoch, fragment_offset; unsigned char *p; /* @@ -3193,15 +3193,17 @@ int mbedtls_ssl_check_dtls_clihlo_cookie( MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: record too short" ) ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } - if( in[0] != MBEDTLS_SSL_MSG_HANDSHAKE || - in[3] != 0 || in[4] != 0 || - in[19] != 0 || in[20] != 0 || in[21] != 0 ) + + epoch = MBEDTLS_GET_UINT16_BE( in, 3 ); + fragment_offset = MBEDTLS_GET_UINT24_BE( in, 19 ); + + if( in[0] != MBEDTLS_SSL_MSG_HANDSHAKE || epoch != 0 || + fragment_offset != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: not a good ClientHello" ) ); MBEDTLS_SSL_DEBUG_MSG( 4, ( " type=%u epoch=%u fragment_offset=%u", - in[0], - (unsigned) in[3] << 8 | in[4], - (unsigned) in[19] << 16 | in[20] << 8 | in[21] ) ); + in[0], (unsigned) epoch, + (unsigned) fragment_offset ) ); return( MBEDTLS_ERR_SSL_DECODE_ERROR ); } diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 4e0756988..7f8047beb 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1278,24 +1278,29 @@ read_record_header: MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); return( MBEDTLS_ERR_SSL_UNEXPECTED_MESSAGE ); } - - MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello v3, handshake len.: %d", - ( buf[1] << 16 ) | ( buf[2] << 8 ) | buf[3] ) ); - - if( buf[1] != 0 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != 0", - (unsigned) buf[1] ) ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); - } - /* We don't support fragmentation of ClientHello (yet?) */ - if( msg_len != mbedtls_ssl_hs_hdr_len( ssl ) + ( ( buf[2] << 8 ) | buf[3] ) ) - { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != %u + %u", + size_t handshake_len = MBEDTLS_GET_UINT24_BE( buf, 1 ); + MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello v3, handshake len.: %d", + ( unsigned ) handshake_len ) ); + + /* The record layer has a record size limit of 2^14 - 1 and + * fragmentation is not supported, so buf[1] should be zero. */ + if( buf[1] != 0 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != 0", + (unsigned) buf[1] ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } + + /* We don't support fragmentation of ClientHello (yet?) */ + if( msg_len != mbedtls_ssl_hs_hdr_len( ssl ) + handshake_len ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != %u + %u", (unsigned) msg_len, (unsigned) mbedtls_ssl_hs_hdr_len( ssl ), - (unsigned) ( buf[2] << 8 ) | buf[3] ) ); - return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + (unsigned) handshake_len ) ); + return( MBEDTLS_ERR_SSL_DECODE_ERROR ); + } } #if defined(MBEDTLS_SSL_PROTO_DTLS) @@ -1330,21 +1335,24 @@ read_record_header: ssl->handshake->out_msg_seq = cli_msg_seq; ssl->handshake->in_msg_seq = cli_msg_seq + 1; } - - /* - * For now we don't support fragmentation, so make sure - * fragment_offset == 0 and fragment_length == length - */ - MBEDTLS_SSL_DEBUG_MSG( - 4, ( "fragment_offset=%u fragment_length=%u length=%u", - (unsigned) ( ssl->in_msg[6] << 16 | ssl->in_msg[7] << 8 | ssl->in_msg[8] ), - (unsigned) ( ssl->in_msg[9] << 16 | ssl->in_msg[10] << 8 | ssl->in_msg[11] ), - (unsigned) ( ssl->in_msg[1] << 16 | ssl->in_msg[2] << 8 | ssl->in_msg[3] ) ) ); - if( ssl->in_msg[6] != 0 || ssl->in_msg[7] != 0 || ssl->in_msg[8] != 0 || - memcmp( ssl->in_msg + 1, ssl->in_msg + 9, 3 ) != 0 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "ClientHello fragmentation not supported" ) ); - return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); + /* + * For now we don't support fragmentation, so make sure + * fragment_offset == 0 and fragment_length == length + */ + size_t fragment_offset, fragment_length, length; + fragment_offset = MBEDTLS_GET_UINT24_BE( ssl->in_msg, 6 ); + fragment_length = MBEDTLS_GET_UINT24_BE( ssl->in_msg, 9 ); + length = MBEDTLS_GET_UINT24_BE( ssl->in_msg, 1 ); + MBEDTLS_SSL_DEBUG_MSG( + 4, ( "fragment_offset=%u fragment_length=%u length=%u", + (unsigned) fragment_offset, (unsigned) fragment_length, + (unsigned) length ) ); + if( fragment_offset != 0 || length != fragment_length ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "ClientHello fragmentation not supported" ) ); + return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); + } } } #endif /* MBEDTLS_SSL_PROTO_DTLS */ From ca35f5bed0e7b56c88fe918ceb9fba7ea912011f Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 15 Jun 2022 07:19:40 -0400 Subject: [PATCH 10/11] test_suite_ssl: Use a zero fragment offset in a test with a too short record Signed-off-by: Andrzej Kurek --- tests/suites/test_suite_ssl.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index b6a46b2a0..4db96037f 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -3380,7 +3380,7 @@ Cookie parsing: sid_len overflow cookie_parsing:"16fefd00000000000000000032010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF730143":MBEDTLS_ERR_SSL_DECODE_ERROR Cookie parsing: record too short -cookie_parsing:"16fefd0000000000000000002f010000de000072000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF":MBEDTLS_ERR_SSL_DECODE_ERROR +cookie_parsing:"16fefd0000000000000000002f010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF":MBEDTLS_ERR_SSL_DECODE_ERROR Cookie parsing: one byte overread cookie_parsing:"16fefd0000000000000000002F010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d0001":MBEDTLS_ERR_SSL_DECODE_ERROR From 755ddff25c68a97cd4e8ac0c5b23761d4ac6a154 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Wed, 15 Jun 2022 07:31:40 -0400 Subject: [PATCH 11/11] Fix print format in a debug message Signed-off-by: Andrzej Kurek --- library/ssl_tls12_server.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls12_server.c b/library/ssl_tls12_server.c index 7f8047beb..ac36f04d2 100644 --- a/library/ssl_tls12_server.c +++ b/library/ssl_tls12_server.c @@ -1280,7 +1280,7 @@ read_record_header: } { size_t handshake_len = MBEDTLS_GET_UINT24_BE( buf, 1 ); - MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello v3, handshake len.: %d", + MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello v3, handshake len.: %u", ( unsigned ) handshake_len ) ); /* The record layer has a record size limit of 2^14 - 1 and