Skip to content

Fixed incorrect SSL_CTX_set0_tmp_dh_pkey() usage #4663

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

Closed
wants to merge 2 commits into from

Conversation

pkl97
Copy link
Contributor

@pkl97 pkl97 commented Sep 4, 2024

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.

Copy link
Member

@aleks-f aleks-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, you broke quite a bit of things with this, please see what else needs to be adressed; won't be merged unless CI is all green

@chrisse74
Copy link

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.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

@aleks-f
Copy link
Member

aleks-f commented Oct 3, 2024

@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

@obiltschnig obiltschnig self-assigned this Oct 28, 2024
@obiltschnig obiltschnig added this to the Release 1.14.0 milestone Oct 28, 2024
@aleks-f aleks-f mentioned this pull request Oct 31, 2024
@aleks-f
Copy link
Member

aleks-f commented Oct 31, 2024

moved to #4753

@aleks-f aleks-f closed this Oct 31, 2024
aleks-f added a commit that referenced this pull request Oct 31, 2024
matejk pushed a commit that referenced this pull request Nov 11, 2024
* Fixed incorrect SSL_CTX_set0_tmp_dh_pkey() usage

* fix(OpenSSL): use DH group enum

* fix(IPAddress): windows scoped test, part II #4644

* fix(OpenSSL): fuzz errors #4663

* chore: remove misplaced comment

---------

Co-authored-by: Peter Klotz <peter.klotz99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants