Re: PostgreSQL - Weak DH group

(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(at)iki(dot)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