Backport DH_check from OpenSSL 1.1.0. #3375
Conversation
|
@markrwilliams, thanks for your PR! By analyzing the history of the files in this pull request, we identified @palaviv, @reaperhulk and @public to be potential reviewers. |
markrwilliams
force-pushed
the
DH_check_backport
branch
from
5252a88 to
2334583
Compare
OpenSSL 1.0.2's DH_check considers the q parameter, allowing it validate more generators and primes; however, OpenSSL 1.1.0's DH_check includes code to handle errors in BN functions, so it's preferred.
markrwilliams
force-pushed
the
DH_check_backport
branch
from
2334583 to
a1fe1a0
Compare
| * should hold. | ||
| */ | ||
|
|
||
| int Cryptography_DH_check(const DH *dh, int *ret) |
There was a problem hiding this comment.
For review purposes, is this from 1.1.0d? Could you add a comment stating what version it came from?
There was a problem hiding this comment.
GitHub says 1.1.0pre6 - not sure which letter release that is. How can I find out?
There was a problem hiding this comment.
1.1.0pre6 is itself a release number (it was the last pre-release prior to 1.1.0). So just add a comment saying it was updated to its current form in 1.1.0pre6
| return (ok); | ||
| } | ||
| #else | ||
| int (*Cryptography_DH_check)(const DH *dh, int *ret) = DH_check; |
There was a problem hiding this comment.
Our normal path for this sort of declaration is to do:
int Cryptography_DH_check(const DH *dh, int *ret) {
return DH_check(dh, ret);
}
While this method will obviously work fine, could you make the change for consistency?
There was a problem hiding this comment.
This is a lot nicer because I can put the cffi declaration in FUNCTIONS!
|
jenkins, add to whitelist |
|
Tests are failing because the new error values are not defined in older versions of OpenSSL. (e.g. constants like |
| @@ -117,6 +117,10 @@ | |||
| #endif | |||
|
|
|||
| #if CRYPTOGRAPHY_OPENSSL_LESS_THAN_110 || defined(LIBRESSL_VERSION_NUMBER) | |||
| # define DH_CHECK_Q_NOT_PRIME 0x10 | |||
There was a problem hiding this comment.
These should each be ifndef for safety since we don't want to fail with duplicate definitions if/when LibreSSL adds these.
This will prevent duplicate definitions when LibreSSL supports a version of DH_check that can return these.
|
@reaperhulk anything else I can do to help this land? And thanks for all the feedback!! |
|
I just need to get some time to review it. Will probably have to wait until Sunday, sorry! |
|
Confirmed this matches the implementation in OpenSSL. |
OpenSSL 1.0.2's DH_check considers the q parameter, allowing it
validate more generators and primes; however, OpenSSL 1.1.0's DH_check
includes code to handle errors in BN functions, so it's preferred.
See openssl/openssl@748e853.