Fix `EVP_PKEY_get_size()` doc and error handling #22459

DDvO · 2023-10-21T09:21:40Z

Attempting to use a provider that does not implement OSSL_PKEY_PARAM_MAX_SIZE
fails on CMS/PKCS7/SMIME signing with the -noattr option,
while not even an error queue entry is provided.

This PR fixes the documentation to point out

the interrelation of EVP_PKEY_get_size and OSSL_PKEY_PARAM_MAX_SIZE (an the same for two similar pairs of functions and parameters)
the fact that providers need to implement OSSL_PKEY_PARAM_MAX_SIZE for supporting some (e.g, CMS) use cases.

It also fixes the error handling and reporting on failure of EVP_PKEY_get_size() and friends.

…ry on failure

DDvO · 2023-10-21T09:25:38Z

BTW, looks like providers that implement the gettable OSSL_PKEY_PARAM_MAX_SIZE keymgmt parameter
also need to implement OSSL_FUNC_SIGNATURE_SET_CTX_PARAMS and OSSL_FUNC_SIGNATURE_SETTABLE_CTX_PARAMS, at least with an empty list of 'known' parameters.

Is this due to a bug of the specific provider or of the underlying OpenSSL provider support?

paulidale

Quite possibly provider support assuming the function is implemented...

DDvO · 2023-10-23T12:24:01Z

Thanks for the swift approvals.

Quite possibly provider support assuming the function is implemented...

@paulidale I assume your comment was meant in response to my question:

BTW, looks like providers that implement the gettable OSSL_PKEY_PARAM_MAX_SIZE keymgmt parameter also need to implement OSSL_FUNC_SIGNATURE_SET_CTX_PARAMS and OSSL_FUNC_SIGNATURE_SETTABLE_CTX_PARAMS, at least with an empty list of 'known' parameters.

Is this due to a bug of the specific provider or of the underlying OpenSSL provider support?

@ all, I'd find it weird that a provider implementing parameter getter functionality
also has to implement querying parameter setter functions.
According to https://www.openssl.org/docs/man3.0/man7/provider-signature.html the statement:
OSSL_FUNC_signature_set_ctx_params and OSSL_FUNC_signature_settable_ctx_params are optional, ...

Yet for instance

openssl cms -provider ... -sign -in ... -inkey ... -signer ... -out ... -noattr

fails if OSSL_FUNC_SIGNATURE_SET_CTX_PARAMS and OSSL_FUNC_SIGNATURE_SETTABLE_CTX_PARAMS are not implemented.

In such use cases, both the cms and pkcs7 libs attempt to set the digest parameter (to the value of the -md option if given, by default to "SHA2-256").
(Without the -noattr option, neither the setter query functions nor the getter for OSSL_PKEY_PARAM_MAX_SIZE are needed.)

openssl-machine · 2023-10-24T10:00:23Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

DDvO · 2023-10-26T14:41:10Z

Oh, meanwhile backporting includes 3.2
Yet why no more 3.0 - I thought it has LTS?

t8m · 2023-10-26T15:04:01Z

Aargh... that removal of the 3.0 label was uintended. I've botched something.

Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #22459)

…ry on failure Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #22459)

Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #22459)

Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #22459) (cherry picked from commit d7ad09d)

…ry on failure Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #22459) (cherry picked from commit ae643b3)

Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from #22459) (cherry picked from commit 0929814)

hlandau · 2023-10-26T15:06:05Z

Merged to 3.2 and master. Thank you.

This did not merge cleanly to 3.0 or 3.1, you may wish to open another PR.

levitte · 2023-10-26T15:09:58Z

crypto/evp/p_lib.c

+    if (size <= 0) {
+        ERR_raise(ERR_LIB_EVP, EVP_R_UNKNOWN_BITS);
+        return 0;
+    }


Errrrrrr.... documentation says:

RETURN VALUES
EVP_PKEY_get_size(), EVP_PKEY_get_bits() and EVP_PKEY_get_security_bits() return a positive number, or 0 if this size isn't available.

To me, that indicates that zero is not an error.

To me, the doc is kind of ambiguous whether it is an error if the size isn't available.

And reality is, callers of EVP_PKEY_get_size() are simply too sloppy.
Just have a look at the output of grep -A 2 -r EVP_PKEY_get_size providers apps crypto test|fgrep .c.
Also in crypto/ and apps/, the 0 return value is in most cases not checked at all!
Instead, typically OPENSSL_malloc() is called immediately with the value obtained,
this if it is 0, a misleading malloc failure is on the error stack, not indicating an issue with getting the size.

And in those few cases where a 0 return value is checked, no error is put on the queue when bailing out.
This was the motivation for coming up with this PR -
namely because callers in practice expect that any needed ERR_raise() is done by the function itself.

Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl/openssl#22459) Signed-off-by: fly2x <fly2x@hitls.org>

…ry on failure Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl/openssl#22459) Signed-off-by: fly2x <fly2x@hitls.org>

Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> (Merged from openssl/openssl#22459) Signed-off-by: fly2x <fly2x@hitls.org>

DDvO added 3 commits October 21, 2023 11:07

CMS and PKCS7: fix handlling of EVP_PKEY_get_size() failure

9762bcb

EVP_PKEY_get_{bits,security_bits,size}(): add missing error queue ent…

4ca586c

…ry on failure

EVP_PKEY_get_size.pod and provider-keymgmt.pod: document their relation

bcf8d0e

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Oct 21, 2023

paulidale approved these changes Oct 22, 2023

View reviewed changes

t8m approved these changes Oct 23, 2023

View reviewed changes

t8m added branch: 3.2 Merge to openssl-3.2 and removed branch: 3.0 Merge to openssl-3.0 branch labels Oct 26, 2023

DDvO added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 26, 2023

t8m added the branch: 3.0 Merge to openssl-3.0 branch label Oct 26, 2023

hlandau closed this Oct 26, 2023

levitte reviewed Oct 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `EVP_PKEY_get_size()` doc and error handling #22459

Fix `EVP_PKEY_get_size()` doc and error handling #22459

DDvO commented Oct 21, 2023 •

edited

DDvO commented Oct 21, 2023 •

edited

paulidale left a comment

DDvO commented Oct 23, 2023 •

edited

openssl-machine commented Oct 24, 2023

DDvO commented Oct 26, 2023

t8m commented Oct 26, 2023

hlandau commented Oct 26, 2023

levitte Oct 26, 2023

DDvO Oct 27, 2023

Fix EVP_PKEY_get_size() doc and error handling #22459

Fix EVP_PKEY_get_size() doc and error handling #22459

Conversation

DDvO commented Oct 21, 2023 • edited

DDvO commented Oct 21, 2023 • edited

paulidale left a comment

Choose a reason for hiding this comment

DDvO commented Oct 23, 2023 • edited

openssl-machine commented Oct 24, 2023

DDvO commented Oct 26, 2023

t8m commented Oct 26, 2023

hlandau commented Oct 26, 2023

levitte Oct 26, 2023

Choose a reason for hiding this comment

DDvO Oct 27, 2023

Choose a reason for hiding this comment

Fix `EVP_PKEY_get_size()` doc and error handling #22459

Fix `EVP_PKEY_get_size()` doc and error handling #22459

DDvO commented Oct 21, 2023 •

edited

DDvO commented Oct 21, 2023 •

edited

DDvO commented Oct 23, 2023 •

edited