Hi, It was recently reported to the Apache HTTPd dev mailing list [1] that the DH * value returned by the callback set with SSL_CTX_set_tmp_dh_callback() is not freed by OpenSSL but must be tracked by the application and freed when no longer needed, whenever that is. Unfortunately this behavior is not clearly documented and the example included in the documentation [2] doesn't do that.
As a result, the patch I submitted earlier in haproxy 1.5 leads haproxy to leak memory for SSL/TLS connections using Diffie Hellman Ephemeral key exchange, as the DH * struct holding the DH parameters is not freed by OpenSSL. This patch fixes the leak by allocating the DH * structs holding the DH parameters once, at configuration time, as it has been done in Apache HTTPd. I am not very satisfied by the fact that it uses 4 file-scope variables, but I didn't see where else to store the DH parameters. Willy, Emeric, do you have any comment? I am sorry I didn't catch that earlier. Regards, [1] http://marc.info/?l=apache-httpd-dev&m=140031858223194&w=2 [2] https://www.openssl.org/docs/ssl/SSL_CTX_set_tmp_dh_callback.html -- Rémi
From 1470d3081fea6c6bd5b6103ef04e4fc78c8428ac Mon Sep 17 00:00:00 2001
From: Remi Gacogne <rgacogne[at]aquaray[dot]fr>
Date: Tue, 15 Jul 2014 11:36:40 +0200
Subject: [PATCH] Fix a memory leak in DHE key exchange
OpenSSL does not free the DH * value returned by the callback specified with SSL_CTX_set_tmp_dh_callback(),
leading to a memory leak for SSL/TLS connections using Diffie Hellman Ephemeral key exchange.
This patch fixes the leak by allocating the DH * structs holding the DH parameters once, at configuration time.
---
src/ssl_sock.c | 43 ++++++++++++++++++++++++++++++++++++-------
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 375225d..cf8adc7 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -105,6 +105,13 @@ enum {
int sslconns = 0;
int totalsslconns = 0;
+#ifndef OPENSSL_NO_DH
+static DH *local_dh_1024 = NULL;
+static DH *local_dh_2048 = NULL;
+static DH *local_dh_4096 = NULL;
+static DH *local_dh_8192 = NULL;
+#endif /* OPENSSL_NO_DH */
+
#ifdef SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB
struct certificate_ocsp {
struct ebmb_node key;
@@ -1034,16 +1041,16 @@ static DH *ssl_get_tmp_dh(SSL *ssl, int export, int keylen)
}
if (keylen >= 8192) {
- dh = ssl_get_dh_8192();
+ dh = local_dh_8192;
}
else if (keylen >= 4096) {
- dh = ssl_get_dh_4096();
+ dh = local_dh_4096;
}
else if (keylen >= 2048) {
- dh = ssl_get_dh_2048();
+ dh = local_dh_2048;
}
else {
- dh = ssl_get_dh_1024();
+ dh = local_dh_1024;
}
return dh;
@@ -1079,11 +1086,11 @@ int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
if (global.tune.ssl_default_dh_param <= 1024) {
/* we are limited to DH parameter of 1024 bits anyway */
- dh = ssl_get_dh_1024();
- if (dh == NULL)
+ local_dh_1024 = ssl_get_dh_1024();
+ if (local_dh_1024 == NULL)
goto end;
- SSL_CTX_set_tmp_dh(ctx, dh);
+ SSL_CTX_set_tmp_dh(ctx, local_dh_1024);
}
else {
SSL_CTX_set_tmp_dh_callback(ctx, ssl_get_tmp_dh);
@@ -1594,6 +1601,28 @@ int ssl_sock_prepare_ctx(struct bind_conf *bind_conf, SSL_CTX *ctx, struct proxy
global.tune.ssl_default_dh_param = 1024;
}
+#ifndef OPENSSL_NO_DH
+ if (global.tune.ssl_default_dh_param >= 1024) {
+ if (local_dh_1024 == NULL) {
+ local_dh_1024 = ssl_get_dh_1024();
+ }
+ if (global.tune.ssl_default_dh_param >= 2048) {
+ if (local_dh_2048 == NULL) {
+ local_dh_2048 = ssl_get_dh_2048();
+ }
+ if (global.tune.ssl_default_dh_param >= 4096) {
+ if (local_dh_4096 == NULL) {
+ local_dh_4096 = ssl_get_dh_4096();
+ }
+ if (global.tune.ssl_default_dh_param >= 8192 &&
+ local_dh_8192 == NULL) {
+ local_dh_8192 = ssl_get_dh_8192();
+ }
+ }
+ }
+ }
+#endif /* OPENSSL_NO_DH */
+
SSL_CTX_set_info_callback(ctx, ssl_sock_infocbk);
#if OPENSSL_VERSION_NUMBER >= 0x00907000L
SSL_CTX_set_msg_callback(ctx, ssl_sock_msgcbk);
--
2.0.1
signature.asc
Description: OpenPGP digital signature
