From 79bb19f7022ee71b7dd1befd98de7faa03bf83e6 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 2 Nov 2022 14:07:46 +0000 Subject: [PATCH 01/10] Remove redundant checks for renegotiation Signed-off-by: David Horstmann --- library/ssl_cli.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 5df2758da0..4817bd6413 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -1089,11 +1089,6 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) * RFC 5077 section 3.4: "When presenting a ticket, the client MAY * generate and include a Session ID in the TLS ClientHello." */ - renegotiating = 0; -#if defined(MBEDTLS_SSL_RENEGOTIATION) - if( ssl->renego_status != MBEDTLS_SSL_INITIAL_HANDSHAKE ) - renegotiating = 1; -#endif if( !renegotiating ) { if( ssl->session_negotiate->ticket != NULL && @@ -1209,11 +1204,6 @@ static int ssl_write_client_hello( mbedtls_ssl_context *ssl ) /* * Add TLS_EMPTY_RENEGOTIATION_INFO_SCSV */ - renegotiating = 0; -#if defined(MBEDTLS_SSL_RENEGOTIATION) - if( ssl->renego_status != MBEDTLS_SSL_INITIAL_HANDSHAKE ) - renegotiating = 1; -#endif if( !renegotiating ) { MBEDTLS_SSL_DEBUG_MSG( 3, ( "adding EMPTY_RENEGOTIATION_INFO_SCSV" ) ); From dbb6f08c3fe57e84a17c72ac13ad05514dd7c926 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 2 Nov 2022 15:33:31 +0000 Subject: [PATCH 02/10] Eliminate bad_params variable Signed-off-by: David Horstmann --- library/ssl_cli.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 4817bd6413..41ea83fa15 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2694,17 +2694,14 @@ static int ssl_check_server_ecdh_params( const mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDH curve: %s", curve_info->name ) ); - int bad_params = 0; #if defined(MBEDTLS_ECP_C) if( mbedtls_ssl_check_curve( ssl, grp_id ) != 0 ) - bad_params = 1; + return( -1 ); #else if( ssl->handshake->ecdh_ctx.grp.nbits < 163 || ssl->handshake->ecdh_ctx.grp.nbits > 521 ) - bad_params = 1; -#endif - if( bad_params ) return( -1 ); +#endif MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, MBEDTLS_DEBUG_ECDH_QP ); From 0448de58d7e2d54167914a31689e9e9bb6eda701 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 2 Nov 2022 18:05:24 +0000 Subject: [PATCH 03/10] Simplify logic in ssl_cli.c Signed-off-by: David Horstmann --- library/ssl_cli.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 41ea83fa15..6e4ca3b0f5 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3454,20 +3454,18 @@ start_processing: if( ( ret = mbedtls_pk_verify_restartable( peer_pk, md_alg, hash, hashlen, p, sig_len, rs_ctx ) ) != 0 ) { - int send_alert_msg = 1; -#if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) - send_alert_msg = ( ret != MBEDTLS_ERR_ECP_IN_PROGRESS ); -#endif - if( send_alert_msg ) - mbedtls_ssl_send_alert_message( - ssl, - MBEDTLS_SSL_ALERT_LEVEL_FATAL, - MBEDTLS_SSL_ALERT_MSG_DECRYPT_ERROR ); - MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify", ret ); #if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) if( ret == MBEDTLS_ERR_ECP_IN_PROGRESS ) - ret = MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS; + { + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify", ret ); + return( MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS ); + } #endif + mbedtls_ssl_send_alert_message( + ssl, + MBEDTLS_SSL_ALERT_LEVEL_FATAL, + MBEDTLS_SSL_ALERT_MSG_DECRYPT_ERROR ); + MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify", ret ); return( ret ); } From 0955f82642daee37a6badd4ff9a30da470cca5f8 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 7 Nov 2022 14:24:21 +0000 Subject: [PATCH 04/10] Tidy up compression logic with auxiliary function This refactors some logic in ssl_cli.c, removing some previously added technical debt. Signed-off-by: David Horstmann --- library/ssl_cli.c | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 6e4ca3b0f5..9cce2e6bc3 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2055,6 +2055,29 @@ static int ssl_parse_hello_verify_request( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_PROTO_DTLS */ +static int is_compression_ok( mbedtls_ssl_context *ssl, unsigned char comp ) +{ + int accept_comp = 1; + + /* Suppress warnings in some configurations */ + ( void )ssl; +#if defined(MBEDTLS_ZLIB_SUPPORT) + /* See comments in ssl_write_client_hello() */ +#if defined(MBEDTLS_SSL_PROTO_DTLS) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + accept_comp = 0; +#endif + + if( comp != MBEDTLS_SSL_COMPRESS_NULL && + comp != MBEDTLS_SSL_COMPRESS_DEFLATE ) + accept_comp = 0; +#else /* MBEDTLS_ZLIB_SUPPORT */ + if( comp != MBEDTLS_SSL_COMPRESS_NULL ) + accept_comp = 0; +#endif/* MBEDTLS_ZLIB_SUPPORT */ + return accept_comp; +} + MBEDTLS_CHECK_RETURN_CRITICAL static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) { @@ -2063,9 +2086,6 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) size_t ext_len; unsigned char *buf, *ext; unsigned char comp; -#if defined(MBEDTLS_ZLIB_SUPPORT) - int accept_comp; -#endif #if defined(MBEDTLS_SSL_RENEGOTIATION) int renegotiation_info_seen = 0; #endif @@ -2234,23 +2254,7 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) */ comp = buf[37 + n]; - int bad_comp = 0; -#if defined(MBEDTLS_ZLIB_SUPPORT) - /* See comments in ssl_write_client_hello() */ - accept_comp = 1; -#if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) - accept_comp = 0; -#endif - - if( comp != MBEDTLS_SSL_COMPRESS_NULL && - ( comp != MBEDTLS_SSL_COMPRESS_DEFLATE || accept_comp == 0 ) ) - bad_comp = 1; -#else /* MBEDTLS_ZLIB_SUPPORT */ - if( comp != MBEDTLS_SSL_COMPRESS_NULL ) - bad_comp = 1; -#endif/* MBEDTLS_ZLIB_SUPPORT */ - if( bad_comp ) + if( !is_compression_ok(ssl, comp) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "server hello, bad compression: %d", comp ) ); From 9fc2f959b3c21638f5976baf1c34f1a02a729f14 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 7 Nov 2022 14:28:30 +0000 Subject: [PATCH 05/10] Change 0-checks to NULL-checks in ecp.c Signed-off-by: David Horstmann --- library/ecp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 402d5dedbb..80adc55c8b 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2051,7 +2051,7 @@ static int ecp_mul_comb_core( const mbedtls_ecp_group *grp, mbedtls_ecp_point *R int have_rng = 1; #if defined(MBEDTLS_ECP_NO_INTERNAL_RNG) - if( f_rng == 0 ) + if( f_rng == NULL ) have_rng = 0; #endif if( have_rng ) @@ -2190,7 +2190,7 @@ final_norm: */ int have_rng = 1; #if defined(MBEDTLS_ECP_NO_INTERNAL_RNG) - if( f_rng == 0 ) + if( f_rng == NULL ) have_rng = 0; #endif if( have_rng ) From bcc18f2becec68270a759924aeca9f83e104f212 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 7 Nov 2022 14:41:13 +0000 Subject: [PATCH 06/10] Simplify PSA fallback logic in ssl_ticket.c Signed-off-by: David Horstmann --- library/ssl_ticket.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index e6abe850da..8a57789f10 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -148,16 +148,22 @@ int mbedtls_ssl_ticket_setup( mbedtls_ssl_ticket_context *ctx, int do_mbedtls_cipher_setup = 1; #if defined(MBEDTLS_USE_PSA_CRYPTO) - do_mbedtls_cipher_setup = 0; - ret = mbedtls_cipher_setup_psa( &ctx->keys[0].ctx, cipher_info, TICKET_AUTH_TAG_BYTES ); - if( ret != 0 && ret != MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ) - return( ret ); - /* We don't yet expect to support all ciphers through PSA, - * so allow fallback to ordinary mbedtls_cipher_setup(). */ - if( ret == MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ) - do_mbedtls_cipher_setup = 1; + + switch( ret ) + { + case 0: + do_mbedtls_cipher_setup = 0; + break; + case MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE: + /* We don't yet expect to support all ciphers through PSA, + * so allow fallback to ordinary mbedtls_cipher_setup(). */ + do_mbedtls_cipher_setup = 1; + break; + default: + return( ret ); + } #endif /* MBEDTLS_USE_PSA_CRYPTO */ if( do_mbedtls_cipher_setup ) if( ( ret = mbedtls_cipher_setup( &ctx->keys[0].ctx, cipher_info ) ) From 08a37516ffdbcd27b2748350492c91693f231ce0 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 7 Nov 2022 15:55:00 +0000 Subject: [PATCH 07/10] Minor style fixes to ssl_cli.c Signed-off-by: David Horstmann --- library/ssl_cli.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 9cce2e6bc3..4bef15cd4e 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2060,7 +2060,7 @@ static int is_compression_ok( mbedtls_ssl_context *ssl, unsigned char comp ) int accept_comp = 1; /* Suppress warnings in some configurations */ - ( void )ssl; + (void) ssl; #if defined(MBEDTLS_ZLIB_SUPPORT) /* See comments in ssl_write_client_hello() */ #if defined(MBEDTLS_SSL_PROTO_DTLS) @@ -2254,7 +2254,7 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) */ comp = buf[37 + n]; - if( !is_compression_ok(ssl, comp) ) + if( !is_compression_ok( ssl, comp ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "server hello, bad compression: %d", comp ) ); From b410566ba7b4ef9987161976133a9d8abde77c72 Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Mon, 7 Nov 2022 16:33:57 +0000 Subject: [PATCH 08/10] Reverse logic for compression in ssl_cli.c Change is_compression_ok() to is_compression_bad() for more semantics that are a better match for what's really going on in the case of no compression support. Signed-off-by: David Horstmann --- library/ssl_cli.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 4bef15cd4e..e3aefc66e1 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2055,9 +2055,9 @@ static int ssl_parse_hello_verify_request( mbedtls_ssl_context *ssl ) } #endif /* MBEDTLS_SSL_PROTO_DTLS */ -static int is_compression_ok( mbedtls_ssl_context *ssl, unsigned char comp ) +static int is_compression_bad( mbedtls_ssl_context *ssl, unsigned char comp ) { - int accept_comp = 1; + int bad_comp = 0; /* Suppress warnings in some configurations */ (void) ssl; @@ -2065,17 +2065,17 @@ static int is_compression_ok( mbedtls_ssl_context *ssl, unsigned char comp ) /* See comments in ssl_write_client_hello() */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) - accept_comp = 0; + bad_comp = 1; #endif if( comp != MBEDTLS_SSL_COMPRESS_NULL && comp != MBEDTLS_SSL_COMPRESS_DEFLATE ) - accept_comp = 0; + bad_comp = 1; #else /* MBEDTLS_ZLIB_SUPPORT */ if( comp != MBEDTLS_SSL_COMPRESS_NULL ) - accept_comp = 0; + bad_comp = 1; #endif/* MBEDTLS_ZLIB_SUPPORT */ - return accept_comp; + return bad_comp; } MBEDTLS_CHECK_RETURN_CRITICAL @@ -2254,7 +2254,7 @@ static int ssl_parse_server_hello( mbedtls_ssl_context *ssl ) */ comp = buf[37 + n]; - if( !is_compression_ok( ssl, comp ) ) + if( is_compression_bad( ssl, comp ) ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "server hello, bad compression: %d", comp ) ); From 1d00c3dea6eeb1805b5839222713ffea414c21da Mon Sep 17 00:00:00 2001 From: aditya-deshpande-arm <112866256+aditya-deshpande-arm@users.noreply.github.com> Date: Tue, 8 Nov 2022 16:08:13 +0000 Subject: [PATCH 09/10] Add comments after #endif Signed-off-by: David Horstmann --- library/ssl_cli.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e3aefc66e1..788c190de4 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2687,7 +2687,7 @@ static int ssl_check_server_ecdh_params( const mbedtls_ssl_context *ssl ) grp_id = ssl->handshake->ecdh_ctx.grp.id; #else grp_id = ssl->handshake->ecdh_ctx.grp_id; -#endif +#endif /* MBEDTLS_ECDH_LEGACY_CONTEXT */ curve_info = mbedtls_ecp_curve_info_from_grp_id( grp_id ); if( curve_info == NULL ) @@ -2705,7 +2705,7 @@ static int ssl_check_server_ecdh_params( const mbedtls_ssl_context *ssl ) if( ssl->handshake->ecdh_ctx.grp.nbits < 163 || ssl->handshake->ecdh_ctx.grp.nbits > 521 ) return( -1 ); -#endif +#endif /* MBEDTLS_ECP_C */ MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, MBEDTLS_DEBUG_ECDH_QP ); @@ -3453,7 +3453,7 @@ start_processing: #if defined(MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED) if( ssl->handshake->ecrs_enabled ) rs_ctx = &ssl->handshake->ecrs_ctx.pk; -#endif +#endif /* MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED */ if( ( ret = mbedtls_pk_verify_restartable( peer_pk, md_alg, hash, hashlen, p, sig_len, rs_ctx ) ) != 0 ) @@ -3464,7 +3464,7 @@ start_processing: MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_pk_verify", ret ); return( MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS ); } -#endif +#endif /* MBEDTLS_SSL_ECP_RESTARTABLE_ENABLED */ mbedtls_ssl_send_alert_message( ssl, MBEDTLS_SSL_ALERT_LEVEL_FATAL, From da2fe26db7dc66fcab316e0e0f39eb89052cdc9f Mon Sep 17 00:00:00 2001 From: David Horstmann Date: Wed, 9 Nov 2022 14:35:23 +0000 Subject: [PATCH 10/10] Fix incorrect condition in is_compression_bad() The transport is allowed to be MBEDTLS_SSL_TRANSPORT_DATAGRAM when the compression is MBEDTLS_SSL_COMPRESS_NULL. Signed-off-by: David Horstmann --- library/ssl_cli.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 788c190de4..3475aa42f4 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2064,7 +2064,8 @@ static int is_compression_bad( mbedtls_ssl_context *ssl, unsigned char comp ) #if defined(MBEDTLS_ZLIB_SUPPORT) /* See comments in ssl_write_client_hello() */ #if defined(MBEDTLS_SSL_PROTO_DTLS) - if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM ) + if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && + comp != MBEDTLS_SSL_COMPRESS_NULL ) bad_comp = 1; #endif