TLS: clear error queue before doing IO or calling SSL_get_error()

Ensure that TLS error is empty before calling SSL_get_error() or doing
SSL I/O so that the result will not get affected by prior error
statuses.

In particular, the improper error handling led to intermittent unit
test failure and, thus, could be responsible for some of the system
test failures and other intermittent TLS-related issues.

See here for more details:

https://www.openssl.org/docs/man3.0/man3/SSL_get_error.html

In particular, it mentions the following:

> The current thread's error queue must be empty before the TLS/SSL
> I/O operation is attempted, or SSL_get_error() will not work
> reliably.

As we use the result of SSL_get_error() to decide on I/O operations,
we need to ensure that it works reliably by cleaning the error queue.

TLS DNS: empty error queue before attempting I/O

diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c
index aad92bb48767d0f5a33b17a871aac1ea80174fe0..a0d55203abfa7aa7d170e7316ca12daaf6221c9a 100644 (file)
--- a/lib/isc/netmgr/tlsdns.c
+++ b/lib/isc/netmgr/tlsdns.c
@@ -1385,6 +1385,27 @@ static isc_result_t
 tls_cycle(isc_nmsocket_t *sock) {
        isc_result_t result;
 
+       /*
+        * Clear the TLS error queue so that SSL_get_error() and SSL I/O
+        * routine calls will not get affected by prior error statuses.
+        *
+        * See here:
+        * https://www.openssl.org/docs/man3.0/man3/SSL_get_error.html
+        *
+        * In particular, it mentions the following:
+        *
+        * The current thread's error queue must be empty before the
+        * TLS/SSL I/O operation is attempted, or SSL_get_error() will not
+        * work reliably.
+        *
+        * As we use the result of SSL_get_error() to decide on I/O
+        * operations, we need to ensure that it works reliably by
+        * cleaning the error queue.
+        *
+        * The sum of details: https://stackoverflow.com/a/37980911
+        */
+       ERR_clear_error();
+
        if (isc__nmsocket_closing(sock)) {
                return (ISC_R_CANCELED);
        }

diff --git a/lib/isc/netmgr/tlsstream.c b/lib/isc/netmgr/tlsstream.c
index acfab53cbbf20bc2399b0019767e79a1d2e9e88d..9cb889f8fee93720c56598716aa92f41408232dc 100644 (file)
--- a/lib/isc/netmgr/tlsstream.c
+++ b/lib/isc/netmgr/tlsstream.c
@@ -394,6 +394,27 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
                isc_nm_pauseread(sock->outerhandle);
        }
 
+       /*
+        * Clear the TLS error queue so that SSL_get_error() and SSL I/O
+        * routine calls will not get affected by prior error statuses.
+        *
+        * See here:
+        * https://www.openssl.org/docs/man3.0/man3/SSL_get_error.html
+        *
+        * In particular, it mentions the following:
+        *
+        * The current thread's error queue must be empty before the
+        * TLS/SSL I/O operation is attempted, or SSL_get_error() will not
+        * work reliably.
+        *
+        * As we use the result of SSL_get_error() to decide on I/O
+        * operations, we need to ensure that it works reliably by
+        * cleaning the error queue.
+        *
+        * The sum of details: https://stackoverflow.com/a/37980911
+        */
+       ERR_clear_error();
+
        if (sock->tlsstream.state == TLS_INIT) {
                INSIST(received_data == NULL && send_data == NULL);
                if (sock->tlsstream.server) {
@@ -530,7 +551,7 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
        }
 
        pending = tls_process_outgoing(sock, finish, send_data);
-       if (pending > 0) {
+       if (pending > 0 && tls_status != SSL_ERROR_SSL) {
                /* We'll continue in tls_senddone */
                return;
        }