>From 69f74e3cced093c9ae2cce830e31c3fd76a8a06b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 6 Oct 2016 16:22:29 +0300
Subject: [PATCH 1/1] Always use 2048 bit DH parameters for OpenSSL ephemeral
 DH ciphers.

1024 bits is considered weak these days, but OpenSSL always passes 1024 as
the key length to the tmp_dh callback. All the code to handle other key
lengths as in fact dead.

To remedy those issues:

* Only include hard-coded 2048-bit parameters.
* Set the parameters directly with SSL_CTX_set_tmp_dh(), without the
  callback
* The file for the DH parameters is now called "dh_params.pem", instead of
  "dh1024.pem" (the files for other key lengths, dh512.pem, dh2048.pem, etc.
  were never actually used)

Per report by Nicolas Guini

Discussion: <CAMxBoUyjOOautVozN6ofzym828aNrDjuCcOTcCquxjwS-L2hGQ@mail.gmail.com>
---
 src/backend/libpq/be-secure-openssl.c | 198 +++++++++-------------------------
 1 file changed, 48 insertions(+), 150 deletions(-)

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 668f217..fc7dd6a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -65,18 +65,19 @@
 #include "tcop/tcopprot.h"
 #include "utils/memutils.h"
 
+/* name of the file containing DH parameters */
+#define DH_PARAMS_FILE	"dh_params.pem"
 
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
 static BIO_METHOD *my_BIO_s_socket(void);
 static int	my_SSL_set_fd(Port *port, int fd);
 
-static DH  *load_dh_file(int keylength);
+static DH  *load_dh_file(void);
 static DH  *load_dh_buffer(const char *, size_t);
-static DH  *generate_dh_parameters(int prime_len, int generator);
-static DH  *tmp_dh_cb(SSL *s, int is_export, int keylength);
 static int	verify_cb(int, X509_STORE_CTX *);
 static void info_cb(const SSL *ssl, int type, int args);
+static void initialize_dh(void);
 static void initialize_ecdh(void);
 static const char *SSLerrmessage(unsigned long ecode);
 
@@ -94,14 +95,14 @@ static SSL_CTX *SSL_context = NULL;
  *	sessions even if the static private key is compromised,
  *	so we are *highly* motivated to ensure that we can use
  *	EDH even if the DBA... or an attacker... deletes the
- *	$DataDir/dh*.pem files.
+ *	$DataDir/dh_params.pem file.
  *
  *	We could refuse SSL connections unless a good DH parameter
  *	file exists, but some clients may quietly renegotiate an
  *	unsecured connection without fully informing the user.
  *	Very uncool.
  *
- *	Alternatively, the backend could attempt to load these files
+ *	Alternatively, the backend could attempt to load the file
  *	on startup if SSL is enabled - and refuse to start if any
  *	do not exist - but this would tend to piss off DBAs.
  *
@@ -111,19 +112,6 @@ static SSL_CTX *SSL_context = NULL;
  *	for suggestions.
  */
 
-static const char file_dh512[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MEYCQQD1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6ypUM2Zafq9AKUJsCRtMIPWak\n\
-XUGfnHy9iUsiGSa6q6Jew1XpKgVfAgEC\n\
------END DH PARAMETERS-----\n";
-
-static const char file_dh1024[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIGHAoGBAPSI/VhOSdvNILSd5JEHNmszbDgNRR0PfIizHHxbLY7288kjwEPwpVsY\n\
-jY67VYy4XTjTNP18F1dDox0YbN4zISy1Kv884bEpQBgRjXyEpwpy1obEAxnIByl6\n\
-ypUM2Zafq9AKUJsCRtMIPWakXUGfnHy9iUsiGSa6q6Jew1XpL3jHAgEC\n\
------END DH PARAMETERS-----\n";
-
 static const char file_dh2048[] =
 "-----BEGIN DH PARAMETERS-----\n\
 MIIBCAKCAQEA9kJXtwh/CBdyorrWqULzBej5UxE5T7bxbrlLOCDaAadWoxTpj0BV\n\
@@ -134,21 +122,6 @@ Q6MdGGzeMyEstSr/POGxKUAYEY18hKcKctaGxAMZyAcpesqVDNmWn6vQClCbAkbT\n\
 CD1mpF1Bn5x8vYlLIhkmuquiXsNV6TILOwIBAg==\n\
 -----END DH PARAMETERS-----\n";
 
-static const char file_dh4096[] =
-"-----BEGIN DH PARAMETERS-----\n\
-MIICCAKCAgEA+hRyUsFN4VpJ1O8JLcCo/VWr19k3BCgJ4uk+d+KhehjdRqNDNyOQ\n\
-l/MOyQNQfWXPeGKmOmIig6Ev/nm6Nf9Z2B1h3R4hExf+zTiHnvVPeRBhjdQi81rt\n\
-Xeoh6TNrSBIKIHfUJWBh3va0TxxjQIs6IZOLeVNRLMqzeylWqMf49HsIXqbcokUS\n\
-Vt1BkvLdW48j8PPv5DsKRN3tloTxqDJGo9tKvj1Fuk74A+Xda1kNhB7KFlqMyN98\n\
-VETEJ6c7KpfOo30mnK30wqw3S8OtaIR/maYX72tGOno2ehFDkq3pnPtEbD2CScxc\n\
-alJC+EL7RPk5c/tgeTvCngvc1KZn92Y//EI7G9tPZtylj2b56sHtMftIoYJ9+ODM\n\
-sccD5Piz/rejE3Ome8EOOceUSCYAhXn8b3qvxVI1ddd1pED6FHRhFvLrZxFvBEM9\n\
-ERRMp5QqOaHJkM+Dxv8Cj6MqrCbfC4u+ZErxodzuusgDgvZiLF22uxMZbobFWyte\n\
-OvOzKGtwcTqO/1wV5gKkzu1ZVswVUQd5Gg8lJicwqRWyyNRczDDoG9jVDxmogKTH\n\
-AaqLulO7R8Ifa1SwF2DteSGVtgWEN8gDpN3RBmmPTDngyF2DHb5qmpnznwtFKdTL\n\
-KWbuHn491xNO25CQWMtem80uKw+pTnisBRF/454n1Jnhub144YRBoN8CAQI=\n\
------END DH PARAMETERS-----\n";
-
 
 /* ------------------------------------------------------------ */
 /*						 Public interface						*/
@@ -261,13 +234,12 @@ be_tls_init(void)
 							SSLerrmessage(ERR_get_error()))));
 	}
 
-	/* set up ephemeral DH keys, and disallow SSL v2/v3 while at it */
-	SSL_CTX_set_tmp_dh_callback(SSL_context, tmp_dh_cb);
+	/*  disallow SSL v2/v3 */
 	SSL_CTX_set_options(SSL_context,
-						SSL_OP_SINGLE_DH_USE |
 						SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
 
-	/* set up ephemeral ECDH keys */
+	/* set up ephemeral DH and ECDH keys */
+	initialize_dh();
 	initialize_ecdh();
 
 	/* set up the allowed cipher list */
@@ -800,43 +772,32 @@ err:
  *	what we expect it to contain.
  */
 static DH  *
-load_dh_file(int keylength)
+load_dh_file(void)
 {
 	FILE	   *fp;
-	char		fnbuf[MAXPGPATH];
+	char	   *filename = DH_PARAMS_FILE;
 	DH		   *dh = NULL;
 	int			codes;
 
 	/* attempt to open file.  It's not an error if it doesn't exist. */
-	snprintf(fnbuf, sizeof(fnbuf), "dh%d.pem", keylength);
-	if ((fp = fopen(fnbuf, "r")) == NULL)
+	if ((fp = fopen(filename, "r")) == NULL)
 		return NULL;
 
-/*	flock(fileno(fp), LOCK_SH); */
 	dh = PEM_read_DHparams(fp, NULL, NULL, NULL);
-/*	flock(fileno(fp), LOCK_UN); */
 	fclose(fp);
 
-	/* is the prime the correct size? */
-	if (dh != NULL && 8 * DH_size(dh) < keylength)
-	{
-		elog(LOG, "DH errors (%s): %d bits expected, %d bits found",
-			 fnbuf, keylength, 8 * DH_size(dh));
-		dh = NULL;
-	}
-
 	/* make sure the DH parameters are usable */
 	if (dh != NULL)
 	{
 		if (DH_check(dh, &codes) == 0)
 		{
-			elog(LOG, "DH_check error (%s): %s", fnbuf,
+			elog(LOG, "DH_check error (%s): %s", filename,
 				 SSLerrmessage(ERR_get_error()));
 			return NULL;
 		}
 		if (codes & DH_CHECK_P_NOT_PRIME)
 		{
-			elog(LOG, "DH error (%s): p is not prime", fnbuf);
+			elog(LOG, "DH error (%s): p is not prime", filename);
 			return NULL;
 		}
 		if ((codes & DH_NOT_SUITABLE_GENERATOR) &&
@@ -844,7 +805,7 @@ load_dh_file(int keylength)
 		{
 			elog(LOG,
 				 "DH error (%s): neither suitable generator or safe prime",
-				 fnbuf);
+				 filename);
 			return NULL;
 		}
 	}
@@ -878,102 +839,6 @@ load_dh_buffer(const char *buffer, size_t len)
 }
 
 /*
- *	Generate DH parameters.
- *
- *	Last resort if we can't load precomputed nor hardcoded
- *	parameters.
- */
-static DH  *
-generate_dh_parameters(int prime_len, int generator)
-{
-	DH		   *dh;
-
-	if ((dh = DH_new()) == NULL)
-		return NULL;
-
-	if (DH_generate_parameters_ex(dh, prime_len, generator, NULL))
-		return dh;
-
-	DH_free(dh);
-	return NULL;
-}
-
-/*
- *	Generate an ephemeral DH key.  Because this can take a long
- *	time to compute, we can use precomputed parameters of the
- *	common key sizes.
- *
- *	Since few sites will bother to precompute these parameter
- *	files, we also provide a fallback to the parameters provided
- *	by the OpenSSL project.
- *
- *	These values can be static (once loaded or computed) since
- *	the OpenSSL library can efficiently generate random keys from
- *	the information provided.
- */
-static DH  *
-tmp_dh_cb(SSL *s, int is_export, int keylength)
-{
-	DH		   *r = NULL;
-	static DH  *dh = NULL;
-	static DH  *dh512 = NULL;
-	static DH  *dh1024 = NULL;
-	static DH  *dh2048 = NULL;
-	static DH  *dh4096 = NULL;
-
-	switch (keylength)
-	{
-		case 512:
-			if (dh512 == NULL)
-				dh512 = load_dh_file(keylength);
-			if (dh512 == NULL)
-				dh512 = load_dh_buffer(file_dh512, sizeof file_dh512);
-			r = dh512;
-			break;
-
-		case 1024:
-			if (dh1024 == NULL)
-				dh1024 = load_dh_file(keylength);
-			if (dh1024 == NULL)
-				dh1024 = load_dh_buffer(file_dh1024, sizeof file_dh1024);
-			r = dh1024;
-			break;
-
-		case 2048:
-			if (dh2048 == NULL)
-				dh2048 = load_dh_file(keylength);
-			if (dh2048 == NULL)
-				dh2048 = load_dh_buffer(file_dh2048, sizeof file_dh2048);
-			r = dh2048;
-			break;
-
-		case 4096:
-			if (dh4096 == NULL)
-				dh4096 = load_dh_file(keylength);
-			if (dh4096 == NULL)
-				dh4096 = load_dh_buffer(file_dh4096, sizeof file_dh4096);
-			r = dh4096;
-			break;
-
-		default:
-			if (dh == NULL)
-				dh = load_dh_file(keylength);
-			r = dh;
-	}
-
-	/* this may take a long time, but it may be necessary... */
-	if (r == NULL || 8 * DH_size(r) < keylength)
-	{
-		ereport(DEBUG2,
-				(errmsg_internal("DH: generating parameters (%d bits)",
-								 keylength)));
-		r = generate_dh_parameters(keylength, DH_GENERATOR_2);
-	}
-
-	return r;
-}
-
-/*
  *	Certificate verification callback
  *
  *	This callback allows us to log intermediate problems during
@@ -1034,6 +899,39 @@ info_cb(const SSL *ssl, int type, int args)
 	}
 }
 
+/*
+ * Obtain DH parameters for generating ephemeral DH keys.  Because
+ * this can take a long time to compute, these must be precomputed.
+ *
+ * Since few sites will bother to precompute these parameter files,
+ * we also also provide a fallback to the parameters provided by the
+ * OpenSSL project.
+ *
+ * These values can be static (once loaded or computed) since the
+ * OpenSSL library can efficiently generate random keys from the
+ * information provided.
+ */
+static void
+initialize_dh(void)
+{
+	DH		   *dh;
+
+	dh = load_dh_file();
+	if (!dh)
+		dh = load_dh_buffer(file_dh2048, sizeof file_dh2048);
+	if (!dh)
+		ereport(FATAL,
+				(errmsg("DH: could not create key")));
+
+	SSL_CTX_set_options(SSL_context, SSL_OP_SINGLE_DH_USE);
+	SSL_CTX_set_tmp_dh(SSL_context, dh);
+}
+
+/*
+ * Set ECDH parameters for generating ephemeral Elliptic Curve DH
+ * keys.  This is much simpler than the DH parameters, as we just
+ * need to provide the name of the curve to OpenSSL.
+ */
 static void
 initialize_ecdh(void)
 {
-- 
2.9.3

