From 1d6338ea9677bb08812bfa4dfa5e68e84ed0feaf Mon Sep 17 00:00:00 2001 From: Remi Tricot-Le Breton Date: Tue, 12 Apr 2022 11:31:55 +0200 Subject: [PATCH] MEDIUM: ssl: Disable DHE ciphers by default DHE ciphers do not present a security risk if the key is big enough but they are slow and mostly obsoleted by ECDHE. This patch removes any default DH parameters. This will effectively disable all DHE ciphers unless a global ssl-dh-param-file is defined, or tune.ssl.default-dh-param is set, or a frontend has DH parameters included in its PEM certificate. In this latter case, only the frontends that have DH parameters will have DHE ciphers enabled. Adding explicitely a DHE ciphers in a "bind" line will not be enough to actually enable DHE. We would still need to know which DH parameters to use so one of the three conditions described above must be met. This request was described in GitHub issue #1604. --- doc/configuration.txt | 20 +++++---- include/haproxy/defaults.h | 2 +- reg-tests/ssl/ssl_dh.vtc | 29 +++++-------- src/ssl_sock.c | 88 ++++++++++++++------------------------ 4 files changed, 55 insertions(+), 84 deletions(-) diff --git a/doc/configuration.txt b/doc/configuration.txt index 95d117711..a3f3e4618 100644 --- a/doc/configuration.txt +++ b/doc/configuration.txt @@ -1960,9 +1960,10 @@ ssl-dh-param-file which do not explicitly define theirs. It will be overridden by custom DH parameters found in a bind certificate file if any. If custom DH parameters are not specified either by using ssl-dh-param-file or by setting them - directly in the certificate file, pre-generated DH parameters of the size - specified by tune.ssl.default-dh-param will be used. Custom parameters are - known to be more secure and therefore their use is recommended. + directly in the certificate file, DHE ciphers will not be used, unless + tune.ssl.default-dh-param is set. In this latter case, pre-defined DH + parameters of the specified size will be used. Custom parameters are known to + be more secure and therefore their use is recommended. Custom DH parameters may be generated by using the OpenSSL command "openssl dhparam ", where size should be at least 2048, as 1024-bit DH parameters should not be considered secure anymore. @@ -2925,11 +2926,14 @@ tune.ssl.default-dh-param the ephemeral/temporary Diffie-Hellman key in case of DHE key exchange. The final size will try to match the size of the server's RSA (or DSA) key (e.g, a 2048 bits temporary DH key for a 2048 bits RSA key), but will not exceed - this maximum value. Default value if 2048. Only 1024 or higher values are - allowed. Higher values will increase the CPU load, and values greater than - 1024 bits are not supported by Java 7 and earlier clients. This value is not - used if static Diffie-Hellman parameters are supplied either directly - in the certificate file or by using the ssl-dh-param-file parameter. + this maximum value. Only 1024 or higher values are allowed. Higher values + will increase the CPU load, and values greater than 1024 bits are not + supported by Java 7 and earlier clients. This value is not used if static + Diffie-Hellman parameters are supplied either directly in the certificate + file or by using the ssl-dh-param-file parameter. + If there is neither a default-dh-param nor a ssl-dh-param-file defined, and + if the server's PEM file of a given frontend does not specify its own DH + parameters, then DHE ciphers will be unavailable for this frontend. tune.ssl.ssl-ctx-cache-size Sets the size of the cache used to store generated certificates to diff --git a/include/haproxy/defaults.h b/include/haproxy/defaults.h index 8079d059d..d4721a794 100644 --- a/include/haproxy/defaults.h +++ b/include/haproxy/defaults.h @@ -354,7 +354,7 @@ /* ssl max dh param size */ #ifndef SSL_DEFAULT_DH_PARAM -#define SSL_DEFAULT_DH_PARAM 2048 +#define SSL_DEFAULT_DH_PARAM 0 #endif /* max memory cost per SSL session */ diff --git a/reg-tests/ssl/ssl_dh.vtc b/reg-tests/ssl/ssl_dh.vtc index 801ba607a..9553f371d 100644 --- a/reg-tests/ssl/ssl_dh.vtc +++ b/reg-tests/ssl/ssl_dh.vtc @@ -2,9 +2,10 @@ # This reg-tests checks that the DH-related mechanisms works properly. # When no DH is specified, either directly in the server's PEM or through a -# ssl-dh-param-file global option, in case of DHE negotiation, the hard coded -# DH parameters included in the sources will be used. We will use DH parameters -# of the same size as the server's RSA or DSA key. +# ssl-dh-param-file global option, and no tune.ssl.default-dh-param is defined, +# DHE ciphers are disabled. +# If a default-dh-param is defined, we will use DH parameters of the same size +# as the server's RSA or DSA key, or default-dh-param if it is smaller. # This test has three distinct HAProxy instances, one with no DH-related option # used, one with the tune.ssl.default-dh-param global parameter set, and one # with an ssl-dh-param-file global option. @@ -123,8 +124,8 @@ haproxy h3 -conf { client c1 -connect ${h1_clearlst_sock} { txreq rxresp - expect resp.status == 200 - expect resp.http.x-ssl-cipher == "DHE-RSA-AES256-GCM-SHA384" + # No DH parameters are defined, DHE ciphers are unavailable + expect resp.status == 503 } -run client c2 -connect ${h2_clearlst_dfltdh_sock} { @@ -144,18 +145,11 @@ client c3 -connect ${h3_clearlst_dhfile_sock} { client c4 -connect ${h1_clearlst_sock} { txreq -url "/gencert" rxresp - expect resp.status == 200 - expect resp.http.x-ssl-cipher == "DHE-RSA-AES256-GCM-SHA384" + # No DH parameters are defined, DHE ciphers are unavailable + expect resp.status == 503 } -run -# -# Check the size of the DH key -# -shell { - echo "Q" | openssl s_client -unix "${tmpdir}/ssl_dflt.sock" -tls1_2 2>/dev/null | grep -E "Server Temp Key: DH, 2048 bits" -} - # On the second HAProxy instance, even if default-dh-param is set to 4096, this # value is only considered as a maximum DH key length and we will always try to # match the server's certificate key length in our DHE key exchange (2048 bits @@ -168,11 +162,6 @@ shell { echo "Q" | openssl s_client -unix "${tmpdir}/ssl_dhfile.sock" -tls1_2 2>/dev/null | grep -E "Server Temp Key: DH, 4096 bits" } -shell { - echo "Q" | openssl s_client -unix "${tmpdir}/ssl_dflt_gencert.sock" -tls1_2 2>/dev/null | grep -E "Server Temp Key: DH, 2048 bits" -} - - # # Add a custom DH to the server's PEM certificate @@ -191,6 +180,8 @@ shell { # # Check that all the SSL backend <-> SSL frontend connections still work +# Common.pem now contains DH parameters so the first instance's frontends +# can now use DHE ciphers. # client c5 -connect ${h1_clearlst_sock} { txreq diff --git a/src/ssl_sock.c b/src/ssl_sock.c index ceb4a067c..145ccb3fb 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -3215,8 +3215,6 @@ static void ssl_sock_set_tmp_dh_from_pkey(SSL_CTX *ctx, EVP_PKEY *pkey) if (!SSL_CTX_set0_tmp_dh_pkey(ctx, dh)) HASSL_DH_free(dh); } - else - SSL_CTX_set_dh_auto(ctx, 1); } #endif @@ -3421,14 +3419,8 @@ static int ssl_sock_load_dh_params(SSL_CTX *ctx, const struct cert_key_and_chain if (!ssl_sock_set_tmp_dh(ctx, dh)) { memprintf(err, "%sunable to load the DH parameter specified in '%s'", err && *err ? *err : "", path); -#if defined(SSL_CTX_set_dh_auto) - SSL_CTX_set_dh_auto(ctx, 1); - memprintf(err, "%s, SSL library will use an automatically generated DH parameter.\n", - err && *err ? *err : ""); -#else memprintf(err, "%s, DH ciphers won't be available.\n", err && *err ? *err : ""); -#endif ret |= ERR_WARN; goto end; } @@ -3443,14 +3435,8 @@ static int ssl_sock_load_dh_params(SSL_CTX *ctx, const struct cert_key_and_chain if (!ssl_sock_set_tmp_dh(ctx, global_dh)) { memprintf(err, "%sunable to use the global DH parameter for certificate '%s'", err && *err ? *err : "", path); -#if defined(SSL_CTX_set_dh_auto) - SSL_CTX_set_dh_auto(ctx, 1); - memprintf(err, "%s, SSL library will use an automatically generated DH parameter.\n", - err && *err ? *err : ""); -#else memprintf(err, "%s, DH ciphers won't be available.\n", err && *err ? *err : ""); -#endif ret |= ERR_WARN; goto end; } @@ -3459,39 +3445,38 @@ static int ssl_sock_load_dh_params(SSL_CTX *ctx, const struct cert_key_and_chain /* Clear openssl global errors stack */ ERR_clear_error(); - if (global_ssl.default_dh_param && global_ssl.default_dh_param <= 1024) { - /* we are limited to DH parameter of 1024 bits anyway */ - if (local_dh_1024 == NULL) - local_dh_1024 = ssl_get_dh_1024(); + /* We do not want DHE ciphers to be added to the cipher list + * unless there is an explicit global dh option in the conf. + */ + if (global_ssl.default_dh_param) { + if (global_ssl.default_dh_param <= 1024) { + /* we are limited to DH parameter of 1024 bits anyway */ + if (local_dh_1024 == NULL) + local_dh_1024 = ssl_get_dh_1024(); - if (local_dh_1024 == NULL) { - memprintf(err, "%sunable to load default 1024 bits DH parameter for certificate '%s'.\n", - err && *err ? *err : "", path); - ret |= ERR_ALERT | ERR_FATAL; - goto end; - } + if (local_dh_1024 == NULL) { + memprintf(err, "%sunable to load default 1024 bits DH parameter for certificate '%s'.\n", + err && *err ? *err : "", path); + ret |= ERR_ALERT | ERR_FATAL; + goto end; + } - if (!ssl_sock_set_tmp_dh(ctx, local_dh_1024)) { - memprintf(err, "%sunable to load default 1024 bits DH parameter for certificate '%s'.\n", - err && *err ? *err : "", path); -#if defined(SSL_CTX_set_dh_auto) - SSL_CTX_set_dh_auto(ctx, 1); - memprintf(err, "%s, SSL library will use an automatically generated DH parameter.\n", - err && *err ? *err : ""); -#else - memprintf(err, "%s, DH ciphers won't be available.\n", - err && *err ? *err : ""); -#endif - ret |= ERR_WARN; - goto end; + if (!ssl_sock_set_tmp_dh(ctx, local_dh_1024)) { + memprintf(err, "%sunable to load default 1024 bits DH parameter for certificate '%s'.\n", + err && *err ? *err : "", path); + memprintf(err, "%s, DH ciphers won't be available.\n", + err && *err ? *err : ""); + ret |= ERR_WARN; + goto end; + } } - } - else { + else { #if (HA_OPENSSL_VERSION_NUMBER < 0x3000000fL) - SSL_CTX_set_tmp_dh_callback(ctx, ssl_get_tmp_dh_cbk); + SSL_CTX_set_tmp_dh_callback(ctx, ssl_get_tmp_dh_cbk); #else - ssl_sock_set_tmp_dh_from_pkey(ctx, ckch ? ckch->key : NULL); + ssl_sock_set_tmp_dh_from_pkey(ctx, ckch ? ckch->key : NULL); #endif + } } } @@ -4828,21 +4813,12 @@ static int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, struct ssl_bind_con #endif #ifndef OPENSSL_NO_DH - if (global_ssl.default_dh_param >= 1024) { - if (local_dh_1024 == NULL) { - local_dh_1024 = ssl_get_dh_1024(); - } - if (global_ssl.default_dh_param >= 2048) { - if (local_dh_2048 == NULL) { - local_dh_2048 = ssl_get_dh_2048(); - } - if (global_ssl.default_dh_param >= 4096) { - if (local_dh_4096 == NULL) { - local_dh_4096 = ssl_get_dh_4096(); - } - } - } - } + if (!local_dh_1024) + local_dh_1024 = ssl_get_dh_1024(); + if (!local_dh_2048) + local_dh_2048 = ssl_get_dh_2048(); + if (!local_dh_4096) + local_dh_4096 = ssl_get_dh_4096(); #endif /* OPENSSL_NO_DH */ SSL_CTX_set_info_callback(ctx, ssl_sock_infocbk);