poco icon indicating copy to clipboard operation
poco copied to clipboard

Fixed incorrect SSL_CTX_set0_tmp_dh_pkey() usage

Open pkl97 opened this issue 1 year ago • 2 comments

This simple program crashes POCO (tested under Red Hat Enterprise Linux 9.4):

#include <Poco/Net/Context.h>

int main()
{
    const Poco::Net::Context context(Poco::Net::Context::CLIENT_USE, "/tmp", Poco::Net::Context::VERIFY_STRICT, 9, false, "ALL");
    return 0;
}

The problem is an incorrect usage of SSL_CTX_set0_tmp_dh_pkey() in Context::initDH(). The return value is not evaluated and the key is freed even if it has been successfully transferred to the SSL Context.

The relevant part of the OpenSSL manpage https://docs.openssl.org/3.1/man3/SSL_CTX_set_tmp_dh_callback/:

Ownership of the dhpkey value is passed to the SSL_CTX or SSL object as a result of this call, and so the caller should not free it if the function call is successful.

pkl97 avatar Sep 04 '24 14:09 pkl97

Hi all,

The error messages show that the newly introduced exception which is raised if SSL_CTX_set0_tmp_dh_pkey() returns with an error is triggering the failures in the CI.

1: CppUnit::TestCaller<HTTPSClientSessionTest>.testGetSmall "Poco::Net::SSLContextException: SSL context exception: Context::initDH():SSL_CTX_set0_tmp_dh_pkey()

error:0A00018A:SSL routines::dh key too small"

OpenSSL rejects the given DH key because it is too small. Prior to this PR this rejection was not reported, leaving the client under the impression that the given DH keys were accepted.

My guess is that the machines running the failing tests are configured to use SECLEVEL=2 (see https://stackoverflow.com/questions/61626206/what-could-cause-dh-key-too-small-error ) and thereby do not support 1024-bit DH keys.

These errors go away if you change dhUse2048Bits from false to true in https://github.com/pocoproject/poco/blob/b6b1fec14f3966ca2cf2b862f607e4c00093c964/NetSSL_OpenSSL/src/Context.cpp#L48C2-L48C23

@aleks-f Could changing dhUse2048Bits in Context::Params::Params() be the way forward?

PS: On my machine this change brings the errors down to two errors (probably unrelated?)

There were 2 errors:
 1: CppUnit::TestCaller<HTTPSClientSessionTest>.testProxy
    "Poco::Net::HTTPException:
HTTP Exception: Cannot establish proxy connection: Forbidden"
    in "<unknown>", line -1
 2: CppUnit::TestCaller<HTTPSStreamFactoryTest>.testProxy
    "Poco::Net::HTTPException:
HTTP Exception: Cannot establish proxy connection: Forbidden"
    in "<unknown>", line -1

chrisse74 avatar Sep 23 '24 14:09 chrisse74

@aleks-f Could changing dhUse2048Bits in Context::Params::Params() be the way forward?

Yes, but let's introduce the default values as parameters to the Params::Params(). And the key size should really be the group number, rather than a simple boolean switch between 1024/2048. There should also be an unit tests to check what exactly we're doing.

PS: On my machine this change brings the errors down to two errors (probably unrelated?)

yes, unrelated

aleks-f avatar Oct 03 '24 09:10 aleks-f

moved to #4753

aleks-f avatar Oct 31 '24 01:10 aleks-f