From 2041b85f61ad58faf5c5ece8fb2247727677e0ef Mon Sep 17 00:00:00 2001 From: Andreas Weigel Date: Mon, 18 Nov 2024 18:38:17 -0500 Subject: [PATCH 01/11] fix loading of dhparams for DHE with Openssl >= 3 The #elif to #if OPENSSL_MAJOR_VERSION < 3 new code block introduced with 742236c7 does not work. Instead of trying to load parameters for DHE, it tries to parse the input file as EC Key if any curve is given to the tls-dh parameter, e.g. tls-dh=prime256v1:/dhparams4096.pem This causes TLS handshakes to fail if the client uses a configuration that does not accept ECDHE. This commit corrects this by setting the type to "DH" and to further also correctly load dhparams into the SSL_CTX from an EVP_PKEY (in contrast to the DH used with openssl < 3), the correct function is used. --- src/security/ServerOptions.cc | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index a9a58555d2e..1f7efe49c4c 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -390,12 +390,12 @@ Security::ServerOptions::loadDhParams() parsedDhParams.resetWithoutLocking(dhp); #else // OpenSSL 3.0+ - const auto type = eecdhCurve.isEmpty() ? "DH" : "EC"; + const auto type = "DH"; Ssl::ForgetErrors(); EVP_PKEY *rawPkey = nullptr; using DecoderContext = std::unique_ptr >; - if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, 0, nullptr, nullptr)}) { + if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, nullptr, nullptr)}) { // OpenSSL documentation is vague on this, but OpenSSL code and our // tests suggest that rawPkey remains nil here while rawCtx keeps @@ -414,7 +414,6 @@ Security::ServerOptions::loadDhParams() assert(rawPkey); const Security::DhePointer pkey(rawPkey); // TODO: verify that the loaded parameters match the curve named in eecdhCurve - if (const Ssl::EVP_PKEY_CTX_Pointer pkeyCtx{EVP_PKEY_CTX_new_from_pkey(nullptr, pkey.get(), nullptr)}) { switch (EVP_PKEY_param_check(pkeyCtx.get())) { case 1: // success @@ -566,9 +565,19 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) // set DH parameters into the server context #if USE_OPENSSL if (parsedDhParams) { - SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get()); - } +#if OPENSSL_VERSION_MAJOR < 3 + if (1 != SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get())) { + debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); + } +#else + EVP_PKEY *tmp = EVP_PKEY_dup(parsedDhParams.get()); + if (1 != SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp)) { + EVP_PKEY_free(tmp); + debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); + } #endif + } +#endif // USE_OPENSSL } void From ba6e0f16e0450ffbc1fc27cc8d5301da5bed02e0 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:12:05 -0500 Subject: [PATCH 02/11] fixup: Undo out-of-scope formatting change --- src/security/ServerOptions.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 1f7efe49c4c..22e4182c04f 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -414,6 +414,7 @@ Security::ServerOptions::loadDhParams() assert(rawPkey); const Security::DhePointer pkey(rawPkey); // TODO: verify that the loaded parameters match the curve named in eecdhCurve + if (const Ssl::EVP_PKEY_CTX_Pointer pkeyCtx{EVP_PKEY_CTX_new_from_pkey(nullptr, pkey.get(), nullptr)}) { switch (EVP_PKEY_param_check(pkeyCtx.get())) { case 1: // success From dc04ed6cefdbaaf3702a407de17ec3bd6d85ba8d Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:15:44 -0500 Subject: [PATCH 03/11] fixup: Avoid Yoda conditions in new code --- src/security/ServerOptions.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 22e4182c04f..3f5fb00579c 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -567,12 +567,12 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) #if USE_OPENSSL if (parsedDhParams) { #if OPENSSL_VERSION_MAJOR < 3 - if (1 != SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get())) { + if (SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get()) != 1) { debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); } #else EVP_PKEY *tmp = EVP_PKEY_dup(parsedDhParams.get()); - if (1 != SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp)) { + if (SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp) != 1) { EVP_PKEY_free(tmp); debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); } From 69be7fe6f1db9fe7956ae88b17878a45a37340f5 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:19:50 -0500 Subject: [PATCH 04/11] fixup: AAA and const-correctness --- src/security/ServerOptions.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 3f5fb00579c..edae6834b17 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -571,7 +571,7 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); } #else - EVP_PKEY *tmp = EVP_PKEY_dup(parsedDhParams.get()); + const auto tmp = EVP_PKEY_dup(parsedDhParams.get()); if (SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp) != 1) { EVP_PKEY_free(tmp); debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); From f0106c64fda39f116bc9ee29bb18d5a12343be4e Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:25:57 -0500 Subject: [PATCH 05/11] fixup: More consistent error phrasing and capitalization "Failed" may be better, but all other ERRORs in this context are using (passable/reasonable) "Unable". Preserve that consistency. --- src/security/ServerOptions.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index edae6834b17..2d94958f4cb 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -568,13 +568,13 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) if (parsedDhParams) { #if OPENSSL_VERSION_MAJOR < 3 if (SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get()) != 1) { - debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); + debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors); } #else const auto tmp = EVP_PKEY_dup(parsedDhParams.get()); if (SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp) != 1) { EVP_PKEY_free(tmp); - debugs(83, DBG_IMPORTANT, "ERROR: failed to set DH parameters on SSL CTX " << Ssl::ReportAndForgetErrors); + debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors); } #endif } From 6a2de085f6cfb70a5ad625d2b293e26cae955156 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:28:40 -0500 Subject: [PATCH 06/11] fixup: Avoid identical messages in two error handling paths This may help reduce triage iterations. --- src/security/ServerOptions.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 2d94958f4cb..34822416dfb 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -568,7 +568,7 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) if (parsedDhParams) { #if OPENSSL_VERSION_MAJOR < 3 if (SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get()) != 1) { - debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors); + debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context (using legacy OpenSSL): " << Ssl::ReportAndForgetErrors); } #else const auto tmp = EVP_PKEY_dup(parsedDhParams.get()); From aa71fdf33d2235c92c140c9077209110c606f7bc Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:32:16 -0500 Subject: [PATCH 07/11] fixup: Do not assume EVP_PKEY_dup() never fails --- src/security/ServerOptions.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 34822416dfb..3b5bb5c93da 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -572,6 +572,10 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) } #else const auto tmp = EVP_PKEY_dup(parsedDhParams.get()); + if (!tmp) { + debugs(83, DBG_IMPORTANT, "ERROR: Unable to duplicate DH parameters: " << Ssl::ReportAndForgetErrors); + return; + } if (SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp) != 1) { EVP_PKEY_free(tmp); debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors); From c382c013b0ffeeac123169214589edde04cd948a Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:33:24 -0500 Subject: [PATCH 08/11] fixup: Report current OpenSSL errors _before_ making another OpenSSL call --- src/security/ServerOptions.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 3b5bb5c93da..247b9012438 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -577,8 +577,8 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) return; } if (SSL_CTX_set0_tmp_dh_pkey(ctx.get(), tmp) != 1) { - EVP_PKEY_free(tmp); debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors); + EVP_PKEY_free(tmp); } #endif } From 404d15ef24fc5fda983c65e2d0a3fd71c3638a96 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:36:45 -0500 Subject: [PATCH 09/11] fixup: Decorate longish/nested #if statement end --- src/security/ServerOptions.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 247b9012438..bff32935f97 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -580,7 +580,7 @@ Security::ServerOptions::updateContextEecdh(Security::ContextPointer &ctx) debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors); EVP_PKEY_free(tmp); } -#endif +#endif // OPENSSL_VERSION_MAJOR } #endif // USE_OPENSSL } From 1972cf5429e50f2a64d892f30354ef4cb5a8770d Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 19 Nov 2024 11:56:23 -0500 Subject: [PATCH 10/11] Documented loadDhParams() and removed incorrect/misleading TODO IIRC, when working on this code earlier (before this branch), I, for one, did not realize that ECDHE curve name is orthogonal to DHE parameters. I incorrectly thought that the curve name is a part of DHE parameters. --- src/security/ServerOptions.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index bff32935f97..07fe2961ffc 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -351,6 +351,12 @@ Security::ServerOptions::loadClientCaFile() return bool(clientCaStack); } +/// Interprets DHE parameters stored in a previously configured dhParamsFile. +/// These DHE parameters are orthogonal to ECDHE curve name that may also be +/// configured when naming that DHE parameters configuration file. When both are +/// configured, the server selects either FFDHE or ECDHE key exchange mechanism +/// (and its cipher suites) depending on client-supported cipher suites. +/// \sa Security::ServerOptions::updateContextEecdh() and RFC 7919 Section 1.2 void Security::ServerOptions::loadDhParams() { @@ -413,8 +419,6 @@ Security::ServerOptions::loadDhParams() if (OSSL_DECODER_from_fp(dctx.get(), in)) { assert(rawPkey); const Security::DhePointer pkey(rawPkey); - // TODO: verify that the loaded parameters match the curve named in eecdhCurve - if (const Ssl::EVP_PKEY_CTX_Pointer pkeyCtx{EVP_PKEY_CTX_new_from_pkey(nullptr, pkey.get(), nullptr)}) { switch (EVP_PKEY_param_check(pkeyCtx.get())) { case 1: // success From 4657834d37cc6cc37e295f39b75dfef74cfa6ac7 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Tue, 26 Nov 2024 11:47:58 -0500 Subject: [PATCH 11/11] fixup: Undo out-of-scope OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS change I hope that changing this parameter is not _required_ to fix the problem this branch is focusing on. If a change is needed, the parameter may need to become OSSL_KEYMGMT_SELECT_OTHER_PARAMETERS. --- src/security/ServerOptions.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc index 07fe2961ffc..4bdd8724442 100644 --- a/src/security/ServerOptions.cc +++ b/src/security/ServerOptions.cc @@ -401,7 +401,7 @@ Security::ServerOptions::loadDhParams() Ssl::ForgetErrors(); EVP_PKEY *rawPkey = nullptr; using DecoderContext = std::unique_ptr >; - if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, nullptr, nullptr)}) { + if (const DecoderContext dctx{OSSL_DECODER_CTX_new_for_pkey(&rawPkey, "PEM", nullptr, type, 0, nullptr, nullptr)}) { // OpenSSL documentation is vague on this, but OpenSSL code and our // tests suggest that rawPkey remains nil here while rawCtx keeps