Merge pull request #931 from AndrzejKurek/clihlo_cookie_pxy_fix
Add a client hello cookie_len overflow test
This commit is contained in:
commit
e0469b5908
9
ChangeLog.d/cookie_parsing_bug.txt
Normal file
9
ChangeLog.d/cookie_parsing_bug.txt
Normal file
@ -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.
|
@ -2270,4 +2270,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 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_misc.h */
|
||||
|
@ -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
|
||||
@ -3149,15 +3149,14 @@ 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 )
|
||||
{
|
||||
size_t sid_len, cookie_len;
|
||||
size_t sid_len, cookie_len, epoch, fragment_offset;
|
||||
unsigned char *p;
|
||||
|
||||
/*
|
||||
@ -3186,26 +3185,55 @@ static int ssl_check_dtls_clihlo_cookie(
|
||||
*
|
||||
* Minimum length is 61 bytes.
|
||||
*/
|
||||
if( in_len < 61 ||
|
||||
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: 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 );
|
||||
}
|
||||
|
||||
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) epoch,
|
||||
(unsigned) fragment_offset ) );
|
||||
return( MBEDTLS_ERR_SSL_DECODE_ERROR );
|
||||
}
|
||||
|
||||
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_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 )
|
||||
return( MBEDTLS_ERR_SSL_DECODE_ERROR );
|
||||
|
||||
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_DECODE_ERROR );
|
||||
}
|
||||
|
||||
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 );
|
||||
}
|
||||
|
||||
@ -3240,8 +3268,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 );
|
||||
}
|
||||
@ -3295,15 +3324,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 )
|
||||
{
|
||||
@ -3481,7 +3508,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 +3515,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.
|
||||
*/
|
||||
|
@ -1189,16 +1189,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] ) );
|
||||
|
||||
/* 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] ) )
|
||||
{
|
||||
MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) );
|
||||
return( MBEDTLS_ERR_SSL_DECODE_ERROR );
|
||||
size_t handshake_len = MBEDTLS_GET_UINT24_BE( buf, 1 );
|
||||
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
|
||||
* 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) handshake_len ) );
|
||||
return( MBEDTLS_ERR_SSL_DECODE_ERROR );
|
||||
}
|
||||
}
|
||||
|
||||
#if defined(MBEDTLS_SSL_PROTO_DTLS)
|
||||
@ -1233,16 +1246,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
|
||||
*/
|
||||
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 */
|
||||
|
@ -3363,3 +3363,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:"16fefd0000000000000000002f010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727dFF":MBEDTLS_ERR_SSL_DECODE_ERROR
|
||||
|
||||
Cookie parsing: one byte overread
|
||||
cookie_parsing:"16fefd0000000000000000002F010000de000000000000011efefd7b7272727272727272727272727272727272727272727272727272727272727d0001":MBEDTLS_ERR_SSL_DECODE_ERROR
|
||||
|
@ -5524,6 +5524,34 @@ void conf_group()
|
||||
}
|
||||
/* 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 */
|
||||
|
||||
/* BEGIN_CASE depends_on:MBEDTLS_TIMING_C:MBEDTLS_HAVE_TIME */
|
||||
void timing_final_delay_accessor( )
|
||||
{
|
||||
|
Loading…
Reference in New Issue
Block a user