From 27485ee5c96d497b1084e0cef6931b14529950ff Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Mon, 7 Aug 2023 15:00:57 -0700 Subject: [PATCH 01/15] Fix S2N blocked issue --- include/aws/io/io.h | 2 ++ source/io.c | 4 ++++ source/s2n/s2n_tls_channel_handler.c | 17 ++++++++++++++--- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/include/aws/io/io.h b/include/aws/io/io.h index 0ce3f2bc8..fc9677fdb 100644 --- a/include/aws/io/io.h +++ b/include/aws/io/io.h @@ -250,6 +250,8 @@ enum aws_io_errors { AWS_IO_STREAM_SEEK_UNSUPPORTED, AWS_IO_STREAM_GET_LENGTH_UNSUPPORTED, + AWS_IO_TLS_ERROR_READ_FAILURE, + AWS_IO_ERROR_END_RANGE = AWS_ERROR_ENUM_END_RANGE(AWS_C_IO_PACKAGE_ID), AWS_IO_INVALID_FILE_HANDLE = AWS_ERROR_INVALID_FILE_HANDLE, }; diff --git a/source/io.c b/source/io.c index 106ec0f97..4e5ecdfa3 100644 --- a/source/io.c +++ b/source/io.c @@ -300,6 +300,10 @@ static struct aws_error_info s_errors[] = { AWS_DEFINE_ERROR_INFO_IO( AWS_IO_STREAM_GET_LENGTH_UNSUPPORTED, "Get length is not supported in the underlying I/O source."), + AWS_DEFINE_ERROR_INFO_IO( + AWS_IO_TLS_ERROR_READ_FAILURE, + "Failed to read during TLS"), + }; /* clang-format on */ diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index bc30ca73b..19addc238 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -523,7 +523,7 @@ static int s_s2n_handler_process_read_message( AWS_LOGF_TRACE( AWS_LS_IO_TLS, "id=%p: Downstream window %llu", (void *)handler, (unsigned long long)downstream_window); - while (processed < downstream_window && blocked == S2N_NOT_BLOCKED) { + while (processed < downstream_window) { struct aws_io_message *outgoing_read_message = aws_channel_acquire_message_from_pool( slot->channel, AWS_IO_MESSAGE_APPLICATION_DATA, downstream_window - processed); @@ -557,8 +557,19 @@ static int s_s2n_handler_process_read_message( } if (read < 0) { - aws_mem_release(outgoing_read_message->allocator, outgoing_read_message); - continue; + /* the socket blocked so exit from the loop */ + if (s2n_error_get_type(s2n_errno) == S2N_ERR_T_BLOCKED) { + break; + } + + AWS_LOGF_ERROR( + AWS_LS_IO_TLS, + "id=%p: S2N failed to read with error: %s (%s)", + (void *)handler, + s2n_strerror(s2n_errno, "EN"), + s2n_strerror_debug(s2n_errno, "EN")); + aws_channel_shutdown(slot->channel, AWS_IO_TLS_ERROR_READ_FAILURE); + return aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); }; processed += read; From b92eefdfa90cd6cc05d44ad9453657ebddeb3818 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Mon, 7 Aug 2023 15:01:26 -0700 Subject: [PATCH 02/15] Add comment --- source/s2n/s2n_tls_channel_handler.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index 19addc238..339f3afa3 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -562,6 +562,7 @@ static int s_s2n_handler_process_read_message( break; } + /* the socket returned a fatal error so shut down */ AWS_LOGF_ERROR( AWS_LS_IO_TLS, "id=%p: S2N failed to read with error: %s (%s)", From ff9f232488c7b31fe7a2433a97be17318c4803da Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Mon, 7 Aug 2023 15:58:08 -0700 Subject: [PATCH 03/15] fix mem leak --- source/s2n/s2n_tls_channel_handler.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index 339f3afa3..035c57e9b 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -557,6 +557,8 @@ static int s_s2n_handler_process_read_message( } if (read < 0) { + aws_mem_release(outgoing_read_message->allocator, outgoing_read_message); + /* the socket blocked so exit from the loop */ if (s2n_error_get_type(s2n_errno) == S2N_ERR_T_BLOCKED) { break; From 8ad78b987f0e241f8939b75d4310a64afaeaed55 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 12:10:10 -0700 Subject: [PATCH 04/15] Update error message --- source/io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/io.c b/source/io.c index 4e5ecdfa3..b54e44d75 100644 --- a/source/io.c +++ b/source/io.c @@ -302,7 +302,7 @@ static struct aws_error_info s_errors[] = { "Get length is not supported in the underlying I/O source."), AWS_DEFINE_ERROR_INFO_IO( AWS_IO_TLS_ERROR_READ_FAILURE, - "Failed to read during TLS"), + "Failure during TLS read."), }; /* clang-format on */ From 597b838cdacd3b185a0d3bb41290fd62194e53e5 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 12:20:44 -0700 Subject: [PATCH 05/15] raise tls error read failure for windows as well --- source/windows/secure_channel_tls_handler.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/windows/secure_channel_tls_handler.c b/source/windows/secure_channel_tls_handler.c index 415da7034..915f8c7de 100644 --- a/source/windows/secure_channel_tls_handler.c +++ b/source/windows/secure_channel_tls_handler.c @@ -1144,8 +1144,7 @@ static int s_do_application_data_decrypt(struct aws_channel_handler *handler) { } else { AWS_LOGF_ERROR( AWS_LS_IO_TLS, "id=%p: Error decrypting message. SECURITY_STATUS is %d.", (void *)handler, (int)status); - int aws_error = s_determine_sspi_error(status); - aws_raise_error(aws_error); + aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); } } while (sc_handler->read_extra); From cb899650e61685410efc133f69ecc4117360e0b0 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 15:15:17 -0700 Subject: [PATCH 06/15] bail out if read is complete --- source/s2n/s2n_tls_channel_handler.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index 035c57e9b..18005f7ff 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -575,6 +575,7 @@ static int s_s2n_handler_process_read_message( return aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); }; + // if read > 0 processed += read; outgoing_read_message->message_data.len = (size_t)read; @@ -587,6 +588,12 @@ static int s_s2n_handler_process_read_message( } else { aws_mem_release(outgoing_read_message->allocator, outgoing_read_message); } + + if (blocked == S2N_NOT_BLOCKED) { + /* read is complete so shutdown with success */ + aws_channel_shutdown(slot->channel, AWS_OP_SUCCESS); + return AWS_OP_SUCCESS; + } } AWS_LOGF_TRACE( From 9a70fc19367d3c1bbad55e1a26dea7200ec8395e Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 15:40:52 -0700 Subject: [PATCH 07/15] PR feedback --- source/darwin/secure_transport_tls_channel_handler.c | 5 ++--- source/windows/secure_channel_tls_handler.c | 2 ++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/source/darwin/secure_transport_tls_channel_handler.c b/source/darwin/secure_transport_tls_channel_handler.c index 23e49ad25..5846807e4 100644 --- a/source/darwin/secure_transport_tls_channel_handler.c +++ b/source/darwin/secure_transport_tls_channel_handler.c @@ -639,9 +639,8 @@ static int s_process_read_message( (int)status); if (status != errSSLClosedGraceful) { - aws_raise_error(AWS_IO_TLS_ERROR_ALERT_RECEIVED); - aws_channel_shutdown( - secure_transport_handler->parent_slot->channel, AWS_IO_TLS_ERROR_ALERT_RECEIVED); + aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); + aws_channel_shutdown(secure_transport_handler->parent_slot->channel, AWS_IO_TLS_ERROR_READ_FAILURE); } else { AWS_LOGF_TRACE(AWS_LS_IO_TLS, "id=%p: connection shutting down gracefully.", (void *)handler); aws_channel_shutdown(secure_transport_handler->parent_slot->channel, AWS_ERROR_SUCCESS); diff --git a/source/windows/secure_channel_tls_handler.c b/source/windows/secure_channel_tls_handler.c index 915f8c7de..4ead6c31a 100644 --- a/source/windows/secure_channel_tls_handler.c +++ b/source/windows/secure_channel_tls_handler.c @@ -1111,6 +1111,8 @@ static int s_do_application_data_decrypt(struct aws_channel_handler *handler) { "id=%p: Decrypt ended exactly on the end of the record, resetting buffer.", (void *)handler); } + } else { + aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); } } /* SEC_E_INCOMPLETE_MESSAGE means the message we tried to decrypt isn't a full record and we need to From 1b1f4d998f941c3dca5531467b8da15541bca5e4 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 15:55:44 -0700 Subject: [PATCH 08/15] break --- source/s2n/s2n_tls_channel_handler.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index 18005f7ff..aa4a4e66c 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -591,8 +591,7 @@ static int s_s2n_handler_process_read_message( if (blocked == S2N_NOT_BLOCKED) { /* read is complete so shutdown with success */ - aws_channel_shutdown(slot->channel, AWS_OP_SUCCESS); - return AWS_OP_SUCCESS; + break; } } From c7126b73d8acc5d295c4e790398c1c72be10ac4c Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 15:56:03 -0700 Subject: [PATCH 09/15] remove comment --- source/s2n/s2n_tls_channel_handler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index aa4a4e66c..9b2134390 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -590,7 +590,7 @@ static int s_s2n_handler_process_read_message( } if (blocked == S2N_NOT_BLOCKED) { - /* read is complete so shutdown with success */ + /* read is complete */ break; } } From c486bb9ebd6e0187acff4ef20a267d076f62cd88 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 16:58:43 -0700 Subject: [PATCH 10/15] Add a log statement --- source/windows/secure_channel_tls_handler.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source/windows/secure_channel_tls_handler.c b/source/windows/secure_channel_tls_handler.c index 4ead6c31a..28a52f914 100644 --- a/source/windows/secure_channel_tls_handler.c +++ b/source/windows/secure_channel_tls_handler.c @@ -1053,7 +1053,7 @@ static int s_do_application_data_decrypt(struct aws_channel_handler *handler) { int error = AWS_OP_ERR; /* when we get an Extra buffer we have to move the pointer and replay the buffer, so we loop until we don't have any extra buffers left over, in the last phase, we then go ahead and send the output. This state function will - always say BLOCKED_ON_READ or SUCCESS. There will never be left over reads.*/ + always say BLOCKED_ON_READ, AWS_IO_TLS_ERROR_READ_FAILURE or SUCCESS. There will never be left over reads.*/ do { error = AWS_OP_ERR; /* 4 buffers are needed, only one is input, the others get zeroed out for the output operation. */ @@ -1112,6 +1112,11 @@ static int s_do_application_data_decrypt(struct aws_channel_handler *handler) { (void *)handler); } } else { + AWS_LOGF_ERROR( + AWS_LS_IO_TLS, + "id=%p: Error decrypting message. Unexpected type of output buffer. SECURITY_STATUS is %d.", + (void *)handler, + (int)status); aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); } } From 570ad9d68a5b8ea7a6f87ef9af4a1837e7310639 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Tue, 8 Aug 2023 16:59:37 -0700 Subject: [PATCH 11/15] remove status it will always be OK --- source/windows/secure_channel_tls_handler.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/source/windows/secure_channel_tls_handler.c b/source/windows/secure_channel_tls_handler.c index 28a52f914..33346bf21 100644 --- a/source/windows/secure_channel_tls_handler.c +++ b/source/windows/secure_channel_tls_handler.c @@ -1114,9 +1114,8 @@ static int s_do_application_data_decrypt(struct aws_channel_handler *handler) { } else { AWS_LOGF_ERROR( AWS_LS_IO_TLS, - "id=%p: Error decrypting message. Unexpected type of output buffer. SECURITY_STATUS is %d.", - (void *)handler, - (int)status); + "id=%p: Error decrypting message. Unexpected type of output buffer.", + (void *)handler); aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); } } From aec3d473744db521ce1339104b3f9b0769176e6e Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Wed, 9 Aug 2023 09:13:04 -0700 Subject: [PATCH 12/15] fix the else branch --- source/windows/secure_channel_tls_handler.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/source/windows/secure_channel_tls_handler.c b/source/windows/secure_channel_tls_handler.c index 33346bf21..9ede8ded5 100644 --- a/source/windows/secure_channel_tls_handler.c +++ b/source/windows/secure_channel_tls_handler.c @@ -1079,6 +1079,7 @@ static int s_do_application_data_decrypt(struct aws_channel_handler *handler) { SECURITY_STATUS status = DecryptMessage(&sc_handler->sec_handle, &buffer_desc, 0, NULL); if (status == SEC_E_OK) { + error = AWS_OP_SUCCESS; /* if SECBUFFER_DATA is the buffer type of the second buffer, we have decrypted data to process. If SECBUFFER_DATA is the type for the fourth buffer we need to keep track of it so we can shift everything before doing another decrypt operation. @@ -1111,12 +1112,6 @@ static int s_do_application_data_decrypt(struct aws_channel_handler *handler) { "id=%p: Decrypt ended exactly on the end of the record, resetting buffer.", (void *)handler); } - } else { - AWS_LOGF_ERROR( - AWS_LS_IO_TLS, - "id=%p: Error decrypting message. Unexpected type of output buffer.", - (void *)handler); - aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); } } /* SEC_E_INCOMPLETE_MESSAGE means the message we tried to decrypt isn't a full record and we need to From 6a3a10a9cc695bed65287acf977115f32b8e055d Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Wed, 9 Aug 2023 10:23:54 -0700 Subject: [PATCH 13/15] get rid of NOT BLOCKED check --- source/s2n/s2n_tls_channel_handler.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index 9b2134390..dda7e5c66 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -588,11 +588,6 @@ static int s_s2n_handler_process_read_message( } else { aws_mem_release(outgoing_read_message->allocator, outgoing_read_message); } - - if (blocked == S2N_NOT_BLOCKED) { - /* read is complete */ - break; - } } AWS_LOGF_TRACE( From 0c23414dcd5174bc1781b1d5c668f86994d7b9d0 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Wed, 9 Aug 2023 13:20:42 -0700 Subject: [PATCH 14/15] Update source/s2n/s2n_tls_channel_handler.c Co-authored-by: Michael Graeb --- source/s2n/s2n_tls_channel_handler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index dda7e5c66..b96ee2cec 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -575,7 +575,7 @@ static int s_s2n_handler_process_read_message( return aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); }; - // if read > 0 + /* if read > 0 */ processed += read; outgoing_read_message->message_data.len = (size_t)read; From 53c061cd15df92753437ec57977e355f68beb0f3 Mon Sep 17 00:00:00 2001 From: Waqar Ahmed Khan Date: Wed, 9 Aug 2023 15:30:09 -0700 Subject: [PATCH 15/15] Update s2n_tls_channel_handler.c --- source/s2n/s2n_tls_channel_handler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/s2n/s2n_tls_channel_handler.c b/source/s2n/s2n_tls_channel_handler.c index b96ee2cec..01052e8c9 100644 --- a/source/s2n/s2n_tls_channel_handler.c +++ b/source/s2n/s2n_tls_channel_handler.c @@ -572,7 +572,7 @@ static int s_s2n_handler_process_read_message( s2n_strerror(s2n_errno, "EN"), s2n_strerror_debug(s2n_errno, "EN")); aws_channel_shutdown(slot->channel, AWS_IO_TLS_ERROR_READ_FAILURE); - return aws_raise_error(AWS_IO_TLS_ERROR_READ_FAILURE); + return AWS_OP_SUCCESS; }; /* if read > 0 */