On 10/05/2016 09:57 PM, Heikki Linnakangas wrote:
On 10/05/2016 05:15 PM, Nicolas Guini wrote:
We are working with Postgres 9.3.14 and executing nmap we
found that it is using “weak DH group” (nmap –script ssl-dh-params). Weak =
1024 bits.
Yeah, it seems that we're a bit behind the times on this...
This issue is similar to what this post explains about using weak DH
parameters: http://www.usefuljs.net/2016/09/29/imperfect-forward-secrecy/
The blog post points out that, as counterintuitive as it sounds, the
SSL_CTX_set_tmp_dh_callback() callback should ignore the keylength
argument, and always return a DH group with 2048 bits, or stronger. As
you pointed out, that's not what our callback does.
I propose the attached patch. It gives up on trying to deal with
multiple key lengths (as noted earlier, OpenSSL just always passed
keylength=1024, so that was useless). Instead of using the callback, it
just sets fixed DH parameters with SSL_CTX_set_tmp_dh(), like we do for
the ECDH curve. The DH parameters are loaded from a file called
"dh_params.pem" (instead of "dh1024.pem"), if present, otherwise the
built-in 2048 bit parameters are used.
I removed the code for generating DH parameters on-the-fly altogether.
The OpenSSL man page clearly says that that should not be done, because
generating the parameters takes a long time. And because OpenSSL always
passed keylength=1024, AFAICS that had been always dead code.
If we want to get really fancy, we could generate the parameters the
first time you turn ssl=on, and store them in $PGDATA. But the
generation takes a very long time, so the admin might think it's stuck.
I note that supplying custom DH parameters in the file is completely
undocumented :-(. We should add a note in the docs on how to generate
the custom DH parameters with openssl. Also, there's no easy way of
telling if your custom parameters are actually been used. If you copy
the params file in $PGDATA, but you have a typo in the name, you won't
notice. Perhaps we should print a line in the log when the file is loaded.
I'm afraid of back-patching this, out of fear that older clients would
stop working, or would downgrade to an even weaker cipher. You could fix
it by putting weaker 1024 bit params in dh_params.pem, so maybe we could
do it if we put a warning and instructions for doing that in the release
notes. Thoughts?
- Heikki
>From 69f74e3cced093c9ae2cce830e31c3fd76a8a06b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@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-l2...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
