From 19e59735662864be54cf1ed48b09e7b5655553e0 Mon Sep 17 00:00:00 2001 From: Leonid Rozenboim Date: Mon, 8 Aug 2022 16:52:38 -0700 Subject: [PATCH 1/3] mbedtls_ssl_check_curve prevent potential NULL pointer dereferencing Avoid the shorthand practice of the form 'x = func(foo)->bar' which exposes the code to NULL pointer de-referencing when the 'func()' returns a NULL pointer. The first chunk is for when the curve group code is not recognized by the library, and is cleanly rejected if offered. The second chunk addresses the unlikely case of an internal error: if 'mbedtls_pk_can_do()' returns TRUE, it should rule out 'mbedtls_pk_ec()' returning a NULL, unless there is a regression. Signed-off-by: Leonid Rozenboim --- library/ssl_tls.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 19b8a41351..670e761fa3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4902,7 +4902,14 @@ int mbedtls_ssl_check_curve_tls_id( const mbedtls_ssl_context *ssl, uint16_t tls */ int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id ) { - uint16_t tls_id = mbedtls_ecp_curve_info_from_grp_id( grp_id )->tls_id; + const mbedtls_ecp_curve_info *grp_info = + mbedtls_ecp_curve_info_from_grp_id(grp_id); + + if (grp_info == NULL) + return -1; + + uint16_t tls_id = grp_info->tls_id; + return mbedtls_ssl_check_curve_tls_id( ssl, tls_id ); } #endif /* MBEDTLS_ECP_C */ @@ -6545,14 +6552,27 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl, /* If certificate uses an EC key, make sure the curve is OK. * This is a public key, so it can't be opaque, so can_do() is a good * enough check to ensure pk_ec() is safe to use here. */ - if( mbedtls_pk_can_do( pk, MBEDTLS_PK_ECKEY ) && - mbedtls_ssl_check_curve( ssl, mbedtls_pk_ec( *pk )->grp.id ) != 0 ) + if( mbedtls_pk_can_do( pk, MBEDTLS_PK_ECKEY ) ) { - ssl->session_negotiate->verify_result |= MBEDTLS_X509_BADCERT_BAD_KEY; + /* and in the unlikely case the above assumption no longer holds + * we are making sure that pk_ec() here does not return a NULL + */ + const mbedtls_ecp_keypair *ec = mbedtls_pk_ec( *pk ); + if( ec == NULL ) + { + MBEDTLS_SSL_DEBUG_MSG( 1, ( "mbedtls_pk_ec() returned MULL")); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } - MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (EC key curve)" ) ); - if( ret == 0 ) - ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; + if( mbedtls_ssl_check_curve( ssl, ec->grp.id ) != 0 ) + { + ssl->session_negotiate->verify_result |= + MBEDTLS_X509_BADCERT_BAD_KEY; + + MBEDTLS_SSL_DEBUG_MSG( 1, ( "bad certificate (EC key curve)" ) ); + if( ret == 0 ) + ret = MBEDTLS_ERR_SSL_BAD_CERTIFICATE; + } } } #endif /* MBEDTLS_ECP_C */ From 20c11373507433e2c82e27e846154223fc028eda Mon Sep 17 00:00:00 2001 From: Tom Cosgrove <81633263+tom-cosgrove-arm@users.noreply.github.com> Date: Wed, 24 Aug 2022 15:06:13 +0100 Subject: [PATCH 2/3] Fix coding style Signed-off-by: Tom Cosgrove Co-authored-by: Dave Rodgman --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 670e761fa3..22c884cba3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -6560,7 +6560,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl, const mbedtls_ecp_keypair *ec = mbedtls_pk_ec( *pk ); if( ec == NULL ) { - MBEDTLS_SSL_DEBUG_MSG( 1, ( "mbedtls_pk_ec() returned MULL")); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "mbedtls_pk_ec() returned NULL" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } From bcc13c943fce49d9e7ea94100972684cfc8f22fb Mon Sep 17 00:00:00 2001 From: Tom Cosgrove <81633263+tom-cosgrove-arm@users.noreply.github.com> Date: Wed, 24 Aug 2022 15:08:16 +0100 Subject: [PATCH 3/3] Add further missing whitespaces inside parentheses Signed-off-by: Tom Cosgrove Co-authored-by: Dave Rodgman --- library/ssl_tls.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 22c884cba3..665beecb32 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4903,9 +4903,9 @@ int mbedtls_ssl_check_curve_tls_id( const mbedtls_ssl_context *ssl, uint16_t tls int mbedtls_ssl_check_curve( const mbedtls_ssl_context *ssl, mbedtls_ecp_group_id grp_id ) { const mbedtls_ecp_curve_info *grp_info = - mbedtls_ecp_curve_info_from_grp_id(grp_id); + mbedtls_ecp_curve_info_from_grp_id( grp_id ); - if (grp_info == NULL) + if ( grp_info == NULL ) return -1; uint16_t tls_id = grp_info->tls_id;