Re: [HACKERS] PostgreSQL - Weak DH group - Mailing list pgsql-hackers

From Heikki Linnakangas
Subject Re: [HACKERS] PostgreSQL - Weak DH group
Date
Msg-id 6a8fb497-17ac-f7b2-cab6-4d7175e3778c@iki.fi
Whole thread Raw
In response to Re: PostgreSQL - Weak DH group  (Heikki Linnakangas <hlinnaka@iki.fi>)
Responses Re: [HACKERS] PostgreSQL - Weak DH group
Re: [HACKERS] PostgreSQL - Weak DH group
List pgsql-hackers
(We dropped the ball back in October, continuing the discussion now)

On 10/10/2016 06:24 PM, Heikki Linnakangas wrote:
> On 10/06/2016 10:26 PM, Christoph Berg wrote:
>> Re: Heikki Linnakangas 2016-10-06 <fd6eb3cc-1585-1469-fd9e-763f8e410b19@iki.fi>
>>> 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.
>>
>> Shouldn't this be a GUC pointing to a configurable location like
>> ssl_cert_file? This way, people reading the security section of the
>> default postgresql.conf would notice that there's something (new) to
>> configure. (And I wouldn't want to start creating symlinks from PGDATA
>> to /etc/ssl/something again...)
>
> Perhaps. The DH parameters are not quite like other configuration files,
> though. The actual parameters used don't matter, you just want to
> generate different parameters for every cluster. The key length of the
> parameters can be considered configuration, though.

Actually, adding a GUC also solves another grief I had about this. There 
is currently no easy way to tell if your parameter file is being used, 
or if the server is failing to read it and is falling back to the 
hard-coded parameters. With a GUC, if the GUC is set it's a good 
indication that the admin expects the file to really exist and to 
contain valid parameters. So if the GUC is set, we should throw an error 
if it cannot be used. And if it's not set, we use the built-in defaults.

I rebased the patch, did some other clean up of error reporting, and 
added a GUC along those lines, as well as docs. How does this look?

It's late in the release cycle, but it would be nice to sneak this into 
v10. Using weak 1024 bit DH parameters is arguably a security issue; it 
was originally reported as such. There's a work-around for older 
versions: generate custom 2048 bit parameters and place them in a file 
called "dh1024.pem", but that's completely undocumented.

Thoughts? Objections to committing this now, instead of waiting for v11?

- Heikki


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

pgsql-hackers by date:

Previous
From: Jeroen Ooms
Date:
Subject: Re: [HACKERS] building libpq.a static library
Next
From: Alvaro Herrera
Date:
Subject: Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately inplanner.c