-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Blake2b: Use OSSL_DIGEST_PARAM_SIZE as settable instead of XOFLEN #22491
Conversation
BLAKE2 is not really an extensible output function unlike SHAKE as the digest size must be set during the context initialization. Thus it makes no sense to use OSSL_DIGEST_PARAM_XOFLEN. We also need to adjust EVP_DigestFinal_ex() to query the OSSL_DIGEST_PARAM_SIZE as gettable ctx param for the size. Fixes openssl#22488
This is an alternative to #22489 |
In the end I like this more. Although it breaks the invariant that EVP_DigestFinal_ex() does not write more than EVP_MAX_MD_SIZE, it does so only on explicit param setting so IMO not a big deal. |
I like it. Someone else needs to indicate being comfortable with this change. |
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.
This option is clearly preferably to #22489 IMO. One minor comment below.
@t-j-h please reconfirm |
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.
Approved assuming CIs pass
Needs a second approval asap to get this in the beta |
I'm glad it was this easy after all |
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.
Great work
Approved again. |
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.
Looks good to me..
As an aside, I am not sure but I think EVP_MD_get_size() probably doesnt work.. since evp_md_cache_constants() probably happens earlier than when a call to the set_ctx_params
There's a conceptual difference between |
It might need to invalidate the size cache in Actually, it's nowhere near that bad because Blake2b already doesn't support any of this and this PR is doing the right thing by attempting to query the context in the final to get the correct size. All it means is that Definitely not stopping this going into beta IMO. |
EVP_MD_get_size() must return the default size. There is no way around that. However there is a problem with EVP_MD_CTX_get_size() - that function is actually a macro |
Pushed. |
BLAKE2 is not really an extensible output function unlike SHAKE as the digest size must be set during the context initialization. Thus it makes no sense to use OSSL_DIGEST_PARAM_XOFLEN. We also need to adjust EVP_DigestFinal_ex() to query the OSSL_DIGEST_PARAM_SIZE as gettable ctx param for the size. Fixes #22488 Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #22491)
BLAKE2 is not really an extensible output function unlike SHAKE as the digest size must be set during the context initialization. Thus it makes no sense to use OSSL_DIGEST_PARAM_XOFLEN. We also need to adjust EVP_DigestFinal_ex() to query the OSSL_DIGEST_PARAM_SIZE as gettable ctx param for the size. Fixes #22488 Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl/openssl#22491) Signed-off-by: fly2x <fly2x@hitls.org>
BLAKE2 is not really an extensible output function unlike SHAKE as the digest size must be set during the context initialization. Thus it makes no sense to use OSSL_DIGEST_PARAM_XOFLEN.
We also need to adjust EVP_DigestFinal_ex() to query the OSSL_DIGEST_PARAM_SIZE as gettable ctx param for the size.
Fixes #22488
Checklist