From 6c30be8e4b693aba886c57457afc10ec2ef9420b Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Sun, 9 Aug 2020 14:53:10 -0400 Subject: [PATCH 1/6] ssl: call signature verification twice for non-restartable operations Signed-off-by: Andrzej Kurek --- library/ssl_cli.c | 5 +++++ library/ssl_srv.c | 7 +++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index e2c24e28a..86063ebd1 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -3100,6 +3100,11 @@ static int ssl_in_server_key_exchange_parse( mbedtls_ssl_context *ssl, { mbedtls_platform_random_delay(); + if( rs_ctx == NULL ) + { + ret = mbedtls_pk_verify_restartable( peer_pk, + md_alg, hash, hashlen, p, sig_len, rs_ctx ); + } if( ret == 0 ) { #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index ec0c21a6a..34479246c 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4643,13 +4643,16 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) } ret = mbedtls_pk_verify( peer_pk, - md_alg, hash_start, hashlen, - ssl->in_msg + i, sig_len ); + md_alg, hash_start, hashlen, + ssl->in_msg + i, sig_len ); if( ret == 0 ) { mbedtls_platform_random_delay(); + ret = mbedtls_pk_verify( peer_pk, + md_alg, hash_start, hashlen, + ssl->in_msg + i, sig_len ); if( ret == 0 ) { mbedtls_ssl_update_handshake_status( ssl ); From 8ec9e136cfb083e321fb7fcd96250b7cc1eb1a67 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Mon, 10 Aug 2020 00:26:40 -0400 Subject: [PATCH 2/6] ssl_tls: Add a flag indicating that encryption succeeded Protect against encryption skipping by introducing a new flag. Signed-off-by: Andrzej Kurek --- library/ssl_tls.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e00dd0147..89345180c 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2589,6 +2589,9 @@ static void ssl_extract_add_data_from_record( unsigned char* add_data, } } +#define ENCRYPTION_SUCCESS 0xCC +#define ENCRYPTION_FAIL 0xAA + int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, mbedtls_ssl_transform *transform, mbedtls_record *rec, @@ -2601,6 +2604,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, unsigned char add_data[13 + 1 + MBEDTLS_SSL_CID_OUT_LEN_MAX ]; size_t add_data_len; size_t post_avail; + int encryption_status = ENCRYPTION_FAIL; /* The SSL context is only used for debugging purposes! */ #if !defined(MBEDTLS_DEBUG_C) @@ -2793,6 +2797,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } + encryption_status = ENCRYPTION_SUCCESS; } else #endif /* MBEDTLS_ARC4_C || MBEDTLS_CIPHER_NULL_CIPHER */ @@ -2891,6 +2896,8 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } #endif + encryption_status = ENCRYPTION_SUCCESS; + MBEDTLS_SSL_DEBUG_BUF( 4, "after encrypt: tag", data + rec->data_len, transform->taglen ); @@ -2994,6 +3001,9 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } #endif + + encryption_status = ENCRYPTION_SUCCESS; + if( rec->data_len != olen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -3082,7 +3092,11 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= encrypt buf" ) ); - return( 0 ); + if( encryption_status == ENCRYPTION_SUCCESS ) + { + return( 0 ); + } + return( MBEDTLS_ERR_PLATFORM_FAULT_DETECTED ); } int mbedtls_ssl_decrypt_buf( mbedtls_ssl_context const *ssl, From ff51721e990998f78ba3a2931d25f994d7568a7b Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Mon, 10 Aug 2020 07:10:35 -0400 Subject: [PATCH 3/6] ssl_tls: reduce the complexity of encryption validation Signed-off-by: Andrzej Kurek --- library/ssl_tls.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 89345180c..7dfc0afb3 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2589,9 +2589,6 @@ static void ssl_extract_add_data_from_record( unsigned char* add_data, } } -#define ENCRYPTION_SUCCESS 0xCC -#define ENCRYPTION_FAIL 0xAA - int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, mbedtls_ssl_transform *transform, mbedtls_record *rec, @@ -2604,7 +2601,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, unsigned char add_data[13 + 1 + MBEDTLS_SSL_CID_OUT_LEN_MAX ]; size_t add_data_len; size_t post_avail; - int encryption_status = ENCRYPTION_FAIL; + int encryption_status = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; /* The SSL context is only used for debugging purposes! */ #if !defined(MBEDTLS_DEBUG_C) @@ -2774,7 +2771,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } - if( ( ret = mbedtls_cipher_crypt( &transform->cipher_ctx, + if( ( ret = encryption_status = mbedtls_cipher_crypt( &transform->cipher_ctx, transform->iv_enc, transform->ivlen, data, rec->data_len, data, &olen ) ) != 0 ) @@ -2783,7 +2780,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } #else - if( ( ret = mbedtls_cipher_crypt( &transform->cipher_ctx_enc, + if( ( ret = encryption_status = mbedtls_cipher_crypt( &transform->cipher_ctx_enc, transform->iv_enc, transform->ivlen, data, rec->data_len, data, &olen ) ) != 0 ) @@ -2797,7 +2794,6 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } - encryption_status = ENCRYPTION_SUCCESS; } else #endif /* MBEDTLS_ARC4_C || MBEDTLS_CIPHER_NULL_CIPHER */ @@ -2874,7 +2870,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } - if( ( ret = mbedtls_cipher_auth_encrypt( &transform->cipher_ctx, + if( ( ret = encryption_status = mbedtls_cipher_auth_encrypt( &transform->cipher_ctx, iv, transform->ivlen, add_data, add_data_len, /* add data */ data, rec->data_len, /* source */ @@ -2885,7 +2881,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } #else - if( ( ret = mbedtls_cipher_auth_encrypt( &transform->cipher_ctx_enc, + if( ( ret = encryption_status = mbedtls_cipher_auth_encrypt( &transform->cipher_ctx_enc, iv, transform->ivlen, add_data, add_data_len, /* add data */ data, rec->data_len, /* source */ @@ -2896,7 +2892,6 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } #endif - encryption_status = ENCRYPTION_SUCCESS; MBEDTLS_SSL_DEBUG_BUF( 4, "after encrypt: tag", data + rec->data_len, transform->taglen ); @@ -2981,7 +2976,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } - if( ( ret = mbedtls_cipher_crypt( &transform->cipher_ctx, + if( ( ret = encryption_status = mbedtls_cipher_crypt( &transform->cipher_ctx, transform->iv_enc, transform->ivlen, data, rec->data_len, @@ -2991,7 +2986,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, return( ret ); } #else - if( ( ret = mbedtls_cipher_crypt( &transform->cipher_ctx_enc, + if( ( ret = encryption_status = mbedtls_cipher_crypt( &transform->cipher_ctx_enc, transform->iv_enc, transform->ivlen, data, rec->data_len, @@ -3002,8 +2997,6 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, } #endif - encryption_status = ENCRYPTION_SUCCESS; - if( rec->data_len != olen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -3092,7 +3085,7 @@ int mbedtls_ssl_encrypt_buf( mbedtls_ssl_context *ssl, MBEDTLS_SSL_DEBUG_MSG( 2, ( "<= encrypt buf" ) ); - if( encryption_status == ENCRYPTION_SUCCESS ) + if( encryption_status == 0 ) { return( 0 ); } From ef34494d80ed077fb5c57d4e77ec76baf79a8d23 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Mon, 10 Aug 2020 07:11:04 -0400 Subject: [PATCH 4/6] ssl_srv.c: change the initial return variable value Signed-off-by: Andrzej Kurek --- library/ssl_srv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 34479246c..1b29cc839 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4456,7 +4456,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) #else /* !MBEDTLS_KEY_EXCHANGE__CERT_REQ_ALLOWED__ENABLED */ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) { - volatile int ret = MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE; + volatile int ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; size_t i, sig_len; unsigned char hash[48]; unsigned char *hash_start = hash; From f74a86c0b0bd1195496f7a92057724c09317249d Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Sun, 20 Sep 2020 01:57:30 +0200 Subject: [PATCH 5/6] Improve FI resistance of certificate verification in ssl_srv.c Signed-off-by: Andrzej Kurek --- library/ssl_srv.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 1b29cc839..96f7446e3 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -4457,6 +4457,7 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) { volatile int ret = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; + volatile int ret_fi = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; size_t i, sig_len; unsigned char hash[48]; unsigned char *hash_start = hash; @@ -4650,10 +4651,10 @@ static int ssl_parse_certificate_verify( mbedtls_ssl_context *ssl ) { mbedtls_platform_random_delay(); - ret = mbedtls_pk_verify( peer_pk, - md_alg, hash_start, hashlen, - ssl->in_msg + i, sig_len ); - if( ret == 0 ) + ret_fi = mbedtls_pk_verify( peer_pk, + md_alg, hash_start, hashlen, + ssl->in_msg + i, sig_len ); + if( ret == 0 && ret_fi == 0 ) { mbedtls_ssl_update_handshake_status( ssl ); From f4d2c7de3156c94665995956b2744af5ffda70cf Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Sun, 20 Sep 2020 02:03:42 +0200 Subject: [PATCH 6/6] Improve FI resistance of pk verification in ssl_cli.c Signed-off-by: Andrzej Kurek --- library/ssl_cli.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index 86063ebd1..523e62cb0 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2794,6 +2794,7 @@ static int ssl_in_server_key_exchange_parse( mbedtls_ssl_context *ssl, * structural change to provide default flow assumes failure */ volatile int ret = 0; + volatile int ret_fi = MBEDTLS_ERR_PLATFORM_FAULT_DETECTED; unsigned char *p; unsigned char *end; @@ -2931,6 +2932,7 @@ static int ssl_in_server_key_exchange_parse( mbedtls_ssl_context *ssl, #endif /* MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED */ { ((void) ret); + ((void) ret_fi); ((void) p); ((void) end); ((void) ciphersuite_info); @@ -3102,10 +3104,14 @@ static int ssl_in_server_key_exchange_parse( mbedtls_ssl_context *ssl, if( rs_ctx == NULL ) { - ret = mbedtls_pk_verify_restartable( peer_pk, + ret_fi = mbedtls_pk_verify_restartable( peer_pk, md_alg, hash, hashlen, p, sig_len, rs_ctx ); } - if( ret == 0 ) + else + { + ret_fi = 0; + } + if( ret == 0 && ret_fi == 0 ) { #if !defined(MBEDTLS_SSL_KEEP_PEER_CERTIFICATE) /* We don't need the peer's public key anymore. Free it,