Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jngrb opened this issue Sep 20, 2022 · 4 comments
Labels

Comments

@jngrb
Copy link

jngrb commented Sep 20, 2022

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
Copy link
Author

jngrb commented Sep 20, 2022

Potentially related issues:

jngrb added a commit to jngrb/poco that referenced this issue Sep 21, 2022
consider that the OpenSSL 3.0.x error behaviour
did not change on Windows comapared to OpenSSL 1.1.1

see pocoproject#3806
@aleks-f aleks-f added this to the Release 1.12.3 milestone Oct 6, 2022
@Burgch
Copy link
Contributor

Burgch commented Mar 17, 2023

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.

@aleks-f aleks-f added bug and removed enhancement labels Mar 17, 2023
@Burgch
Copy link
Contributor

Burgch commented Mar 20, 2023

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

@jngrb
Copy link
Author

jngrb commented Mar 20, 2023

Thanks for merging the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants