From 969ccc62892b3a1c0f637e28fcef7859c9e877dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 26 Mar 2014 19:53:25 +0100 Subject: [PATCH] Fix length checking of various ClientKeyExchange's --- ChangeLog | 5 +++++ library/ecdh.c | 11 ++++++++++- library/ssl_srv.c | 50 ++++++++++++++++++++++++++++------------------- 3 files changed, 45 insertions(+), 21 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2deb77a93..57400a80f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ PolarSSL ChangeLog (Sorted per branch, date) += PolarSSL 1.3 branch + +Bugfix + * The length of various ClientKeyExchange messages was not properly checked. + = PolarSSL 1.3.5 released on 2014-03-26 Features * HMAC-DRBG as a separate module diff --git a/library/ecdh.c b/library/ecdh.c index cd656904e..e35602db8 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -218,10 +218,19 @@ int ecdh_make_public( ecdh_context *ctx, size_t *olen, int ecdh_read_public( ecdh_context *ctx, const unsigned char *buf, size_t blen ) { + int ret; + const unsigned char *p = buf; + if( ctx == NULL ) return( POLARSSL_ERR_ECP_BAD_INPUT_DATA ); - return ecp_tls_read_point( &ctx->grp, &ctx->Qp, &buf, blen ); + if( ( ret = ecp_tls_read_point( &ctx->grp, &ctx->Qp, &p, blen ) ) != 0 ) + return( ret ); + + if( (size_t)( p - buf ) != blen ) + return( POLARSSL_ERR_ECP_BAD_INPUT_DATA ); + + return( 0 ); } /* diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 0241b43b7..16fb264dc 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -2394,19 +2394,20 @@ static int ssl_parse_client_dh_public( ssl_context *ssl, unsigned char **p, n = ( (*p)[0] << 8 ) | (*p)[1]; *p += 2; - if( n < 1 || n > ssl->handshake->dhm_ctx.len || *p + n > end ) + if( *p + n > end ) { SSL_DEBUG_MSG( 1, ( "bad client key exchange message" ) ); return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE ); } - if( ( ret = dhm_read_public( &ssl->handshake->dhm_ctx, - *p, n ) ) != 0 ) + if( ( ret = dhm_read_public( &ssl->handshake->dhm_ctx, *p, n ) ) != 0 ) { SSL_DEBUG_RET( 1, "dhm_read_public", ret ); return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_RP ); } + *p += n; + SSL_DEBUG_MPI( 3, "DHM: GY", &ssl->handshake->dhm_ctx.GY ); return( ret ); @@ -2583,7 +2584,7 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) if( ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_DHE_RSA ) { unsigned char *p = ssl->in_msg + 4; - unsigned char *end = ssl->in_msg + ssl->in_msglen; + unsigned char *end = ssl->in_msg + ssl->in_hslen; if( ( ret = ssl_parse_client_dh_public( ssl, &p, end ) ) != 0 ) { @@ -2591,6 +2592,12 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) return( ret ); } + if( p != end ) + { + SSL_DEBUG_MSG( 1, ( "bad client key exchange" ) ); + return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE ); + } + ssl->handshake->pmslen = ssl->handshake->dhm_ctx.len; if( ( ret = dhm_calc_secret( &ssl->handshake->dhm_ctx, @@ -2615,17 +2622,8 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_ECDH_RSA || ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_ECDH_ECDSA ) { - size_t n = ssl->in_msg[3]; - - if( n < 1 || n > mpi_size( &ssl->handshake->ecdh_ctx.grp.P ) * 2 + 2 || - n + 4 != ssl->in_hslen ) - { - SSL_DEBUG_MSG( 1, ( "bad client key exchange message" ) ); - return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE ); - } - if( ( ret = ecdh_read_public( &ssl->handshake->ecdh_ctx, - ssl->in_msg + 4, n ) ) != 0 ) + ssl->in_msg + 4, ssl->in_hslen - 4 ) ) != 0 ) { SSL_DEBUG_RET( 1, "ecdh_read_public", ret ); return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_RP ); @@ -2654,7 +2652,7 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) if( ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_PSK ) { unsigned char *p = ssl->in_msg + 4; - unsigned char *end = ssl->in_msg + ssl->in_msglen; + unsigned char *end = ssl->in_msg + ssl->in_hslen; if( ( ret = ssl_parse_client_psk_identity( ssl, &p, end ) ) != 0 ) { @@ -2662,6 +2660,12 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) return( ret ); } + if( p != end ) + { + SSL_DEBUG_MSG( 1, ( "bad client key exchange" ) ); + return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE ); + } + if( ( ret = ssl_psk_derive_premaster( ssl, ciphersuite_info->key_exchange ) ) != 0 ) { @@ -2675,7 +2679,7 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) if( ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_RSA_PSK ) { unsigned char *p = ssl->in_msg + 4; - unsigned char *end = ssl->in_msg + ssl->in_msglen; + unsigned char *end = ssl->in_msg + ssl->in_hslen; if( ( ret = ssl_parse_client_psk_identity( ssl, &p, end ) ) != 0 ) { @@ -2702,7 +2706,7 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) if( ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_DHE_PSK ) { unsigned char *p = ssl->in_msg + 4; - unsigned char *end = ssl->in_msg + ssl->in_msglen; + unsigned char *end = ssl->in_msg + ssl->in_hslen; if( ( ret = ssl_parse_client_psk_identity( ssl, &p, end ) ) != 0 ) { @@ -2715,6 +2719,12 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) return( ret ); } + if( p != end ) + { + SSL_DEBUG_MSG( 1, ( "bad client key exchange" ) ); + return( POLARSSL_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE ); + } + if( ( ret = ssl_psk_derive_premaster( ssl, ciphersuite_info->key_exchange ) ) != 0 ) { @@ -2728,7 +2738,7 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) if( ciphersuite_info->key_exchange == POLARSSL_KEY_EXCHANGE_ECDHE_PSK ) { unsigned char *p = ssl->in_msg + 4; - unsigned char *end = ssl->in_msg + ssl->in_msglen; + unsigned char *end = ssl->in_msg + ssl->in_hslen; if( ( ret = ssl_parse_client_psk_identity( ssl, &p, end ) ) != 0 ) { @@ -2759,10 +2769,10 @@ static int ssl_parse_client_key_exchange( ssl_context *ssl ) { if( ( ret = ssl_parse_encrypted_pms( ssl, ssl->in_msg + 4, - ssl->in_msg + ssl->in_msglen, + ssl->in_msg + ssl->in_hslen, 0 ) ) != 0 ) { - SSL_DEBUG_RET( 1, ( "ssl_parse_parse_ecrypted_pms_secret" ), ret ); + SSL_DEBUG_RET( 1, ( "ssl_parse_parse_encrypted_pms_secret" ), ret ); return( ret ); } }