From ece360393fb866e49b4fb3aa576c37b827cb7bb2 Mon Sep 17 00:00:00 2001
From: Aleksandar Fabijanic <aleks-f@users.noreply.github.com>
Date: Tue, 2 Apr 2024 11:53:42 -0500
Subject: [PATCH] 4435 secure sock thread (#4512)

* fix(SecureSocket): Refactor detection of timeout when reading, writing or handshaking. (#3725)

* enh(SecureSocket): some trivial C++17 modernisation changes.

* chore: indentation and compiler warning

* fix(SecureSocketImpl): not thread-safe (1st attempt) #4435

* fix(SecureSocketImpl): silence CodeQL cpp/certificate-not-checked

---------

Co-authored-by: Matej Kenda <matejken@gmail.com>
---
 .../include/Poco/Net/SecureSocketImpl.h       |  7 ++-
 NetSSL_OpenSSL/src/SecureSocketImpl.cpp       | 43 ++++++++++++++++---
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/NetSSL_OpenSSL/include/Poco/Net/SecureSocketImpl.h b/NetSSL_OpenSSL/include/Poco/Net/SecureSocketImpl.h
index c8eedb638..38b7ea502 100644
--- a/NetSSL_OpenSSL/include/Poco/Net/SecureSocketImpl.h
+++ b/NetSSL_OpenSSL/include/Poco/Net/SecureSocketImpl.h
@@ -284,16 +284,21 @@ protected:
 		/// Callback to handle new session data sent by server.
 
 private:
+	using MutexT = Poco::FastMutex;
+	using LockT = MutexT::ScopedLock;
+	using UnLockT = Poco::ScopedLockWithUnlock<MutexT>;
+
 	SecureSocketImpl(const SecureSocketImpl&);
 	SecureSocketImpl& operator = (const SecureSocketImpl&);
 
-	SSL* _pSSL;
+	std::atomic<SSL*> _pSSL;
 	Poco::AutoPtr<SocketImpl> _pSocket;
 	Context::Ptr _pContext;
 	bool _needHandshake;
 	std::string _peerHostName;
 	Session::Ptr _pSession;
 	bool _bidirectShutdown = true;
+	mutable MutexT _mutex;
 
 	friend class SecureStreamSocketImpl;
 	friend class Context;
diff --git a/NetSSL_OpenSSL/src/SecureSocketImpl.cpp b/NetSSL_OpenSSL/src/SecureSocketImpl.cpp
index d82c834bd..2963d7f9e 100644
--- a/NetSSL_OpenSSL/src/SecureSocketImpl.cpp
+++ b/NetSSL_OpenSSL/src/SecureSocketImpl.cpp
@@ -80,6 +80,8 @@ void SecureSocketImpl::acceptSSL()
 {
 	poco_assert (!_pSSL);
 
+	LockT l(_mutex);
+
 	BIO* pBIO = ::BIO_new(BIO_s_socket());
 	if (!pBIO) throw SSLException("Cannot create BIO object");
 	BIO_set_fd(pBIO, static_cast<int>(_pSocket->sockfd()), BIO_NOCLOSE);
@@ -156,6 +158,8 @@ void SecureSocketImpl::connectSSL(bool performHandshake)
 	poco_assert (!_pSSL);
 	poco_assert (_pSocket->initialized());
 
+	LockT l(_mutex);
+
 	::BIO* pBIO = ::BIO_new(BIO_s_socket());
 	if (!pBIO) throw SSLException("Cannot create SSL BIO object");
 	BIO_set_fd(pBIO, static_cast<int>(_pSocket->sockfd()), BIO_NOCLOSE);
@@ -253,6 +257,8 @@ void SecureSocketImpl::shutdown()
 {
 	if (_pSSL)
 	{
+		UnLockT l(_mutex);
+
 		// Don't shut down the socket more than once.
 		int shutdownState = ::SSL_get_shutdown(_pSSL);
 		bool shutdownSent = (shutdownState & SSL_SENT_SHUTDOWN) == SSL_SENT_SHUTDOWN;
@@ -301,6 +307,9 @@ void SecureSocketImpl::shutdown()
 			int rc = ::SSL_shutdown(_pSSL);
 #endif
 			if (rc < 0) handleError(rc);
+
+			l.unlock();
+
 			if (_pSocket->getBlocking())
 			{
 				_pSocket->shutdown();
@@ -345,6 +354,9 @@ int SecureSocketImpl::sendBytes(const void* buffer, int length, int flags)
 	poco_check_ptr (_pSSL);
 
 	int rc;
+
+	LockT l(_mutex);
+
 	if (_needHandshake)
 	{
 		rc = completeHandshake();
@@ -381,6 +393,9 @@ int SecureSocketImpl::receiveBytes(void* buffer, int length, int flags)
 	poco_check_ptr (_pSSL);
 
 	int rc;
+
+	LockT l(_mutex);
+
 	if (_needHandshake)
 	{
 		rc = completeHandshake();
@@ -414,6 +429,8 @@ int SecureSocketImpl::available() const
 {
 	poco_check_ptr (_pSSL);
 
+	LockT l(_mutex);
+
 	return ::SSL_pending(_pSSL);
 }
 
@@ -468,7 +485,7 @@ long SecureSocketImpl::verifyPeerCertificateImpl(const std::string& hostName)
 {
 	Context::VerificationMode mode = _pContext->verificationMode();
 	if (mode == Context::VERIFY_NONE || !_pContext->extendedCertificateVerificationEnabled() ||
-	    (mode != Context::VERIFY_STRICT && isLocalHost(hostName)))
+	   (mode != Context::VERIFY_STRICT && isLocalHost(hostName)))
 	{
 		return X509_V_OK;
 	}
@@ -499,10 +516,19 @@ bool SecureSocketImpl::isLocalHost(const std::string& hostName)
 
 X509* SecureSocketImpl::peerCertificate() const
 {
+	LockT l(_mutex);
+
+	X509* pCert = nullptr;
+
 	if (_pSSL)
-		return ::SSL_get_peer_certificate(_pSSL);
-	else
-		return nullptr;
+	{
+		pCert = ::SSL_get_peer_certificate(_pSSL);
+		if (X509_V_OK != SSL_get_verify_result(_pSSL))
+			throw CertificateValidationException("SecureSocketImpl::peerCertificate(): "
+				"Certificate verification error " + Utility::getLastError());
+	}
+
+	return pCert;
 }
 
 
@@ -637,6 +663,8 @@ void SecureSocketImpl::reset()
 {
 	if (_pSSL)
 	{
+		LockT l(_mutex);
+
 		::SSL_set_ex_data(_pSSL, SSLManager::instance().socketIndex(), nullptr);
 		::SSL_free(_pSSL);
 		_pSSL = nullptr;
@@ -665,9 +693,12 @@ void SecureSocketImpl::useSession(Session::Ptr pSession)
 bool SecureSocketImpl::sessionWasReused()
 {
 	if (_pSSL)
+	{
+		LockT l(_mutex);
 		return ::SSL_session_reused(_pSSL) != 0;
-	else
-		return false;
+	}
+
+	return false;
 }