From e6062210c7cdc6a3e0bde2214bad72103d3dec5f Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 6 Apr 2021 20:57:45 +0300 Subject: [PATCH 1/2] Handle buggy situations with SSL_ERROR_SYSCALL See "BUGS" section at: https://www.openssl.org/docs/man1.1.1/man3/SSL_get_error.html It is mentioned there that when TLS status equals SSL_ERROR_SYSCALL AND errno == 0 it means that underlying transport layer returned EOF prematurely. However, we are managing the transport ourselves, so we should just resume reading from the TCP socket. It seems that this case has been handled properly on modern versions of OpenSSL. That being said, the situation goes in line with the manual: it is briefly mentioned there that SSL_ERROR_SYSCALL might be returned not only in a case of low-level errors (like system call failures). --- lib/isc/netmgr/tlsstream.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index 012e9fb1f6..b535e0564a 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -9,6 +9,7 @@ * information regarding copyright ownership. */ +#include #include #include #include @@ -329,6 +330,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, int rv = 0; bool sent_shutdown = false, received_shutdown = false; size_t len = 0; + int saved_errno = 0; REQUIRE(VALID_NMSOCK(sock)); REQUIRE(sock->tid == isc_nm_tid()); @@ -359,6 +361,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, received_data->length, &len); if (rv <= 0 || len != received_data->length) { result = ISC_R_TLSERROR; + saved_errno = errno; goto error; } @@ -410,6 +413,28 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, } } tls_status = SSL_get_error(sock->tlsstream.tls, rv); + saved_errno = errno; + + /* See "BUGS" section at: + * https://www.openssl.org/docs/man1.1.1/man3/SSL_get_error.html + * + * It is mentioned there that when TLS status equals + * SSL_ERROR_SYSCALL AND errno == 0 it means that underlying + * transport layer returned EOF prematurely. However, we are + * managing the transport ourselves, so we should just resume + * reading from the TCP socket. + * + * It seems that this case has been handled properly on modern + * versions of OpenSSL. That being said, the situation goes in + * line with the manual: it is briefly mentioned there that + * SSL_ERROR_SYSCALL might be returned not only in a case of + * low-level errors (like system call failures). + */ + if (tls_status == SSL_ERROR_SYSCALL && saved_errno == 0 && + received_data == NULL && send_data == NULL && finish == false) + { + tls_status = SSL_ERROR_WANT_READ; + } pending = tls_process_outgoing(sock, finish, send_data); if (pending > 0) { @@ -451,8 +476,12 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, error: isc_log_write(isc_lctx, ISC_LOGCATEGORY_GENERAL, ISC_LOGMODULE_NETMGR, - ISC_LOG_ERROR, "SSL error in BIO: %d %s", tls_status, - isc_result_totext(result)); + ISC_LOG_NOTICE, + "SSL error in BIO: %d %s (errno: %d). Arguments: " + "received_data: %p, " + "send_data: %p, finish: %s", + tls_status, isc_result_totext(result), saved_errno, + received_data, send_data, finish ? "true" : "false"); tls_failed_read_cb(sock, result); } From ee10948e2d7069a1472177502fc3ba3535369e61 Mon Sep 17 00:00:00 2001 From: Artem Boldariev Date: Tue, 6 Apr 2021 23:44:37 +0300 Subject: [PATCH 2/2] Remove dead code which was supposed to handle TLS shutdowns nicely Fixes Coverity issue CID 330954 (See #2612). --- lib/isc/netmgr/tlsstream.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c index b535e0564a..bf9c5f9ce5 100644 --- a/lib/isc/netmgr/tlsstream.c +++ b/lib/isc/netmgr/tlsstream.c @@ -328,7 +328,6 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, isc_result_t result = ISC_R_SUCCESS; int pending, tls_status = SSL_ERROR_NONE; int rv = 0; - bool sent_shutdown = false, received_shutdown = false; size_t len = 0; int saved_errno = 0; @@ -375,7 +374,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, } else if (send_data != NULL) { INSIST(received_data == NULL); INSIST(sock->tlsstream.state > TLS_HANDSHAKE); - received_shutdown = + bool received_shutdown = ((SSL_get_shutdown(sock->tlsstream.tls) & SSL_RECEIVED_SHUTDOWN) != 0); rv = SSL_write_ex(sock->tlsstream.tls, @@ -445,11 +444,6 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data, switch (tls_status) { case SSL_ERROR_NONE: case SSL_ERROR_ZERO_RETURN: - if (sent_shutdown && received_shutdown) { - /* clean shutdown */ - isc_nm_cancelread(sock->outerhandle); - isc__nm_tls_close(sock); - }; return; case SSL_ERROR_WANT_WRITE: if (sock->tlsstream.nsending == 0) {