Re: Replacing the EDH SKIP primes

On Wed, Jul 03, 2019 at 10:56:41AM +0200, Daniel Gustafsson wrote:
> OpenSSL provides DH_check() which we use in load_dh_file() to ensure that the
> user is passing a valid prime in the DH file. Adding this to the hardcoded
> blob seems overkill though, once the validity has been verified before it being
> committed.

Agreed, and I didn't notice this check... There could be an argument
for having DH_check within an assertion block but as this changes once
per decade there is little value.

> A DH param in PEM (or DER) format can be checked with the openssl dhparam tool.
> Assuming the PEM is extracted from the patch into a file, one can do:
>
> openssl dhparam -inform PEM -in /tmp/dh.pem -check -text
>
> The prime is returned and can be diffed against the one in the RFC. If you
> modify the blob you will see that the check complains about it not being prime.

Ah, thanks. I can see that the new key matches the RFC.

> There is an expected warning in the output however: "the g value is not a
> generator” (this is also present when subjecting the PEM for the 2048 MODP in
> OpenSSL).

Indeed, I saw that also from OpenSSL. That looks to come from dh.c
(there are two other code paths, haven't checked in details). Thanks
for the pointer.

> I’m far from a cryptographer, but AFAICT from reading it essentially means that
> the RFC authors chose to limit the search space of the shared secret rather
> than leaking a bit of it, and OpenSSL has chosen in DH_check() that leaking a
> bit is preferrable. (This makes me wonder if we should downgrade the check in
> load_dh_file() "codes & DH_NOT_SUITABLE_GENERATOR” to WARNING, but the lack of
> reports of it being a problem either shows that most people are just using
> openssl dhparam generated parameters which can leak a bit, or aren’t using DH
> files at all.)

Yeah, no objections per the arguments from the RFC. I am not sure if
we actually need to change that part though. We'd still get a
complaint for a key which is not a prime, and for the default one this
is not something we care much about, because we know its properties,
no? It would be nice to add a comment on that though, perhaps in
libpq-be.h where the key is defined.
--
Michael