diff --git a/ChangeLog.d/cookie_parsing_bug.txt b/ChangeLog.d/cookie_parsing_bug.txt new file mode 100644 index 0000000000..1c25f39527 --- /dev/null +++ b/ChangeLog.d/cookie_parsing_bug.txt @@ -0,0 +1,9 @@ +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. + Reported by the Cybeats PSI Team. diff --git a/include/mbedtls/ssl_internal.h b/include/mbedtls/ssl_internal.h index f50cf9ff5b..83047c5bda 100644 --- a/include/mbedtls/ssl_internal.h +++ b/include/mbedtls/ssl_internal.h @@ -1306,4 +1306,12 @@ void mbedtls_ssl_buffering_free( mbedtls_ssl_context *ssl ); void mbedtls_ssl_flight_free( mbedtls_ssl_flight_item *flight ); #endif /* MBEDTLS_SSL_PROTO_DTLS */ +#if defined(MBEDTLS_TEST_HOOKS) +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, + unsigned char *obuf, size_t buf_len, size_t *olen ); +#endif + #endif /* ssl_internal.h */ diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 5a4574d2de..dfcdc93272 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -3229,8 +3229,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 @@ -3239,10 +3239,9 @@ 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( - mbedtls_ssl_cookie_write_t *f_cookie_write, - mbedtls_ssl_cookie_check_t *f_cookie_check, - void *p_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, unsigned char *obuf, size_t buf_len, size_t *olen ) @@ -3276,26 +3275,53 @@ 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_BAD_HS_CLIENT_HELLO ); + } + 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_BAD_HS_CLIENT_HELLO ); } 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, + (unsigned) in_len - 61 ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } + 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 ) - return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); - - if( f_cookie_check( p_cookie, in + sid_len + 61, cookie_len, - cli_id, cli_id_len ) == 0 ) + if( 59 + 1 + sid_len + 1 + cookie_len > in_len ) { - /* Valid cookie */ + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: cookie_len=%u > %u", + (unsigned) cookie_len, + (unsigned) ( in_len - sid_len - 61 ) ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } + + 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 ) + { + MBEDTLS_SSL_DEBUG_MSG( 4, ( "check cookie: valid" ) ); return( 0 ); } @@ -3330,8 +3356,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 ); } @@ -3385,15 +3412,13 @@ static int ssl_handle_possible_reconnect( mbedtls_ssl_context *ssl ) return( 0 ); } - ret = ssl_check_dtls_clihlo_cookie( - ssl->conf->f_cookie_write, - ssl->conf->f_cookie_check, - ssl->conf->p_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 ) { @@ -3571,7 +3596,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 ]; mbedtls_ssl_read_version( &major_ver, &minor_ver, @@ -3580,16 +3604,19 @@ static int ssl_parse_record_header( mbedtls_ssl_context const *ssl, if( major_ver != ssl->major_ver ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "major version mismatch" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "major version mismatch: got %u, expected %u", + (unsigned) major_ver, + (unsigned) ssl->major_ver ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } if( minor_ver > ssl->conf->max_minor_ver ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "minor version mismatch" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "minor version mismatch: got %u, expected max %u", + (unsigned) minor_ver, + (unsigned) ssl->conf->max_minor_ver ) ); return( MBEDTLS_ERR_SSL_INVALID_RECORD ); } - /* * Parse/Copy record sequence number. */ diff --git a/library/ssl_srv.c b/library/ssl_srv.c index bce51cd83f..1733ec931f 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -1604,11 +1604,19 @@ read_record_header: MBEDTLS_SSL_DEBUG_MSG( 3, ( "client hello v3, handshake len.: %d", ( buf[1] << 16 ) | ( buf[2] << 8 ) | buf[3] ) ); - /* 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( buf[1] != 0 ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message: %u != 0", + (unsigned) buf[1] ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } + /* 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", + (unsigned) msg_len, + (unsigned) mbedtls_ssl_hs_hdr_len( ssl ), + (unsigned) ( buf[2] << 8 ) | buf[3] ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } @@ -1649,6 +1657,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 ) { diff --git a/tests/suites/test_suite_ssl.data b/tests/suites/test_suite_ssl.data index 0f32671aad..0e97e6fed9 100644 --- a/tests/suites/test_suite_ssl.data +++ b/tests/suites/test_suite_ssl.data @@ -10689,3 +10689,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_BAD_HS_CLIENT_HELLO + +Cookie parsing: non-zero fragment offset +cookie_parsing:"16fefd00000000000000000032010000de000072000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d01730143":MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO + +Cookie parsing: sid_len overflow +cookie_parsing:"16fefd00000000000000000032010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF730143":MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO + +Cookie parsing: record too short +cookie_parsing:"16fefd0000000000000000002f010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF":MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO + +Cookie parsing: one byte overread +cookie_parsing:"16fefd0000000000000000002F010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d0001":MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 8318df01cc..a1e660f28b 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -4624,3 +4624,31 @@ exit: USE_PSA_DONE( ); } /* END_CASE */ + +/* 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; + 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( 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 ); + mbedtls_ssl_config_free( &conf ); +} +/* END_CASE */