Hi, On 05/26/2015 11:09 PM, Willy Tarreau wrote: >> - a new configuration option, something like ssl-dh-param-file, allowing >> the use of a global DH parameters file (which may still be overridden by >> setting the DH parameters directly in a certificate file) ; > > Just a question, does it make sense to have different dh-param files > per key size so that depending on the cert key size we use a different > file, or are they totally decorrelated ?
I used to think that it made sense, but clearly the trend is to decorrelate the two. RSA 1024 is (at last) being phased out, and we have seen on this mailing-list that a DH greater than 2048-bit is simply too costly for a lot of users. I am pretty sure that there is no use anymore trying to adapt the DH size to the RSA key size. We should probably remove this logic in 1.6 and just use the size specified by tune.ssl.default-dh-param, regardless of what the RSA key size is. I didn't have enough time to implement the 3 patches I promised yet, so here is the most important one for now, fixing the bug reported by Hervé Commowick. I think it should be backported to 1.5. I added a small patch for 1.6 that add a destructor to clean up at exit the memory allocated internally by OpenSSL. It makes my life easier when using valgrind to check for leaks, so I figured it could be of interest to others as well :) I expect to be able to send the ssl-dh-param-file patch tomorrow, as it is mostly written (but not well tested yet), as well as the patch to move from 1024-bit DH to 2048-bit by default. -- Rémi
From 621a4a07ab6e84d5c591adae5cdb0ef54c142ce9 Mon Sep 17 00:00:00 2001 From: Remi Gacogne <[email protected]> Date: Thu, 28 May 2015 16:23:00 +0200 Subject: [PATCH 1/2] BUG/MEDIUM: ssl: fix tune.ssl.default-dh-param value being overwritten MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hervé Commowick reported that the logic used to avoid complaining about ssl-default-dh-param not being set when static DH params are present in the certificate file was clearly wrong when more than one sni_ctx is used. This patch stores whether static DH params are being used for each SSL_CTX individually, and does not overwrite the value of tune.ssl.default-dh-param. --- src/ssl_sock.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 9302869..a6717a3 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -47,6 +47,9 @@ #if (defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined OPENSSL_NO_OCSP) #include <openssl/ocsp.h> #endif +#ifndef OPENSSL_NO_DH +#include <openssl/dh.h> +#endif #include <common/buffer.h> #include <common/compat.h> @@ -120,6 +123,7 @@ struct list tlskeys_reference = LIST_HEAD_INIT(tlskeys_reference); #endif #ifndef OPENSSL_NO_DH +static int ssl_dh_ptr_index = -1; static DH *local_dh_1024 = NULL; static DH *local_dh_2048 = NULL; static DH *local_dh_4096 = NULL; @@ -1347,10 +1351,12 @@ int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file) if (dh) { ret = 1; SSL_CTX_set_tmp_dh(ctx, dh); - /* Setting ssl default dh param to the size of the static DH params - found in the file. This way we know that there is no use - complaining later about ssl-default-dh-param not being set. */ - global.tune.ssl_default_dh_param = DH_size(dh) * 8; + + if (ssl_dh_ptr_index >= 0) { + /* store a pointer to the DH params to avoid complaining about + ssl-default-dh-param not being set for this SSL_CTX */ + SSL_CTX_set_ex_data(ctx, ssl_dh_ptr_index, dh); + } } else { /* Clear openssl global errors stack */ @@ -1545,6 +1551,12 @@ static int ssl_sock_load_cert_file(const char *path, struct bind_conf *bind_conf * the tree, so it will be discovered and cleaned in time. */ #ifndef OPENSSL_NO_DH + /* store a NULL pointer to indicate we have not yet loaded + a custom DH param file */ + if (ssl_dh_ptr_index >= 0) { + SSL_CTX_set_ex_data(ctx, ssl_dh_ptr_index, NULL); + } + ret = ssl_sock_load_dh_params(ctx, path); if (ret < 0) { if (err) @@ -1891,7 +1903,9 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, SSL_CTX *ctx, struct proxy /* If tune.ssl.default-dh-param has not been set and no static DH params were in the certificate file. */ - if (global.tune.ssl_default_dh_param == 0) { + if (global.tune.ssl_default_dh_param == 0 && + (ssl_dh_ptr_index == -1 || + SSL_CTX_get_ex_data(ctx, ssl_dh_ptr_index) == NULL)) { ssl = SSL_new(ctx); @@ -5040,6 +5054,10 @@ static void __ssl_sock_init(void) global.ssl_session_max_cost = SSL_SESSION_MAX_COST; global.ssl_handshake_max_cost = SSL_HANDSHAKE_MAX_COST; + +#ifndef OPENSSL_NO_DH + ssl_dh_ptr_index = SSL_CTX_get_ex_new_index(0, NULL, NULL, NULL, NULL); +#endif } /* -- 2.4.2
From fa18f916107c39c0f93ee9f0b244b746b6fbc625 Mon Sep 17 00:00:00 2001 From: Remi Gacogne <[email protected]> Date: Thu, 28 May 2015 16:39:47 +0200 Subject: [PATCH 2/2] MINOR: ssl: add a destructor to free allocated SSL ressources Using valgrind or another memory leak tracking tool is easier when the memory internally allocated by OpenSSL is cleanly released at shutdown. --- src/ssl_sock.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/ssl_sock.c b/src/ssl_sock.c index a6717a3..e528db2 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -5060,6 +5060,42 @@ static void __ssl_sock_init(void) #endif } +__attribute__((destructor)) +static void __ssl_sock_deinit(void) +{ +#ifndef OPENSSL_NO_DH + if (local_dh_1024) { + DH_free(local_dh_1024); + local_dh_1024 = NULL; + } + + if (local_dh_2048) { + DH_free(local_dh_2048); + local_dh_2048 = NULL; + } + + if (local_dh_4096) { + DH_free(local_dh_4096); + local_dh_4096 = NULL; + } + + if (local_dh_8192) { + DH_free(local_dh_8192); + local_dh_8192 = NULL; + } +#endif + + ERR_remove_state(0); + ERR_free_strings(); + + EVP_cleanup(); + +#if OPENSSL_VERSION_NUMBER >= 0x00907000L + CRYPTO_cleanup_all_ex_data(); +#endif +} + + /* * Local variables: * c-indent-level: 8 -- 2.4.2
signature.asc
Description: OpenPGP digital signature
