-
-
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
Intentionally break EVP_DigestFinal for SHAKE128 and SHAKE256 #24105
Conversation
This is my alternative proposal to #23877 |
Apart from my review comment, those looks good to me |
I wonder whether we have a (potential) regression with PKCS#12 |
How so? I do not see SHAKE being used in PKCS#12. It would make no sense. |
It's not clear why we're adding SHAKE-128/128 and SHAKE-256/256. I would prefer that we just don't support a shake with a fixed length, something should always set the length. |
This is up to debate. IMO we definitely need to add SHAKE-128/256 and SHAKE-256/512. As for these shortened ones, the reason is to provide a simple replacement for those existing use-cases which depend on the existing shortened default lengths. It is however questionable, whether these use-cases are important enough and whether either using DigestFinalXOF or explicitly set the XOFLEN parameter wouldn't be a more appropriate workaround. |
and why those use-cases can't use sha-3-256 and sha-3-512 respectively? also, do we really want to define a hash function in 2023 that has a 64 bit level of security of collision resistance: |
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.
Before multiple definitions for SHAKE128 and SHAKE256 are added we need to figure out the OID problem.
It needs to work nicely with signatures (See https://github.com/openssl/openssl/pull/23114/files)
This also ties in with https://github.com/openssl/openssl/pull/22684/files. HSS and XMSS uses So it looks like ideally SHAKE128 and SHAKE256 SHOULD use XOFFinal only and NOT HAVE THE OID ASSOCIATED WITH THEM.. |
The question is why would this artificial limit be applied? If this was a different algorithm and produced different outputs then I would definitely apply it but it does not make much sense to me. And especially not if we move the OID! |
Because I was thinking of using them like any other digest algorithm that uses DigestFinal. Why would you have multiple different names and then make them equivalent by being able to set the xof length manually. |
You could already use the existing SHAKE algorithms with DigestFinal. I do not see a reason why not to allow multiple ways how to use the newly named ones the same way. |
ossl_keccak_init(ctx, pad, bitlen, mdlen); \ | ||
if (mdlen == SIZE_MAX) \ | ||
ctx->md_size = SIZE_MAX; \ |
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.
I'm not entirely convinced that SIZE_MAX
is the better indicator for indefinite length. How about zero?
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.
SIZE_MAX
is better IMO. It requires less special case code in callers and generating more than SIZE_MAX
output is likely to be highly suspect and not storable or processable in reasonable time.
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.
I disagree. Zero is a much better indicator that you haven't initialised something to a fixed length.
This is C code after all.
Haven't we used the pattern of using 0 elsewhere - I don't know that we have used a SIZE_MAX approach to indicate indefinite elsewhere ...
Consistency here should be the guide.
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.
I doubt there will be much of a difference for the caller. Basically, this would work as an indicator whether it's appropriate to Squeeze without limit rather than using DigestFinalXOF, at least as long as we don't really have any other way for the caller to check what sort of functionality is supported by the implementation.
So in the end, it's this check:
// Can I squeeze?
if (md_size == SIZE_MAX)
...
or this:
// Can I squeeze?
if (md_size == 0)
...
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.
We do use SIZE_MAX
quite a bit, but that's in cases where size maximums are a thing. SHAKE isn't expressed in terms of maximum size, it's expressed in "indefinite" terms (even though there are admonitions about practicality).
Furthermore, md_size
is usually the (fixed) size, not a maximum, and SIZE_MAX
is a bit weird of an exception from that interpretation compare to zero, in my mind.
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.
I require X
bytes of output.
Thus either:
if (X < max_size || max_size == 0)
versus
if (X < max_size)
The latter is, by far, the simpler pattern IMO.
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.
I'm definitely not advocating for the essentially equivalent:
if (X < max_size - 1)
which is very counter intuitive.
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.
The problem is that 0
value is already very special and we already have a test (evp_test) that depends on EVP_DigestFinalXOF(ctx, ptr, 0)
returning success! So no, we have to keep SIZE_MAX as the unset value.
Checking for < max_size seems strange to me. I would expect max_size to be valid. 0 does not look like a valid value.
|
I believe this is now ready for review. |
sizeof(shake256_input)))) | ||
return 0; | ||
ERR_set_mark(); | ||
if (!TEST_false(EVP_DigestFinal(ctx, out, &digest_length))) { |
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.
Do we have a set the xoflen and then use digestfinal testcase?
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.
Yes. The evp_test exercises this code path. That was actually how I found the necessity for the below fix in EVP_DigestFinal_ex().
@@ -454,6 +454,8 @@ int EVP_DigestFinal_ex(EVP_MD_CTX *ctx, unsigned char *md, unsigned int *isize) | |||
if (ctx->digest->prov == NULL) | |||
goto legacy; | |||
|
|||
if (sz == 0) /* Assuming a xoflen must have been set. */ |
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.
Is it possible that something could have returned 0 for other algorithms?
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.
Then it would be an useless hash if it wasn't a XOF one in the same way as SHAKE is.
Alternative is to use SIZE_MAX as the invalid default size indicator. It has its downsides as well.
CHANGES.md needs a rebase.. |
It will work only if OSSL_DIGEST_PARAM_XOFLEN is set.
The force push was just a rebase with trivial CHANGES.md conflict resolution. |
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. |
Merged to the master branch. Thank you for the reviews. |
It will work only if OSSL_DIGEST_PARAM_XOFLEN is set. Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> (Merged from #24105)
Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> (Merged from #24105)
These were added as a POC in openssl#24387. However, such combinations are no longer unusable since openssl#24105 got merged. This should unbreak all build failures on mainline. Partially reverts: 1bfc8d1 (rsa-oaep: block SHAKE usage in FIPS mode, 2024-05-13) Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@surgut.co.uk>
These were added as a POC in #24387. However, such combinations are no longer unusable since #24105 got merged. This should unbreak all build failures on mainline. Partially reverts: 1bfc8d1 (rsa-oaep: block SHAKE usage in FIPS mode, 2024-05-13) Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@surgut.co.uk> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #24463)
It will work only if OSSL_DIGEST_PARAM_XOFLEN is set. Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> (Merged from openssl#24105)
Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Shane Lontis <shane.lontis@oracle.com> (Merged from openssl#24105)
These were added as a POC in openssl#24387. However, such combinations are no longer unusable since openssl#24105 got merged. This should unbreak all build failures on mainline. Partially reverts: 1bfc8d1 (rsa-oaep: block SHAKE usage in FIPS mode, 2024-05-13) Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@surgut.co.uk> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#24463)
It will work only if OSSL_DIGEST_PARAM_XOFLEN is set.
Also add new SHAKE-128/128, SHAKE-256/256, SHAKE-128/256 and SHAKE-256/512 algorithms which have explicit default XOFLEN set.
Checklist