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 47c21dbd4d8c5638255b04f4f070129cd56c911d..051dbf814f4fc64b645936d34c84a8aaa9475a2a 100644 (file)
--- a/lib/isc/netmgr/tlsdns.c
+++ b/lib/isc/netmgr/tlsdns.c
@@ -1394,6 +1394,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 a90485a0665901d3535aab15e8e4ec4709de319e..bd562f3313be2ab38c185637efdc660e06b62bc0 100644 (file)
--- a/lib/isc/netmgr/tlsstream.c
+++ b/lib/isc/netmgr/tlsstream.c
@@ -399,6 +399,27 @@ tls_do_bio(isc_nmsocket_t *sock, isc_region_t *received_data,
                isc_nm_read_stop(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) {
@@ -534,7 +555,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;
        }