Merge pull request #944 from AndrzejKurek/clihlo_cookie_pxy_fix_2_28
[Backport 2.28] Add a client hello cookie_len overflow test
diff --git a/ChangeLog.d/cookie_parsing_bug.txt b/ChangeLog.d/cookie_parsing_bug.txt
new file mode 100644
index 0000000..1c25f39
--- /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 f50cf9f..83047c5 100644
--- a/include/mbedtls/ssl_internal.h
+++ b/include/mbedtls/ssl_internal.h
@@ -1306,4 +1306,12 @@
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 5a4574d..dfcdc93 100644
--- a/library/ssl_msg.c
+++ b/library/ssl_msg.c
@@ -3229,8 +3229,8 @@
#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 @@
* 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 @@
*
* 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 @@
/* 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 @@
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 @@
/*
* 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 @@
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 bce51cd..1733ec9 100644
--- a/library/ssl_srv.c
+++ b/library/ssl_srv.c
@@ -1604,11 +1604,19 @@
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 @@
* 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 0f32671..0e97e6f 100644
--- a/tests/suites/test_suite_ssl.data
+++ b/tests/suites/test_suite_ssl.data
@@ -10689,3 +10689,21 @@
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 8318df0..a1e660f 100644
--- a/tests/suites/test_suite_ssl.function
+++ b/tests/suites/test_suite_ssl.function
@@ -4624,3 +4624,31 @@
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 */