From: Andreas Weigel <andreas.weigel@securepoint.de>
Date: Thu, 13 Mar 2025 11:30:28 +0000 (+0000)
Subject: Fix tls-dh support for DHE parameters with OpenSSL v3+ (#1949)
X-Git-Tag: SQUID_7_0_2~17
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=42ca4bf51157d8a21a5feadb3eeb5dcf6c4fe7fc;p=thirdparty%2Fsquid.git

Fix tls-dh support for DHE parameters with OpenSSL v3+ (#1949)

    # When applying tls-dh=prime256v1:dhparams.pem configuration:
    WARNING: Failed to decode EC parameters 'dhparams.pem'

    # When forcing the use of FFDHE with something like
    # openssl s_client -tls1_2 -cipher DHE-RSA-AES256-SHA256 -connect...
    ERROR: failure while accepting a TLS connection on:
        SQUID_TLS_ERR_ACCEPT+TLS_LIB_ERR=A0000C1+TLS_IO_ERR=1

Squid `https_port ... tls-dh=curve:dhparams.pem` configuration is
supposed to support _both_ ECDHE and FFDHE key exchange mechanisms (and
their cipher suites), depending on client-supported cipher suites. ECDHE
mechanism should use the named curve (e.g., `prime256v1`), and FFDHE
mechanism should use key exchange parameters loaded from the named PEM
file (e.g., `ffdhe4096` named group specified in RFC 7919).

When 2022 commit 742236c added support for OpenSSL v3 APIs, new
loadDhParams() code misinterpreted curve name presence in `tls-dh` value
as an indication that the named parameters file contains ECDHE
parameters, setting OSSL_DECODER_CTX_new_for_pkey() type parameter to
"EC", and (when parameter file specified FFDHE details) triggering the
WARNING message quoted above.

Squid should not expect additional ECDHE parameters when the elliptic
curve group is already fully specified by naming it at the start of
`tls-dh` value. Squid now reverts to earlier (v4) behavior, where
the two mechanisms can coexist and can be configured separately as
described above:

    $ openssl s_client -tls1_2 -cipher DHE-RSA-AES256-SHA256 -connect...
    Server Temp Key: DH, 4096 bits

    $ openssl s_client -connect...
    Server Temp Key: ECDH, prime256v1, 256 bits

Furthermore, updateContextEecdh() code in commit 742236c continued  to
load parsed parameters using old SSL_CTX_set_tmp_dh() call but should
have used SSL_CTX_set0_tmp_dh_pkey() API because the type of parsed
parameters (i.e. DhePointer) have changed from DH to EVP_PKEY pointer.
This second bug affected configurations with and without an explicit
curve name in `tls-dh` value.

Also report a failure to load parsed parameters into TLS context.
---

diff --git a/src/security/ServerOptions.cc b/src/security/ServerOptions.cc
index a9a58555d2..4bdd872444 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()
 {
@@ -390,7 +396,7 @@ 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;
@@ -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
@@ -566,9 +570,23 @@ 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 (SSL_CTX_set_tmp_dh(ctx.get(), parsedDhParams.get()) != 1) {
+            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());
+        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) {
+            debugs(83, DBG_IMPORTANT, "ERROR: Unable to set DH parameters in TLS context: " << Ssl::ReportAndForgetErrors);
+            EVP_PKEY_free(tmp);
+        }
+#endif // OPENSSL_VERSION_MAJOR
     }
-#endif
+#endif // USE_OPENSSL
 }
 
 void