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

EVP: Fix EVP_Digest{Sign,Verify}Init() to handle no default digest #11576

Closed
wants to merge 6 commits into from

Conversation

@levitte
Copy link
Member

@levitte levitte commented Apr 20, 2020

EVP_DigestSignInit() and EVP_DigestVerifyInit() would detect if there
is no default digest when using legacy (EVP_PKEY_ASN1_METHOD)
implementations. However, it doesn't do that when provider side keys
are used.

Furthermore, because EVP_PKEY_get_default_digest_name() was used in
the portion of the code that uses the provider implementation, the
EVP_PKEY_ASN1_METHOD would be used if the key has one attached. This
is now changed to use evp_keymgmt_util_get_deflt_digest_name()
instead.

Finally, we make sure to detect if the provider implementation
supports the digest name parameters (default or mandatory), and
returns with error if not. This is what the legacy portion of the

Fixes #11571

@levitte levitte added this to the 3.0.0 milestone Apr 20, 2020
@levitte levitte added this to In progress in 3.0 New Core + FIPS via automation Apr 20, 2020
@levitte
Copy link
Member Author

@levitte levitte commented Apr 20, 2020

This turned out to be a logical exercise as well. Basically, the previous logic in do_sigver_init() seemed to be that if the provider implementation didn't specify any default or mandatory digest (i.e. that parameter is unimplemented), then no digest must be used. However, that doesn't seem to be the logic for legacy implementations, where the interpretation rather seems to be that if the default md nid ctrl (ASN1_PKEY_CTRL_DEFAULT_MD_NID) is unsupported, then the caller must specify a digest, i.e. type is not allowed to be NULL.

So essentially, we're turning the provider implementation logic around to be legacy compatible, i.e. the provider implementation must provide a default or mandatory digest name if there is such a thing, and the empty string gets the special meaning that no digest must be used with this algorithm.

Copy link
Contributor

@paulidale paulidale left a comment

I not at all sure about the return_size kludge.

params[0] =
OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_DEFAULT_DIGEST,
mddefault, sizeof(mddefault));
params[0].return_size = sizeof(mddefault) + 1;

This comment has been minimized.

@paulidale

paulidale Apr 20, 2020
Contributor

Setting the return size here will have no effect. The return size is set by the recipient.

This isn't a great solution to the did the other end set this question that we've thus far not been able to answered.

This comment has been minimized.

@levitte

levitte Apr 20, 2020
Author Member

Setting the return size here will have no effect. The return size is set by the recipient.

... if the recipient supports this parameter, and in that case, it will be set to something other than this initial value.

This comment has been minimized.

@paulidale

paulidale Apr 20, 2020
Contributor

I noted this, I don't like this way of detecting a parameter being set.

It's error prone -- the return size is set but the buffer not if the return size is larger than the data length. So a legitimate return of size sizeof(mddefault) + 1 could be a problem -- it isn't here because of another check but I think it is a bad precedent to set.

I'd consider using (size_t)-1 would be better. Best would be for the helper functions to support what is required. The construct calls clearing whatever sentinel is used, the set calls flag it and an additional function to check.

This comment has been minimized.

@levitte

levitte Apr 20, 2020
Author Member

So a legitimate return of size sizeof(mddefault) + 1 could be a problem

If p.data_size is sizeof(mddefault), I don't see how that would be a legitimate return size... that would mean that the receiving end has overwritten the buffer.

I'd consider using (size_t)-1 would be better.

I've considered that, but couldn't shake the feeling that there may be platforms where that's a perfectly legitimate size that doesn't necessarily eat all available memory (i.e. where size_t is smaller in size than void *). It's unlikely that we'll hit that kind of size, at least for now, but...

Best would be for the helper functions to support what is required. The construct calls clearing whatever sentinel is used, the set calls flag it and an additional function to check.

I have nothing against that idea.

This comment has been minimized.

@paulidale

paulidale Apr 20, 2020
Contributor

If p.data_size is sizeof(mddefault), I don't see how that would be a legitimate return size...

If the return size is larger than the buffer size, the return size is set but no data is returned. The function also returns 0, so it's not an issue here.

I'll try make a stab at an API tomorrow. No promises.

This comment has been minimized.

@levitte

levitte Apr 20, 2020
Author Member

Don't make this a blocker for the upcoming alpha.

This comment has been minimized.

@kaduk

kaduk Apr 20, 2020
Contributor

Interestingly, a compliant C99 implementation is allowed to set SIZE_MAX as small as 65535.

This comment has been minimized.

@paulidale

paulidale Apr 20, 2020
Contributor

I've used processors where even that would be way too large :)

This comment has been minimized.

@paulidale

paulidale Apr 21, 2020
Contributor

Kind of moot given the delay but refer #11588 for an alternative

This comment has been minimized.

@levitte

levitte Apr 21, 2020
Author Member

I've come to terms with the (size_t)-1 value. It will probably be eons before we see a legitimate size get even close...

OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_MANDATORY_DIGEST,
mdmandatory,
sizeof(mdmandatory));
params[1].return_size = sizeof(mdmandatory) + 1;

This comment has been minimized.

@paulidale

paulidale Apr 20, 2020
Contributor

Likewise.

doc/man3/EVP_PKEY_get_default_digest_nid.pod Outdated Show resolved Hide resolved
3.0 New Core + FIPS automation moved this from In progress to Needs review Apr 20, 2020
Copy link
Member

@mattcaswell mattcaswell left a comment

Do we need a test for this?

@levitte
Copy link
Member Author

@levitte levitte commented Apr 20, 2020

Do we need a test for this?

That should be a generalized test. Not in this PR

@t8m
Copy link
Member

@t8m t8m commented Apr 20, 2020

The Travis breakage is relevant.

@kaduk
Copy link
Contributor

@kaduk kaduk commented Apr 20, 2020

There seems to be some missing text in the commit message for "EVP: Fix EVP_Digest{Sign,Verify}Init() to handle no default digest" (and the pull request description), that cuts off after:

Finally, we make sure to detect if the provider implementation
supports the digest name parameters (default or mandatory), and
returns with error if not.  This is what the legacy portion of the
@levitte
Copy link
Member Author

@levitte levitte commented Apr 20, 2020

The Travis breakage is relevant.

It turns out that the provider implementation of EC keys came with no default digest...

@levitte levitte force-pushed the levitte:fix-11571 branch Apr 20, 2020
@levitte
Copy link
Member Author

@levitte levitte commented Apr 20, 2020

There seems to be some missing text in the commit message for "EVP: Fix EVP_Digest{Sign,Verify}Init() to handle no default digest" (and the pull request description), that cuts off after:

Reworded

@levitte
Copy link
Member Author

@levitte levitte commented Apr 21, 2020

The remaining Travis failures are not relevant here (one is fixed in #11573)

levitte added 6 commits Apr 20, 2020
evp_keymgmt_util_get_deflt_digest_name() is a refactor of the provider
side key part of EVP_PKEY_get_default_digest_name(), that takes
EVP_KEYMGMT and provider keydata pointers instead of an EVP_PKEY
pointer.

We also ensure that it uses SN_undef as the default name if the
provider implementation gave us an empty string, since this is what
EVP_PKEY_get_default_digest_name() responds when getting the digest
name via a EVP_PKEY_ASN1_METHOD ctrl call that returns NID_undef.
EVP_DigestSignInit() and EVP_DigestVerifyInit() would detect if there
is no default digest when using legacy (EVP_PKEY_ASN1_METHOD)
implementations.  However, it doesn't do that when provider side keys
are used.

Furthermore, because EVP_PKEY_get_default_digest_name() was used in
the portion of the code that uses the provider implementation, the
EVP_PKEY_ASN1_METHOD would be used if the key has one attached.  This
is now changed to use evp_keymgmt_util_get_deflt_digest_name()
instead.

Finally, we make sure to detect if the provider implementation
supports the digest name parameters (default or mandatory), and
returns with error if not.  This is what the legacy portion of the
code does.

Fixes #11571
This adds handling of the parameter "mandatory-digest" and responds
with an empty string, meaning that no digest may be used.
@levitte levitte force-pushed the levitte:fix-11571 branch to 7e5deee Apr 21, 2020
@levitte
Copy link
Member Author

@levitte levitte commented Apr 21, 2020

Rebased. The #11592 fix should result in a clean Travis run

Copy link
Contributor

@paulidale paulidale left a comment

If this is slated for the alpha release, the param modification detection can be switched over after it.

@levitte
Copy link
Member Author

@levitte levitte commented Apr 22, 2020

@paulidale, if you could remember the "approval: done" label, that would be quit helpful.

@paulidale
Copy link
Contributor

@paulidale paulidale commented Apr 22, 2020

Oops, sorry.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Apr 23, 2020

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.

openssl-machine pushed a commit that referenced this pull request Apr 23, 2020
evp_keymgmt_util_get_deflt_digest_name() is a refactor of the provider
side key part of EVP_PKEY_get_default_digest_name(), that takes
EVP_KEYMGMT and provider keydata pointers instead of an EVP_PKEY
pointer.

We also ensure that it uses SN_undef as the default name if the
provider implementation gave us an empty string, since this is what
EVP_PKEY_get_default_digest_name() responds when getting the digest
name via a EVP_PKEY_ASN1_METHOD ctrl call that returns NID_undef.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #11576)
openssl-machine pushed a commit that referenced this pull request Apr 23, 2020
EVP_DigestSignInit() and EVP_DigestVerifyInit() would detect if there
is no default digest when using legacy (EVP_PKEY_ASN1_METHOD)
implementations.  However, it doesn't do that when provider side keys
are used.

Furthermore, because EVP_PKEY_get_default_digest_name() was used in
the portion of the code that uses the provider implementation, the
EVP_PKEY_ASN1_METHOD would be used if the key has one attached.  This
is now changed to use evp_keymgmt_util_get_deflt_digest_name()
instead.

Finally, we make sure to detect if the provider implementation
supports the digest name parameters (default or mandatory), and
returns with error if not.  This is what the legacy portion of the
code does.

Fixes #11571

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #11576)
openssl-machine pushed a commit that referenced this pull request Apr 23, 2020
This adds handling of the parameter "mandatory-digest" and responds
with an empty string, meaning that no digest may be used.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #11576)
openssl-machine pushed a commit that referenced this pull request Apr 23, 2020
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #11576)
@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Apr 23, 2020

Pushed!

3.0 New Core + FIPS automation moved this from Needs review to Done Apr 23, 2020
@levitte levitte deleted the levitte:fix-11571 branch Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

6 participants