New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change DH parameters to generate the order q subgroup instead of 2q #9363
Change DH parameters to generate the order q subgroup instead of 2q #9363
Conversation
Naturally I like the direction of this, but I would suggest to also add 4 as possible generator and probably even make that the default: Since 4 is a squre over the natural numbers, it will also always be one in Z_p, allowing to both skip the test and take all safeprimes, making this into a performance-improvement as well. (I don't know enough about efficient squaring with 2 vs. 3 vs. 4 vs. 5 to make a statement which one is the fastest, but I at least would not be surprised if 4 would outperform 3 and 5 if there is a difference at all.) |
Good news is: 3 is always a square over safe primes. And DH_check will now only check that |
Correction: I meant 3 is always a square over safe primes > 7 |
Do you have a proof for that? I'm not saying that I don't believe you, after all I just wrote a script that failed to find a counter-example, but a proof is of course still the best way to approach this topic. |
Yes. It is in the comment that I have unscrambled: |
I wonder if this qualifies as a bug fix, and should get backported to 1.1.1 |
Okay, I'm convinced of three. I don't claim to have an intuitive understanding of why the proof works, but doing the math (and believing a handful of theorems) it does indeed come up with that result. So while this means that three is a good default, I still think that the very simple proof for four has enough merit to justify the inclusion on the basis that people might be more willing to trust it. Normally that shouldn't be too much of an issue, but if the option is offered in the end-user-interface in the first place, then this part shouldn't hurt. Regarding whether this is a bug: I argued that it is one from the start because it leaks secrets (one bit in DH doesn't hurt too much, but if someone were to use the groups with ElGamal the effect could be devastating) and because it wastes a substantial amount of performance. |
I don't see any reason why DH parameters should change the default from 2 to any other number. I'll explain why I think that 2, 3, 5 is a more sensible choice than say 2, 4, 5. Let's consider for instance P=14440944214298199839 I can instantly solve 2^x = 4 (mod P) result: x = 2 I can even solve 4^x = 2 (mod P) result: x = 3610236053574549960
but I can't solve: 2^x = 3 (mod P) 3^x = 2 (mod P) 5^x = 2 (mod P) can you solve them? |
Well, actually, I am able to solve this equation: using python you can verify:
The following is a sage program that took > 1 hour to solve the discrete log.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Largely cosmetic changes suggested.
* for 3, p mod 12 == 5 | ||
* for 5, p mod 10 == 3 or 7 | ||
* should hold. | ||
* g is a suitable generator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of any element in a group of size 2q
with q
a prime, is 1, 2, q
or 2q
. Only 1
and p-1
have the first two orders, all other values of g
have order q
if g
is a quadratic residue mod p
and 2q
otherwise. We should generally prefer quadratic residues and order q
as this avoids having Z_2
as a subgroup, and "leaking bit 0" (do we really care about that???).
The choice of 2
as a generator is a sound default, because it makes key agreement computations more efficient. All the FFDHE groups for TLS 1.3 have g = 2
with g
a quadratic residue of order q
: https://tools.ietf.org/html/rfc7919#appendix-A
What this means for OpenSSL is that we should generally prefer p = 7 mod 8
to make 2
a quadratic residue, and also p = 2 mod 3
so that q = (p-1)/2
stands a chance of being prime, which combines to p = 23 mod 24
and q = 11 mod 12
.
If we wanted 2
to be a non-residue so that g
generates the whole order 2q
multiplicative group, we'd need p = 3 mod 8
, and p = 2 mod 3
as before, and so p = 11 mod 24
. So OpenSSL previously prefers o(g) = 2q
, but that's less preferred than o(g) = q
. So our default choice of prime should switch from 11 mod 24
to 23 mod 24
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right.
*ret |= DH_NOT_SUITABLE_GENERATOR; | ||
} else | ||
*ret |= DH_UNABLE_TO_CHECK_GENERATOR; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we perhaps check that g != p - 1
? Right now we're only checking for g < p
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, not 0, 1, p-1, not negative, not larger than p-1, etc...
That is why I added a call to DH_check_params above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I miss the check for p-1
somewhere? I did not see it where I expected to see it, perhaps did not look in the right place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you're using DH_check_params()
correctly? It does not return true/false for valid parameters, rather it returns a mask of misfeatures via its second argument...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea to accumulate more "misfeature" bits before returning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I hope so.
The idea is to accumulate all bits from DH_check_params, and or in all bits from DH_check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly, in case DH_check_params adds more checks in the future.
* for 5, p mod 10 == 3 or 7 | ||
* Using the quadratic reciprocity law it is possible to solve | ||
* (g/p) == 1 for the special values 2, 3, 5: | ||
* (2/p) == 1 if p mod 8 == 1 or 7. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But 1 mod 8
makes q
divisible by 4, so only 7 mod 8 is viable, and then as above also 2 mod 3, or in all
23 mod 24`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this holds for any p = odd prime.
I use the properties of safe primes in the next paragraph.
* Using the quadratic reciprocity law it is possible to solve | ||
* (g/p) == 1 for the special values 2, 3, 5: | ||
* (2/p) == 1 if p mod 8 == 1 or 7. | ||
* (3/p) == 1 if p mod 12 == 1 or 11. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, 1 mod 12
is excluded, so 11 mod 12
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I wanted that result to be in sync with the literature about the Legendre symbol,
where p is odd prime, not safe prime.
* (g/p) == 1 for the special values 2, 3, 5: | ||
* (2/p) == 1 if p mod 8 == 1 or 7. | ||
* (3/p) == 1 if p mod 12 == 1 or 11. | ||
* (5/p) == 1 if p mod 5 == 1 or 4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, only 4 mod 5
, but also 3 mod 4
and 2 mod 3
, so 59 mod 60
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that follows from p being a safe prime.
Again the reference below gives this result for p is odd prime.
/* clear bit 0, since it won't be a secret anyway */ | ||
if (!BN_clear_bit(priv_key, 0)) | ||
goto err; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably harmless, but is this useful or necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly harmless.
One might argue, it saves one additional modular multiplication by g.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the mod 5 case, what is the cost of computing p mod 5
every time we generate a key? Is it worth it? Since 256 mod 5
is one, the residue is just the sum of all the octets of p
mod 5, which can perhaps be computed faster than a generic mod 5 reduction. But do we really case about g = 5
? Does anybody actually use that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I'd bet 5, is never used. and it is probably more expensive than what it could save theoretically.
BIGNUM *t1 = NULL, *t2 = NULL; | ||
|
||
*ret = 0; | ||
if (!DH_check_params(dh, ret)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please look here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*ret = 0; is removed, and DH_check_params used to initialize *ret.
So DH_check_params returns 1, but *ret is already DH_NOT_SUITABLE_GENERATOR.
Please rebase so that CI checks can run. |
ecc5324
to
28b0c80
Compare
done. |
This avoids leaking bit 0 of the private key. Backport-of: openssl#9363
This can now be merged, after rebasing again (conflict in the CHANGES file?) |
Yes, okay, I wanted to wait 24H but that is quite difficult with that CHANGES file. |
This avoids leaking bit 0 of the private key.
28b0c80
to
9087476
Compare
Merged as a38c878. Thanks! |
This avoids leaking bit 0 of the private key. Reviewed-by: Viktor Dukhovni <viktor@openssl.org> Reviewed-by: Kurt Roeckx <kurt@roeckx.be> (Merged from #9363)
Just an academical question, to satisfy my curiosity: Does it really make a practical difference whether I choose my secret from an order 2q subgroup and leak a single bit, or choose it from an order q subgroup? Wouldn't my secret have log2(q) bits in both cases? |
No, don't worry, your parameters are still safe, if you use them for DH and not something else. |
if (l == (BN_ULONG)-1) | ||
goto err; | ||
if (l != 11) | ||
*ret |= DH_NOT_SUITABLE_GENERATOR; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tests in Node.js that depended on this block of removed code. Perhaps wrongly. See nodejs/node#29550 (comment) for a discussion of what we could do work around this change as we update to 1.1.1d. Is it the position of OpenSSL that mods of 11 and 7 are in fact suitable generators, and that no one using DH with this param should have/would have been paying attention to this rc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need a safe prime of at least 1024 bits, or better 2048 that will be safe for 30+ years.
g can generate the order-q or 2q subgroup but must of course not be 0, 1, p-1.
So 11 and 7 are safe primes, but they will be rejected in not too far future.
The order-q subgroup is preferable but even if we stop generating 2q subgroups
we will not have a check for g being a quadratic residue for backward compatibility.
For good tests vectors I would suggest use p=2048-bit safe prime,
and g=quadratic non-residue (as generated previously)
For bad test vectors use g=p-1, or p=not prime, or not safe prime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the background, @bernd-edlinger
OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment)
OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: #29550 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
@mspncp: It depends: For pure DHKX it doesn't really matter (though even if you argue that, the code was buggy before since it essentially had halve the performance it could trivially have had), for ElGamal the security of the entire scheme hinges on it. See my original article and bug-report for more information on the topic. |
OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: #29550 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: nodejs#29550 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: #29550 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Ported from OpenSUSE:nodejs8-8.17.0-lp152.147.1:fix_build_with_openssl_1.1.1d.patch Original commit message: FROM: https://github.com/nodejs/node/pull/29550/commits From 94c599e Mon Sep 17 00:00:00 2001 From: Sam Roberts <vieuxtech@gmail.com> Date: Thu, 19 Sep 2019 13:06:46 -0700 Subject: [PATCH] fixup! test: well-defined DH groups now verify clean test/parallel/test-crypto-binary-default.js | 3 +-- test/parallel/test-crypto-dh.js | 17 ++--------------- 2 files changed, 3 insertions(+), 17 deletions(-) From 7dc56e082b96aeee34e83dabbad81ee12607e38f Mon Sep 17 00:00:00 2001 From: Sam Roberts <vieuxtech@gmail.com> Date: Fri, 13 Sep 2019 13:19:06 -0700 Subject: [PATCH] test: well-defined DH groups now verify clean OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) Signed-off-by: Su Baocheng <baocheng.su@siemens.com>
Ported from OpenSUSE:nodejs8-8.17.0-lp152.147.1:fix_build_with_openssl_1.1.1d.patch Original commit message: FROM: https://github.com/nodejs/node/pull/29550/commits From 94c599e Mon Sep 17 00:00:00 2001 From: Sam Roberts <vieuxtech@gmail.com> Date: Thu, 19 Sep 2019 13:06:46 -0700 Subject: [PATCH] fixup! test: well-defined DH groups now verify clean test/parallel/test-crypto-binary-default.js | 3 +-- test/parallel/test-crypto-dh.js | 17 ++--------------- 2 files changed, 3 insertions(+), 17 deletions(-) From 7dc56e082b96aeee34e83dabbad81ee12607e38f Mon Sep 17 00:00:00 2001 From: Sam Roberts <vieuxtech@gmail.com> Date: Fri, 13 Sep 2019 13:19:06 -0700 Subject: [PATCH] test: well-defined DH groups now verify clean OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) Signed-off-by: Su Baocheng <baocheng.su@siemens.com>
This avoids leaking bit 0 of the private key.
Checklist