Re: PostgreSQL - Weak DH group

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