From bbc7918b6b8e18184328ce2a10bdf5c78d1332a9 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Wed, 30 May 2018 09:13:21 +0200 Subject: [PATCH] Fixes different off by ones --- ChangeLog | 3 +++ library/ssl_cli.c | 6 +++--- library/ssl_srv.c | 43 ++++++++++++++++++++++++++++++++++++------- library/ssl_tls.c | 9 +++++++++ 4 files changed, 51 insertions(+), 10 deletions(-) diff --git a/ChangeLog b/ChangeLog index 24d20503a..9e592e06f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -13,6 +13,9 @@ Bugfix Changes * Change the shebang line in Perl scripts to look up perl in the PATH. Contributed by fbrosson in #1533. + * Adds of lengths checks in different functions (not a security issue as + original buffer is overgrown) thanks to Philippe Antoine from Catena + cyber. #1663. = mbed TLS 2.1.13 branch released 2018-06-18 diff --git a/library/ssl_cli.c b/library/ssl_cli.c index d3a8ecf72..344f24840 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1141,12 +1141,12 @@ static int ssl_parse_supported_point_formats_ext( mbedtls_ssl_context *ssl, size_t list_size; const unsigned char *p; - list_size = buf[0]; - if( list_size + 1 != len ) + if( len == 0 || (size_t)( buf[0] + 1 ) != len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad server hello message" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_SERVER_HELLO ); } + list_size = buf[0]; p = buf + 1; while( list_size > 0 ) @@ -2494,7 +2494,7 @@ static int ssl_parse_certificate_request( mbedtls_ssl_context *ssl ) * therefore the buffer length at this point must be greater than that * regardless of the actual code path. */ - if( ssl->in_hslen <= mbedtls_ssl_hs_hdr_len( ssl ) + 2 + n ) + if( ssl->in_hslen <= mbedtls_ssl_hs_hdr_len( ssl ) + 3 + n ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate request message" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE_REQUEST ); diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 3ebf9d9f4..c04bb5324 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -97,6 +97,13 @@ static int ssl_parse_servername_ext( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 3, ( "parse ServerName extension" ) ); + if( len < 2 ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } servername_list_size = ( ( buf[0] << 8 ) | ( buf[1] ) ); if( servername_list_size + 2 != len ) { @@ -105,7 +112,7 @@ static int ssl_parse_servername_ext( mbedtls_ssl_context *ssl, } p = buf + 2; - while( servername_list_size > 0 ) + while( servername_list_size > 2 ) { hostname_len = ( ( p[1] << 8 ) | p[2] ); if( hostname_len + 3 > servername_list_size ) @@ -211,6 +218,12 @@ static int ssl_parse_signature_algorithms_ext( mbedtls_ssl_context *ssl, mbedtls_md_type_t md_cur; mbedtls_pk_type_t sig_cur; + if ( len < 2 ) { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } sig_alg_list_size = ( ( buf[0] << 8 ) | ( buf[1] ) ); if( sig_alg_list_size + 2 != len || sig_alg_list_size % 2 != 0 ) @@ -276,6 +289,12 @@ static int ssl_parse_supported_elliptic_curves( mbedtls_ssl_context *ssl, const unsigned char *p; const mbedtls_ecp_curve_info *curve_info, **curves; + if ( len < 2 ) { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } list_size = ( ( buf[0] << 8 ) | ( buf[1] ) ); if( list_size + 2 != len || list_size % 2 != 0 ) @@ -327,12 +346,12 @@ static int ssl_parse_supported_point_formats( mbedtls_ssl_context *ssl, size_t list_size; const unsigned char *p; - list_size = buf[0]; - if( list_size + 1 != len ) + if( len == 0 || (size_t)( buf[0] + 1 ) != len ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); } + list_size = buf[0]; p = buf + 1; while( list_size > 0 ) @@ -1573,10 +1592,16 @@ read_record_header: while( ext_len != 0 ) { - unsigned int ext_id = ( ( ext[0] << 8 ) - | ( ext[1] ) ); - unsigned int ext_size = ( ( ext[2] << 8 ) - | ( ext[3] ) ); + unsigned int ext_id; + unsigned int ext_size; + if ( ext_len < 4 ) { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client hello message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_HELLO ); + } + ext_id = ( ( ext[0] << 8 ) | ( ext[1] ) ); + ext_size = ( ( ext[2] << 8 ) | ( ext[3] ) ); if( ext_size + 4 > ext_len ) { @@ -3166,6 +3191,10 @@ static int ssl_parse_encrypted_pms( mbedtls_ssl_context *ssl, defined(MBEDTLS_SSL_PROTO_TLS1_2) if( ssl->minor_ver != MBEDTLS_SSL_MINOR_VERSION_0 ) { + if ( p + 2 > end ) { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad client key exchange message" ) ); + return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE ); + } if( *p++ != ( ( len >> 8 ) & 0xFF ) || *p++ != ( ( len ) & 0xFF ) ) { diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e3c851eeb..ae963b169 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1132,6 +1132,9 @@ int mbedtls_ssl_psk_derive_premaster( mbedtls_ssl_context *ssl, mbedtls_key_exch * other_secret already set by the ClientKeyExchange message, * and is 48 bytes long */ + if( end - p < 2 ) + return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); + *p++ = 0; *p++ = 48; p += 48; @@ -4461,6 +4464,12 @@ int mbedtls_ssl_parse_certificate( mbedtls_ssl_context *ssl ) while( i < ssl->in_hslen ) { + if ( i + 3 > ssl->in_hslen ) { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) ); + mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECODE_ERROR ); + return( MBEDTLS_ERR_SSL_BAD_HS_CERTIFICATE ); + } if( ssl->in_msg[i] != 0 ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate message" ) );