Skip to content
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

Fix "openssl dhparam -check" #14146

Closed
wants to merge 5 commits into from

Conversation

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Feb 10, 2021

Both DH_check_ex() and DH_check_params_ex() check the parameters. DH_check_ex() performs a more complete check, while DH_check_params_ex() performs a lightweight check. In 1.1.1 EVP_PKEY_param_check() would call DH_check_ex() for DH keys but in master it was calling DH_check_params_ex. For backwards compatibility we should instead call DH_check_ex(). By doing so we fix the issue with the "dhparam -check" command noted in #13501.

However, making this change reveals that libssl was calling EVP_PKEY_param_check() as a replacement for DH_check_params_ex() which is used in 1.1.1. But the checks performed by the newly modified EVP_PKEY_param_check() function are not appropriate for use in libssl. Specifically we tolerate non-safe primes in libssl which EVP_PKEY_param_check() does not. Therefore we introduce a new function EVP_PKEY_param_check_quick() that enables implementations to provide a quicker form of parameter validation that omits some checks if appropriate.

Finally making the EVP_PKEY_param_check change reveals that not all parameters that can be generated by genpkey are actually valid according to dhparam -check. The test in test_dhparam_check is modified to avoid failing on the invalid data and a separate issue has been raised (#14145) to fix genpkey.

Fixes #13501

mattcaswell added 4 commits Feb 9, 2021
Both DH_check_ex() and DH_check_params_ex() check the parameters.
DH_check_ex() performs a more complete check, while DH_check_params_ex()
performs a lightweight check. In 1.1.1 EVP_PKEY_param_check() would call
DH_check_ex() for DH keys. For backwards compatibility we should continue
with that behaviour.

Fixes #13501
The low level DH API has two functions for checking parameters:
DH_check_ex() and DH_check_params_ex(). The former does a "full" check,
while the latter does a "quick" check. Most importantly it skips the
check for a safe prime. We're ok without using safe primes here because
we're doing ephemeral DH.

Now that libssl is fully using the EVP API, we need a way to specify that
we want a quick check instead of a full check. Therefore we introduce
EVP_PKEY_param_check_quick() and use it.
genpkey can sometimes create files that fail "openssl dhparam -check". See
issue #14145. We had some instances of such invalid files in the
dhparam_check test. Now that "openssl dhparam -check" has been fixed to
work the same way as it did in 1.1.1 these tests were failing. We move the
invalid files inot the "invalid" directory. A future PR will have to fix
genpkey to not generate invalid files.

We also remove a "SKIP" block that was skipping tests in a no deprecated
build unnecessarily. Nothing being tested is deprecated.
Copy link
Contributor

@paulidale paulidale left a comment

A couple of wording suggestions but it LGTM.

doc/man3/EVP_PKEY_check.pod Outdated Show resolved Hide resolved
doc/man3/EVP_PKEY_check.pod Outdated Show resolved Hide resolved
@@ -21,6 +22,12 @@ EVP_PKEY_private_check, EVP_PKEY_pairwise_check
EVP_PKEY_param_check() validates the parameters component of the key
given by B<ctx>.

EVP_PKEY_param_check_quick() also validates the parameters component of the key
give by B<ctx>. However some algorithm implementations may offer a quicker form
of validation that omits some checks in order to perform a sanity check of the

This comment has been minimized.

@paulidale

paulidale Feb 10, 2021
Contributor

Perhaps: a basic sanity check? To me a sanity check is still quite a strong term.

Other words in place of basic could include lightweight, quick, diaphanous, fast, imponderous, ...

This comment has been minimized.

@slontis

slontis Feb 10, 2021
Contributor

What about partial as the name instead of quick?

This comment has been minimized.

@paulidale

paulidale Feb 10, 2021
Contributor

I do worry that partial implies incompleteness in some sense. Being security software, users will be paranoid and thus avoid it.

This comment has been minimized.

@slontis

slontis Feb 10, 2021
Contributor

It is an incomplete test.. See my last comment also.

This comment has been minimized.

@paulidale

paulidale Feb 10, 2021
Contributor

If NIST is using partial, we ought to too.

This comment has been minimized.

@slontis

slontis Feb 10, 2021
Contributor

Well we use 'check' and not 'validate' also (since it is a existing API) so I am not too fussed either way.

This comment has been minimized.

@t8m

t8m Feb 11, 2021
Member

IMO we should not imply that we are automatically performing the NIST validation tests with the _check() calls. We could, in some cases where it makes sense, but we definitely do not want to always do that.

This comment has been minimized.

@mattcaswell

mattcaswell Feb 11, 2021
Author Member

I went with "lightweight sanity check".

Re quick vs partial - I'm not sure we need to align with NIST here. I personally prefer quick over partial so I've left it as is for now.

@slontis
Copy link
Contributor

@slontis slontis commented Feb 10, 2021

This reminds me of something (very simple) that I didn't implement related to EC public key validation.
i.e SP800-56A specifies the following section '5.6.2.3.4 ECC Partial Public-Key Validation Routine'

This is where I am getting the 'partial' keyword from.
I can add it after this PR if you like.
Note: I dont care too much if the name 'partial' is used or not.

@slontis
Copy link
Contributor

@slontis slontis commented Feb 10, 2021

Not sure why there is a leak in here?

@@ -28,10 +28,12 @@ TESTDIR=test/recipes/20-test_dhparam_check_data/valid
rm -rf $TESTDIR
mkdir -p $TESTDIR
#TODO(3.0): These 3 currently create invalid output - see issue #14145

This comment has been minimized.

@t8m

t8m Feb 11, 2021
Member

Yes, this is a bug in genpkey. #13871

./util/opensslwrap.sh genpkey -genparam -algorithm DH -pkeyopt dh_rfc5114:1 -out $TESTDIR/dh5114_1.pem
./util/opensslwrap.sh genpkey -genparam -algorithm DH -pkeyopt dh_rfc5114:2 -out $TESTDIR/dh5114_2.pem
./util/opensslwrap.sh genpkey -genparam -algorithm DH -pkeyopt dh_rfc5114:3 -out $TESTDIR/dh5114_3.pem
#TODO(3.0): These 4 currently create invalid output - see issue #14145

This comment has been minimized.

@t8m

t8m Feb 11, 2021
Member

Yes, this is a bug in genpkey. #13956

@mattcaswell
Copy link
Member Author

@mattcaswell mattcaswell commented Feb 11, 2021

Some doc tweaks pushed.

@paulidale - can you reconfirm?

openssl-machine pushed a commit that referenced this pull request Feb 15, 2021
Both DH_check_ex() and DH_check_params_ex() check the parameters.
DH_check_ex() performs a more complete check, while DH_check_params_ex()
performs a lightweight check. In 1.1.1 EVP_PKEY_param_check() would call
DH_check_ex() for DH keys. For backwards compatibility we should continue
with that behaviour.

Fixes #13501

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14146)
openssl-machine pushed a commit that referenced this pull request Feb 15, 2021
The low level DH API has two functions for checking parameters:
DH_check_ex() and DH_check_params_ex(). The former does a "full" check,
while the latter does a "quick" check. Most importantly it skips the
check for a safe prime. We're ok without using safe primes here because
we're doing ephemeral DH.

Now that libssl is fully using the EVP API, we need a way to specify that
we want a quick check instead of a full check. Therefore we introduce
EVP_PKEY_param_check_quick() and use it.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14146)
openssl-machine pushed a commit that referenced this pull request Feb 15, 2021
genpkey can sometimes create files that fail "openssl dhparam -check". See
issue #14145. We had some instances of such invalid files in the
dhparam_check test. Now that "openssl dhparam -check" has been fixed to
work the same way as it did in 1.1.1 these tests were failing. We move the
invalid files inot the "invalid" directory. A future PR will have to fix
genpkey to not generate invalid files.

We also remove a "SKIP" block that was skipping tests in a no deprecated
build unnecessarily. Nothing being tested is deprecated.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14146)
openssl-machine pushed a commit that referenced this pull request Feb 15, 2021
Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #14146)
@slontis
Copy link
Contributor

@slontis slontis commented Feb 15, 2021

Merged to master.

@slontis slontis closed this Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants