poco icon indicating copy to clipboard operation
poco copied to clipboard

HTTPClientSession::receiveResponse() gives NoMessage instead of Timeout exception for SSL connection on Windows when using OpenSSL 3.0.x

Open jngrb opened this issue 2 years ago • 1 comments

I am using these versions:

  • Poco 1.12.2 with the NetSSL_OpenSSL module ( not NetSSL_Win - I have reasons to do this)
  • OpenSSL 3.0.5

compiled with VS 2019 (VS 16.11.2) on Windows 10

I use a test case where a HTTPS server is sending delayed responses ("mock server").

The test client uses a Poco::Net::HTTPSClientSession to send requests to the mock server. In the relevant test cases, I configure the timeout to be less than the delay of the mock server.

I observe the following changes behavior going from Poco 1.11.0 with OpenSSL 1.1.1k to Poco 1.12.2 with OpenSSL 3.0.5:

  • Poco 1.11.0 with OpenSSL 1.1.1k: Poco::Net::HTTPSClientSession::receiveResponse (== HTTPClientSession::receiveResponse) gives a Poco::TimeoutException
  • Poco 1.12.2 with OpenSSL 3.0.5: Poco::Net::HTTPSClientSession::receiveResponse (== HTTPClientSession::receiveResponse) gives a Poco::NoMessageException

On Linux, the behavior did not change:

  • Poco 1.11.0 with OpenSSL 1.1.1k: HTTPClientSession::receiveResponse gives a Poco::TimeoutException
  • Poco 1.12.2 with OpenSSL 3.0.5: HTTPClientSession::receiveResponse gives a Poco::TimeoutException

The reasons seems to be this code:

int SecureSocketImpl::handleError(int rc)
{
	if (rc > 0) return rc;

	int sslError = SSL_get_error(_pSSL, rc);
	int socketError = SocketImpl::lastError();

	switch (sslError)
	{
[...]
	// SSL_GET_ERROR(3ossl):
	// On an unexpected EOF, versions before OpenSSL 3.0 returned
	// SSL_ERROR_SYSCALL, nothing was added to the error stack, and
	// errno was 0.  Since OpenSSL 3.0 the returned error is
	// SSL_ERROR_SSL with a meaningful error on the error stack.
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
	case SSL_ERROR_SSL:
#else
	case SSL_ERROR_SYSCALL:
#endif
		if (socketError)
		{
			SocketImpl::error(socketError);
		}
		// fallthrough
	default:
		{
			long lastError = ERR_get_error();
[...]
		}
 		break;
	}
	return rc;
}

Based on my observations, I'd say the comment should say

On an unexpected EOF, versions before OpenSSL 3.0 returned SSL_ERROR_SYSCALL, nothing was added to the error stack, and errno was 0. Since OpenSSL 3.0 the returned error is SSL_ERROR_SSL with a meaningful error on the error stack for Linux. While the behavior did not change on Windows.

Thus, I propose the following change:

int SecureSocketImpl::handleError(int rc)
{
	if (rc > 0) return rc;

	int sslError = SSL_get_error(_pSSL, rc);
	int socketError = SocketImpl::lastError();

	switch (sslError)
	{
[...]
	// SSL_GET_ERROR(3ossl):
	// On an unexpected EOF, versions before OpenSSL 3.0 returned
	// SSL_ERROR_SYSCALL, nothing was added to the error stack, and
	// errno was 0.  Since OpenSSL 3.0 the returned error is
	// SSL_ERROR_SSL with a meaningful error on the error stack
        // on Linux. While the behavior did not change on Windows.
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
	case SSL_ERROR_SSL:
		// fallthrough
#endif
	case SSL_ERROR_SYSCALL:
		if (socketError)
		{
			SocketImpl::error(socketError);
		}
		// fallthrough
	default:
[...]
}

jngrb avatar Sep 20 '22 17:09 jngrb

Potentially related issues:

  • #3672 for Windows 11 and Poco 1.11.3 with plain HTTP (likely different from this issue)
  • #3725 for Ubuntu 2022 and Poco 1.11.1 using an unknown OpenSSL version (likely different from this issue)

jngrb avatar Sep 20 '22 17:09 jngrb

This is a bug, not an enhancement. The current code is failing to handle any socket errors with OpenSSL 3.0 because SSL_ERROR_SYSCALL is still a valid error code, and still needs to result in socket errors being handled appropriately. As it stands, this is a breaking change in behaviour with Poco 1.12. Please merge the fix in to the next Poco 1.12 release. I've created an up-to-date pull request with the fix for the devel branch.

Burgch avatar Mar 17 '23 12:03 Burgch

Thanks @aleks-f - this issue can now be closed I believe.

Burgch avatar Mar 20 '23 10:03 Burgch

Thanks for merging the fix.

jngrb avatar Mar 20 '23 20:03 jngrb