From 17109391c6e67dc5d8124a8ebb56629ad3caf2d6 Mon Sep 17 00:00:00 2001 From: John Thiltges Date: Tue, 1 Jun 2021 15:15:11 -0500 Subject: [PATCH 1/4] [XrdCrypto] Clear OpenSSL error queue in XrdCryptosslX509ParseBucket() --- src/XrdCrypto/XrdCryptosslAux.cc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/XrdCrypto/XrdCryptosslAux.cc b/src/XrdCrypto/XrdCryptosslAux.cc index 48fddd8e0ba..5d60b29e59d 100644 --- a/src/XrdCrypto/XrdCryptosslAux.cc +++ b/src/XrdCrypto/XrdCryptosslAux.cc @@ -43,6 +43,7 @@ #include "XrdCrypto/XrdCryptosslX509.hh" #include "XrdCrypto/XrdCryptosslTrace.hh" #include "XrdTls/XrdTlsPeerCerts.hh" +#include #include // Error code from verification set by verify callback function @@ -605,6 +606,7 @@ int XrdCryptosslX509ParseBucket(XrdSutBucket *b, XrdCryptoX509Chain *chain) if (BIO_write(bmem,(const void *)(b->buffer),b->size) != b->size) { DEBUG("problems writing data to BIO"); BIO_free(bmem); + ERR_clear_error(); return nci; } @@ -621,12 +623,18 @@ int XrdCryptosslX509ParseBucket(XrdSutBucket *b, XrdCryptoX509Chain *chain) } else { DEBUG("could not create certificate: memory exhausted?"); BIO_free(bmem); + ERR_clear_error(); return nci; } // reset cert otherwise the next one is not fetched xcer = 0; } + // Clear OpenSSL error queue from PEM_read_bio_X509() loop end condition + if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { + ERR_clear_error(); + } + // If we found something, and we are asked to extract a key, // refill the BIO and search again for the key (this is mandatory // as read operations modify the BIO contents; a read-only BIO @@ -690,6 +698,13 @@ int XrdCryptosslX509ParseBucket(XrdSutBucket *b, XrdCryptoX509Chain *chain) // Cleanup BIO_free(bmem); + // Report and clear any other OpenSSL errors on the queue + char eBuff[120]; int eCode; + while ((eCode = ERR_get_error())) { + ERR_error_string_n(eCode, eBuff, sizeof(eBuff)); + DEBUG("TLS error: " << eBuff); + } + // We are done return nci; } From 13f0349679090ab81db3f81f58f0444786f986e9 Mon Sep 17 00:00:00 2001 From: John Thiltges Date: Tue, 8 Jun 2021 17:29:55 -0500 Subject: [PATCH 2/4] Revert "[XrdTls] Call ERR_clear_error() ahead of SSL_read(), SSL_write(), and SSL_connect() for client connections" This reverts commit 9d355f6e2a53f83617459c9cf11a344571397dbc. --- src/XrdTls/XrdTlsSocket.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/XrdTls/XrdTlsSocket.cc b/src/XrdTls/XrdTlsSocket.cc index 38a3a8acfa1..16497d76485 100644 --- a/src/XrdTls/XrdTlsSocket.cc +++ b/src/XrdTls/XrdTlsSocket.cc @@ -265,8 +265,7 @@ XrdTls::RC XrdTlsSocket::Connect(const char *thehost, std::string *eWhy) // Do the connect. // -do{if (pImpl->isClient) ERR_clear_error(); - int rc = SSL_connect( pImpl->ssl ); +do{int rc = SSL_connect( pImpl->ssl ); if (rc == 1) break; ssler = Diagnose("TLS_Connect", rc, XrdTls::dbgSOK); @@ -641,8 +640,7 @@ XrdTls::RC XrdTlsSocket::Read( char *buffer, size_t size, int &bytesRead ) // have to explicitly call SSL_connect or SSL_do_handshake. //------------------------------------------------------------------------ - do{if (pImpl->isClient) ERR_clear_error(); - int rc = SSL_read( pImpl->ssl, buffer, size ); + do{int rc = SSL_read( pImpl->ssl, buffer, size ); // Note that according to SSL whenever rc > 0 then SSL_ERROR_NONE can be // returned to the caller. So, we short-circuit all the error handling. @@ -787,8 +785,7 @@ XrdTls::RC XrdTlsSocket::Write( const char *buffer, size_t size, // have to explicitly call SSL_connect or SSL_do_handshake. //------------------------------------------------------------------------ - do{if (pImpl->isClient) ERR_clear_error(); - int rc = SSL_write( pImpl->ssl, buffer, size ); + do{int rc = SSL_write( pImpl->ssl, buffer, size ); // Note that according to SSL whenever rc > 0 then SSL_ERROR_NONE can be // returned to the caller. So, we short-circuit all the error handling. From c13a357a6f2da27e650538ac2f420091c183d02c Mon Sep 17 00:00:00 2001 From: John Thiltges Date: Thu, 10 Jun 2021 11:55:49 -0500 Subject: [PATCH 3/4] Revert "[XrdCrypto] Clear OpenSSL error queue in XrdCryptosslX509ParseBucket()" This reverts commit 17109391c6e67dc5d8124a8ebb56629ad3caf2d6. --- src/XrdCrypto/XrdCryptosslAux.cc | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/XrdCrypto/XrdCryptosslAux.cc b/src/XrdCrypto/XrdCryptosslAux.cc index 5d60b29e59d..48fddd8e0ba 100644 --- a/src/XrdCrypto/XrdCryptosslAux.cc +++ b/src/XrdCrypto/XrdCryptosslAux.cc @@ -43,7 +43,6 @@ #include "XrdCrypto/XrdCryptosslX509.hh" #include "XrdCrypto/XrdCryptosslTrace.hh" #include "XrdTls/XrdTlsPeerCerts.hh" -#include #include // Error code from verification set by verify callback function @@ -606,7 +605,6 @@ int XrdCryptosslX509ParseBucket(XrdSutBucket *b, XrdCryptoX509Chain *chain) if (BIO_write(bmem,(const void *)(b->buffer),b->size) != b->size) { DEBUG("problems writing data to BIO"); BIO_free(bmem); - ERR_clear_error(); return nci; } @@ -623,18 +621,12 @@ int XrdCryptosslX509ParseBucket(XrdSutBucket *b, XrdCryptoX509Chain *chain) } else { DEBUG("could not create certificate: memory exhausted?"); BIO_free(bmem); - ERR_clear_error(); return nci; } // reset cert otherwise the next one is not fetched xcer = 0; } - // Clear OpenSSL error queue from PEM_read_bio_X509() loop end condition - if (ERR_GET_REASON(ERR_peek_last_error()) == PEM_R_NO_START_LINE) { - ERR_clear_error(); - } - // If we found something, and we are asked to extract a key, // refill the BIO and search again for the key (this is mandatory // as read operations modify the BIO contents; a read-only BIO @@ -698,13 +690,6 @@ int XrdCryptosslX509ParseBucket(XrdSutBucket *b, XrdCryptoX509Chain *chain) // Cleanup BIO_free(bmem); - // Report and clear any other OpenSSL errors on the queue - char eBuff[120]; int eCode; - while ((eCode = ERR_get_error())) { - ERR_error_string_n(eCode, eBuff, sizeof(eBuff)); - DEBUG("TLS error: " << eBuff); - } - // We are done return nci; } From a327a65198ee547cc62e01cbd2a28dc19dd9b5e9 Mon Sep 17 00:00:00 2001 From: John Thiltges Date: Thu, 10 Jun 2021 13:04:34 -0500 Subject: [PATCH 4/4] [XrdCl] Clear OpenSSL errors from XrdSecProtocolgsi::getCredentials() XrdSecProtocolgsi::getCredentials() will leave OpenSSL errors on the queue, even with non-encrypted channels. Clean up the queue to avoid affecting other TLS traffic. --- src/XrdCl/XrdClXRootDTransport.cc | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/XrdCl/XrdClXRootDTransport.cc b/src/XrdCl/XrdClXRootDTransport.cc index a7635529e17..fde6a4f7950 100644 --- a/src/XrdCl/XrdClXRootDTransport.cc +++ b/src/XrdCl/XrdClXRootDTransport.cc @@ -2241,13 +2241,11 @@ namespace XrdCl "[%s] Authenticated with %s.", hsData->streamName.c_str(), protocolName.c_str() ); - if( info->encrypted || ( info->serverFlags & kXR_gotoTLS ) || - ( info->serverFlags & kXR_tlsLogin ) ) - //-------------------------------------------------------------------- - // Clear the SSL error queue of the calling thread, as there might be - // some leftover from the authentication! - //-------------------------------------------------------------------- - Tls::ClearErrorQueue(); + //-------------------------------------------------------------------- + // Clear the SSL error queue of the calling thread, as there might be + // some leftover from the authentication! + //-------------------------------------------------------------------- + Tls::ClearErrorQueue(); return XRootDStatus(); } @@ -2311,13 +2309,11 @@ namespace XrdCl MarshallRequest( msg ); delete credentials; - if( info->encrypted || ( info->serverFlags & kXR_gotoTLS ) || - ( info->serverFlags & kXR_tlsLogin ) ) - //------------------------------------------------------------------------ - // Clear the SSL error queue of the calling thread, as there might be - // some leftover from the authentication! - //------------------------------------------------------------------------ - Tls::ClearErrorQueue(); + //------------------------------------------------------------------------ + // Clear the SSL error queue of the calling thread, as there might be + // some leftover from the authentication! + //------------------------------------------------------------------------ + Tls::ClearErrorQueue(); return XRootDStatus( stOK, suContinue ); } @@ -2425,6 +2421,7 @@ namespace XrdCl info->authProtocol = 0; info->authParams = 0; info->authEnv = 0; + Tls::ClearErrorQueue(); return Status(); }