Обсуждение: Replacing the EDH SKIP primes

The current hardcoded EDH parameter fallback use the old SKIP primes, for which
the source disappeared from the web a long time ago.  Referencing a known dead
source seems a bit silly, so I think we should either switch to a non-dead
source of MODP primes or use an archive.org link for SKIP.  Personally I prefer
the former.

This was touched upon, but never really discussed AFAICT, back when then EDH
parameters were reworked a few years ago.  Instead of replacing with custom
ones, as suggested in [1] it we might as well replace with standardized ones as
this is a fallback.  Custom ones wont make it more secure, just add more work
for the project.  The attached patch replace the SKIP prime with the 2048 bit
MODP group from RFC 3526, which is the same change that OpenSSL did a few years
back [2].

cheers ./daniel

[1] https://www.postgresql.org/message-id/54f44984-2f09-8744-927f-140a90c379dc%40ohmu.fi
[2] https://github.com/openssl/openssl/commit/fb015ca6f05e09b11a3932f89d25bae697c8af1e

On Tue, Jun 18, 2019 at 02:05:00PM +0200, Daniel Gustafsson wrote:
> The current hardcoded EDH parameter fallback use the old SKIP primes, for which
> the source disappeared from the web a long time ago.  Referencing a known dead
> source seems a bit silly, so I think we should either switch to a non-dead
> source of MODP primes or use an archive.org link for SKIP.  Personally I prefer
> the former.

I agree with you that it sounds more sensible to switch to a new prime
instead of relying on an archive of the past one.

> This was touched upon, but never really discussed AFAICT, back when then EDH
> parameters were reworked a few years ago.  Instead of replacing with custom
> ones, as suggested in [1] it we might as well replace with standardized ones as
> this is a fallback.  Custom ones wont make it more secure, just add more work
> for the project.  The attached patch replace the SKIP prime with the 2048 bit
> MODP group from RFC 3526, which is the same change that OpenSSL did a few years
> back [2].

Fine by me.  Let's stick with the 2048b-long one for now as we did in
c0a15e0.  I am wondering if we should sneak that into v12, but I'd
rather just wait for v13 to open.
--
Michael

> On 19 Jun 2019, at 05:40, Michael Paquier <michael@paquier.xyz> wrote:

> Fine by me.  Let's stick with the 2048b-long one for now as we did in
> c0a15e0.  I am wondering if we should sneak that into v12, but I'd
> rather just wait for v13 to open.

I think this is v13 material, I’ll stick it in the next commitfest.

cheers ./daniel

On Wed, Jun 19, 2019 at 07:44:46AM +0200, Daniel Gustafsson wrote:
> I think this is v13 material, I’ll stick it in the next commitfest.

Thanks!
--
Michael

On 2019-06-18 13:05, Daniel Gustafsson wrote:
> This was touched upon, but never really discussed AFAICT, back when then EDH
> parameters were reworked a few years ago.  Instead of replacing with custom
> ones, as suggested in [1] it we might as well replace with standardized ones as
> this is a fallback.  Custom ones wont make it more secure, just add more work
> for the project.  The attached patch replace the SKIP prime with the 2048 bit
> MODP group from RFC 3526, which is the same change that OpenSSL did a few years
> back [2].

It appears that we have consensus to go ahead with this.

<paranoia>
I was wondering whether the provided binary blob contained any checksums
or other internal checks.  How would we know whether it contains
transposed characters or replaces a 1 by a I or a l?  If I just randomly
edit the blob, the ssl tests still pass.  (The relevant load_dh_buffer()
call does get called by the tests.)  How can we make sure we actually
got a good copy?
</paranoia>

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jul 02, 2019 at 08:14:25AM +0100, Peter Eisentraut wrote:
> It appears that we have consensus to go ahead with this.

Yeah, I was planning to look at that one next.  Or perhaps you would
like to take care of it, Peter?

> <paranoia>
> I was wondering whether the provided binary blob contained any checksums
> or other internal checks.  How would we know whether it contains
> transposed characters or replaces a 1 by a I or a l?  If I just randomly
> edit the blob, the ssl tests still pass.  (The relevant load_dh_buffer()
> call does get called by the tests.)  How can we make sure we actually
> got a good copy?
> </paranoia>

PEM_read_bio_DHparams() has some checks on the Diffie-Hellman key, but
it is up to the caller to make sure that it is normally providing a
prime number in this case to make the cracking harder, no?  RFC 3526
has a small formula in this case, which we can use to double-check the
patch.
--
Michael

> On 2 Jul 2019, at 09:49, Michael Paquier <michael@paquier.xyz> wrote:
> On Tue, Jul 02, 2019 at 08:14:25AM +0100, Peter Eisentraut wrote:

>> <paranoia>
>> I was wondering whether the provided binary blob contained any checksums
>> or other internal checks.  How would we know whether it contains
>> transposed characters or replaces a 1 by a I or a l?  If I just randomly
>> edit the blob, the ssl tests still pass.  (The relevant load_dh_buffer()
>> call does get called by the tests.)  How can we make sure we actually
>> got a good copy?
>> </paranoia>
>
> PEM_read_bio_DHparams() has some checks on the Diffie-Hellman key, but
> it is up to the caller to make sure that it is normally providing a
> prime number in this case to make the cracking harder, no?

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.

>  RFC 3526
> has a small formula in this case, which we can use to double-check the
> patch.

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.

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).  From reading RFC2412 which outlines how the primes are generated,
this is by design.  In Appendix E:

  "Because these two primes are congruent to 7 (mod 8), 2 is a quadratic
   residue of each prime.  All powers of 2 will also be quadratic
   residues.  This prevents an opponent from learning the low order bit
   of the Diffie-Hellman exponent (AKA the subgroup confinement
   problem).  Using 2 as a generator is efficient for some modular
   exponentiation algorithms.  [Note that 2 is technically not a
   generator in the number theory sense, because it omits half of the
   possible residues mod P.  From a cryptographic viewpoint, this is a
   virtue.]"

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.)

cheers ./daniel

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

> On 3 Jul 2019, at 12:11, Michael Paquier <michael@paquier.xyz> wrote:

> It would be nice to add a comment on that though, perhaps in
> libpq-be.h where the key is defined.

Agreed, I’ve updated the patch with a comment on this formulated such that it
should stand the test of time even as OpenSSL changes etc.

cheers ./daniel

On Wed, Jul 03, 2019 at 08:56:42PM +0200, Daniel Gustafsson wrote:
> Agreed, I’ve updated the patch with a comment on this formulated such that it
> should stand the test of time even as OpenSSL changes etc.

I'd like to think that we had rather mention the warning issue
explicitely, so as people don't get surprised, like that for example:

 *  This is the 2048-bit DH parameter from RFC 3526.  The generation of the
 *  prime is specified in RFC 2412, which also discusses the design choice
 *  of the generator.  Note that when loaded with OpenSSL this causes
 *  DH_check() to fail on with DH_NOT_SUITABLE_GENERATOR, where leaking
 *  a bit is preferred.

Now this makes an OpenSSL-specific issue pop up within a section of
the code where we want to make things more generic with SSL, so your
simpler version has good arguments as well.

I have just rechecked the shape of the key, and we have an exact
match.
--
Michael

> On 04 Jul 2019, at 02:58, Michael Paquier <michael@paquier.xyz> wrote:
>
>> On Wed, Jul 03, 2019 at 08:56:42PM +0200, Daniel Gustafsson wrote:
>> Agreed, I’ve updated the patch with a comment on this formulated such that it
>> should stand the test of time even as OpenSSL changes etc.
>
> I'd like to think that we had rather mention the warning issue
> explicitely, so as people don't get surprised, like that for example:
>
> *  This is the 2048-bit DH parameter from RFC 3526.  The generation of the
> *  prime is specified in RFC 2412, which also discusses the design choice
> *  of the generator.  Note that when loaded with OpenSSL this causes
> *  DH_check() to fail on with DH_NOT_SUITABLE_GENERATOR, where leaking
> *  a bit is preferred.
>
> Now this makes an OpenSSL-specific issue pop up within a section of
> the code where we want to make things more generic with SSL, so your
> simpler version has good arguments as well.
>
> I have just rechecked the shape of the key, and we have an exact
> match.

LGTM, thanks.

cheers ./daniel

On Thu, Jul 04, 2019 at 08:24:13AM +0200, Daniel Gustafsson wrote:
> LGTM, thanks.

Okay, done, after rechecking the shape of the key.  Thanks!
--
Michael