Hi,

This is an updated version of the previous patch. This version adds a
new configuration option named "tune.ssl.max-dh-param-size", which sets
the maximum size of the ephemeral DH key used for DHE key exchange, if
no static DH parameters are found in the certificate file.

The default value for max-dh-param-size is set to 1024, thus keeping
the current behavior by default. Setting a higher value (for example
2048 with a 2048 bits RSA/DSA server key) allows an easy upgrade
to stronger ephemeral DH keys (and back if needed).

Regarding the recent discussions on this matter, I agree with the fact
that ECDHE should be preferred over DHE whenever it is available, but
I think we may want to keep offering a decent forward secrecy option to
clients not supporting elliptic curves yet, for example older versions
of Android or clients using OpenSSL 0.9.8.

This patch is a proposal based on the feedback I got from the previous
one, please feel free to criticize anything from the core idea (an easy
way to use stronger DHE key size) to the new parameter's name, I will
gladly welcome any remarks :)


Regards,

-- 
Rémi Gacogne

Aqua Ray
SAS au capital de 105.720 Euros
RCS Créteil 447 997 099
www.aquaray.fr

14, rue Jules Vanzuppe
94854 IVRY-SUR-SEINE CEDEX (France)
Tel : (+33) (0)1 84 04 04 05
Fax : (+33) (0)1 77 65 60 42
From f03b547984c513855383b11dc76aaecbbbc65838 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <rgacogne[at]aquaray[dot]fr>
Date: Fri, 2 May 2014 15:41:13 +0200
Subject: [PATCH] Add a configurable support of standardized DH parameters >=
 1024 bits, disabled by default

When no static DH parameters are specified, this patch makes haproxy
use standardized (rfc 2409 / rfc 3526) DH parameters with prime lenghts
of 1024, 2048, 4096 and 8192 bits for DHE key exchange. The size of the
temporary/ephemeral DH key is computed as the minimum of the RSA/DSA server
key size and the value of a new option named tune.ssl.max-dh-param-size.
---
 doc/configuration.txt     |  11 ++++
 include/common/defaults.h |   5 ++
 include/types/global.h    |   1 +
 src/cfgparse.c            |   8 +++
 src/haproxy.c             |   1 +
 src/ssl_sock.c            | 154 ++++++++++++++++++++++++++++++++++------------
 6 files changed, 142 insertions(+), 38 deletions(-)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index 8207067..1c9e4e6 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -496,6 +496,7 @@ The following keywords are supported in the "global" section :
    - tune.ssl.cachesize
    - tune.ssl.lifetime
    - tune.ssl.maxrecord
+   - tune.ssl.max-dh-param-size
    - tune.zlib.memlevel
    - tune.zlib.windowsize
 
@@ -1006,6 +1007,16 @@ tune.ssl.maxrecord <number>
   best value. Haproxy will automatically switch to this setting after an idle
   stream has been detected (see tune.idletimer above).
 
+tune.ssl.max-dh-param-size <number>
+ Sets the maximum size of the Diffie-Hellman parameters used for generating
+ 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 1024. 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 via the certificate file.
+
 tune.zlib.memlevel <number>
   Sets the memLevel parameter in zlib initialization for each session. It
   defines how much memory should be allocated for the internal compression
diff --git a/include/common/defaults.h b/include/common/defaults.h
index f765e90..944f3aa 100644
--- a/include/common/defaults.h
+++ b/include/common/defaults.h
@@ -214,4 +214,9 @@
 #define SSLCACHESIZE 20000
 #endif
 
+/* ssl max dh param size */
+#ifndef SSL_MAX_DH_PARAM
+#define SSL_MAX_DH_PARAM 1024
+#endif
+
 #endif /* _COMMON_DEFAULTS_H */
diff --git a/include/types/global.h b/include/types/global.h
index 241afe9..2fba7ca 100644
--- a/include/types/global.h
+++ b/include/types/global.h
@@ -131,6 +131,7 @@ struct global {
 		int sslcachesize;  /* SSL cache size in session, defaults to 20000 */
 		unsigned int ssllifetime;   /* SSL session lifetime in seconds */
 		unsigned int ssl_max_record; /* SSL max record size */
+		unsigned int ssl_max_dh_param; /* SSL maximum DH parameter size */
 #endif
 #ifdef USE_ZLIB
 		int zlibmemlevel;    /* zlib memlevel */
diff --git a/src/cfgparse.c b/src/cfgparse.c
index c4f092f..9a976ba 100644
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -630,6 +630,14 @@ int cfg_parse_global(const char *file, int linenum, char **args, int kwm)
 		}
 		global.tune.ssl_max_record = atol(args[1]);
 	}
+	else if (!strcmp(args[0], "tune.ssl.max-dh-param-size")) {
+		if (*(args[1]) == 0) {
+			Alert("parsing [%s:%d] : '%s' expects an integer argument.\n", file, linenum, args[0]);
+			err_code |= ERR_ALERT | ERR_FATAL;
+			goto out;
+		}
+		global.tune.ssl_max_dh_param = atol(args[1]);
+	}
 #endif
 	else if (!strcmp(args[0], "tune.bufsize")) {
 		if (*(args[1]) == 0) {
diff --git a/src/haproxy.c b/src/haproxy.c
index 81eaeba..489a0d7 100644
--- a/src/haproxy.c
+++ b/src/haproxy.c
@@ -143,6 +143,7 @@ struct global global = {
 		.chksize = BUFSIZE,
 #ifdef USE_OPENSSL
 		.sslcachesize = SSLCACHESIZE,
+		.ssl_max_dh_param = SSL_MAX_DH_PARAM,
 #ifdef DEFAULT_SSL_MAX_RECORD
 		.ssl_max_record = DEFAULT_SSL_MAX_RECORD,
 #endif
diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 229290f..b084fc5 100644
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -314,6 +314,107 @@ static int ssl_sock_switchctx_cbk(SSL *ssl, int *al, struct bind_conf *s)
 #endif /* SSL_CTRL_SET_TLSEXT_HOSTNAME */
 
 #ifndef OPENSSL_NO_DH
+
+static DH * ssl_get_dh_1024(void)
+{
+	DH *dh = DH_new();
+	if (dh) {
+		dh->p = get_rfc2409_prime_1024(NULL);
+		/* See RFC 2409, Section 6 "Oakley Groups"
+		   for the reason why we use 2 as a generator.
+		*/
+		BN_dec2bn(&dh->g, "2");
+		if (!dh->p || !dh->g) {
+			DH_free(dh);
+			dh = NULL;
+		}
+	}
+	return dh;
+}
+
+static DH *ssl_get_dh_2048(void)
+{
+	DH *dh = DH_new();
+	if (dh) {
+		dh->p = get_rfc3526_prime_2048(NULL);
+		/* See RFC 3526, Section 3 "2048-bit MODP Group"
+		   for the reason why we use 2 as a generator.
+		*/
+		BN_dec2bn(&dh->g, "2");
+		if (!dh->p || !dh->g) {
+			DH_free(dh);
+			dh = NULL;
+		}
+	}
+	return dh;
+}
+
+static DH *ssl_get_dh_4096(void)
+{
+	DH *dh = DH_new();
+	if (dh) {
+		dh->p = get_rfc3526_prime_4096(NULL);
+		/* See RFC 3526, Section 5 "4096-bit MODP Group"
+		   for the reason why we use 2 as a generator.
+		*/
+		BN_dec2bn(&dh->g, "2");
+		if (!dh->p || !dh->g) {
+			DH_free(dh);
+			dh = NULL;
+		}
+	}
+	return dh;
+}
+
+static DH *ssl_get_dh_8192(void)
+{
+	DH *dh = DH_new();
+	if (dh) {
+		dh->p = get_rfc3526_prime_8192(NULL);
+		/* See RFC 3526, Section 7 "8192-bit MODP Group"
+		   for the reason why we use 2 as a generator.
+		*/
+		BN_dec2bn(&dh->g, "2");
+		if (!dh->p || !dh->g) {
+			DH_free(dh);
+			dh = NULL;
+		}
+	}
+	return dh;
+}
+
+/* Returns Diffie-Hellman parameters matching the private key length
+   but not exceeding global.tune.ssl_max_dh_param size */
+static DH *ssl_get_tmp_dh(SSL *ssl, int export, int keylen)
+{
+	DH *dh = NULL;
+	EVP_PKEY *pkey = SSL_get_privatekey(ssl);
+	int type = pkey ? EVP_PKEY_type(pkey->type) : EVP_PKEY_NONE;
+
+	if (type == EVP_PKEY_RSA || type == EVP_PKEY_DSA) {
+		keylen = EVP_PKEY_bits(pkey);
+	}
+
+	if (keylen > global.tune.ssl_max_dh_param) {
+		keylen = global.tune.ssl_max_dh_param;
+	}
+
+	if (keylen >= 8192) {
+		dh = ssl_get_dh_8192();
+	}
+	else if (keylen >= 4096) {
+		dh = ssl_get_dh_4096();
+	}
+	else if (keylen >= 2048) {
+		dh = ssl_get_dh_2048();
+	}
+	else {
+		dh = ssl_get_dh_1024();
+	}
+
+	return dh;
+}
+
 /* Loads Diffie-Hellman parameter from a file. Returns 1 if loaded, else -1
    if an error occured, and 0 if parameter not found. */
 int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
@@ -321,29 +422,6 @@ int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
 	int ret = -1;
 	BIO *in;
 	DH *dh = NULL;
-	/* If not present, use parameters generated using 'openssl dhparam 1024 -C':
-	 * -----BEGIN DH PARAMETERS-----
-	 * MIGHAoGBAJJAJDXDoS5E03MNjnjK36eOL1tRqVa/9NuOVlI+lpXmPjJQbP65EvKn
-	 * fSLnG7VMhoCJO4KtG88zf393ltP7loGB2bofcDSr+x+XsxBM8yA/Zj6BmQt+CQ9s
-	 * TF7hoOV+wXTT6ErZ5y5qx9pq6hLfKXwTGFT78hrE6HnCO7xgtPdTAgEC
-	 * -----END DH PARAMETERS-----
-	*/
-	static const unsigned char dh1024_p[] = {
-		0x92, 0x40, 0x24, 0x35, 0xC3, 0xA1, 0x2E, 0x44, 0xD3, 0x73, 0x0D, 0x8E,
-		0x78, 0xCA, 0xDF, 0xA7, 0x8E, 0x2F, 0x5B, 0x51, 0xA9, 0x56, 0xBF, 0xF4,
-		0xDB, 0x8E, 0x56, 0x52, 0x3E, 0x96, 0x95, 0xE6, 0x3E, 0x32, 0x50, 0x6C,
-		0xFE, 0xB9, 0x12, 0xF2, 0xA7, 0x7D, 0x22, 0xE7, 0x1B, 0xB5, 0x4C, 0x86,
-		0x80, 0x89, 0x3B, 0x82, 0xAD, 0x1B, 0xCF, 0x33, 0x7F, 0x7F, 0x77, 0x96,
-		0xD3, 0xFB, 0x96, 0x81, 0x81, 0xD9, 0xBA, 0x1F, 0x70, 0x34, 0xAB, 0xFB,
-		0x1F, 0x97, 0xB3, 0x10, 0x4C, 0xF3, 0x20, 0x3F, 0x66, 0x3E, 0x81, 0x99,
-		0x0B, 0x7E, 0x09, 0x0F, 0x6C, 0x4C, 0x5E, 0xE1, 0xA0, 0xE5, 0x7E, 0xC1,
-		0x74, 0xD3, 0xE8, 0x4A, 0xD9, 0xE7, 0x2E, 0x6A, 0xC7, 0xDA, 0x6A, 0xEA,
-		0x12, 0xDF, 0x29, 0x7C, 0x13, 0x18, 0x54, 0xFB, 0xF2, 0x1A, 0xC4, 0xE8,
-		0x79, 0xC2, 0x3B, 0xBC, 0x60, 0xB4, 0xF7, 0x53,
-	};
-	static const unsigned char dh1024_g[] = {
-		0x02,
-	};
 
 	in = BIO_new(BIO_s_file());
 	if (in == NULL)
@@ -353,28 +431,28 @@ int ssl_sock_load_dh_params(SSL_CTX *ctx, const char *file)
 		goto end;
 
 	dh = PEM_read_bio_DHparams(in, NULL, ctx->default_passwd_callback, ctx->default_passwd_callback_userdata);
-	if (!dh) {
+	if (dh) {
+		ret = 1;
+		SSL_CTX_set_tmp_dh(ctx, dh);
+	}
+	else {
 		/* Clear openssl global errors stack */
 		ERR_clear_error();
 
-		dh = DH_new();
-		if (dh == NULL)
-			goto end;
-
-		dh->p = BN_bin2bn(dh1024_p, sizeof(dh1024_p), NULL);
-		if (dh->p == NULL)
-			goto end;
+		if (global.tune.ssl_max_dh_param <= 1024) {
+			/* we are limited to DH parameter of 1024 bits anyway */
+			dh = ssl_get_dh_1024();
+			if (dh == NULL)
+				goto end;
 
-		dh->g = BN_bin2bn(dh1024_g, sizeof(dh1024_g), NULL);
-		if (dh->g == NULL)
-			goto end;
+			SSL_CTX_set_tmp_dh(ctx, dh);
+		}
+		else {
+			SSL_CTX_set_tmp_dh_callback(ctx, ssl_get_tmp_dh);
+		}
 
 		ret = 0; /* DH params not found */
 	}
-	else
-		ret = 1;
-
-	SSL_CTX_set_tmp_dh(ctx, dh);
 
 end:
 	if (dh)
-- 
1.9.2

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to