Intentionally break EVP_DigestFinal for SHAKE128 and SHAKE256 #24105

t8m · 2024-04-11T08:30:39Z

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

documentation is added or updated
tests are added or updated

t8m · 2024-04-11T08:31:50Z

This is my alternative proposal to #23877

providers/implementations/include/prov/names.h

levitte · 2024-04-11T09:34:50Z

Apart from my review comment, those looks good to me

beldmit · 2024-04-11T19:56:22Z

I wonder whether we have a (potential) regression with PKCS#12

t8m · 2024-04-12T06:17:38Z

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.

kroeckx · 2024-04-12T10:08:32Z

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.

t8m · 2024-04-12T12:26:24Z

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.

tomato42 · 2024-04-12T13:12:38Z

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: "SHAKE-128/128:SHAKE128/128" ?

slontis

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)

slontis · 2024-04-14T22:30:34Z

This also ties in with https://github.com/openssl/openssl/pull/22684/files.

HSS and XMSS uses
SHAKE-128-256, SHAKE-256-512,
as well as the truncated versions 256-256, 256-192

So it looks like ideally SHAKE128 and SHAKE256 SHOULD use XOFFinal only and NOT HAVE THE OID ASSOCIATED WITH THEM..
And where I was heading with this is that the others names you have added are 'fixed' and the xoflen cant be changed.

t8m · 2024-04-15T08:55:15Z

And where I was heading with this is that the others names you have added are 'fixed' and the xoflen cant be changed.

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!

slontis · 2024-04-15T23:01:06Z

And where I was heading with this is that the others names you have added are 'fixed' and the xoflen cant be changed.

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.

t8m · 2024-04-16T06:54:32Z

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.

levitte · 2024-04-17T07:58:11Z

providers/implementations/digests/sha3_prov.c

+    ossl_keccak_init(ctx, pad, bitlen, mdlen);                                 \
+    if (mdlen == SIZE_MAX)                                                     \
+        ctx->md_size = SIZE_MAX;                                               \


I'm not entirely convinced that SIZE_MAX is the better indicator for indefinite length. How about zero?

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.

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.

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) ...

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.

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.

I'm definitely not advocating for the essentially equivalent:
if (X < max_size - 1)
which is very counter intuitive.

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.

kroeckx · 2024-04-17T09:56:37Z

Checking for < max_size seems strange to me. I would expect max_size to be valid. 0 does not look like a valid value.

slontis · 2024-04-25T22:55:33Z

test/evp_xof_test.c

+                   sizeof(shake256_input))))
+        return 0;
+    ERR_set_mark();
+    if (!TEST_false(EVP_DigestFinal(ctx, out, &digest_length))) {


Do we have a set the xoflen and then use digestfinal testcase?

Yes. The evp_test exercises this code path. That was actually how I found the necessity for the below fix in EVP_DigestFinal_ex().

slontis · 2024-04-25T22:59:54Z

crypto/evp/digest.c

    if (ctx->digest->prov == NULL)
        goto legacy;

+    if (sz == 0) /* Assuming a xoflen must have been set. */


Is it possible that something could have returned 0 for other algorithms?

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.

slontis · 2024-05-14T05:07:56Z

CHANGES.md needs a rebase..

It will work only if OSSL_DIGEST_PARAM_XOFLEN is set.

t8m · 2024-05-14T07:25:51Z

The force push was just a rebase with trivial CHANGES.md conflict resolution.

openssl-machine · 2024-05-15T06:00:18Z

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.

t8m · 2024-05-15T10:11:19Z

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 <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #24105)

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (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 <[email protected]>

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 <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from #24463)

It will work only if OSSL_DIGEST_PARAM_XOFLEN is set. Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from openssl#24105)

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (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 <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#24463)

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 <[email protected]> Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Tomas Mraz <[email protected]> (Merged from openssl#24463) (cherry picked from commit a0da3cb)

t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels Apr 11, 2024

t8m requested review from a team and slontis April 11, 2024 08:30

t8m removed approval: review pending This pull request needs review by a committer approval: otc review pending labels Apr 11, 2024

t8m added the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Apr 11, 2024

t8m mentioned this pull request Apr 11, 2024

Create alternative PR for the SHAKE 128/256 default length fix openssl/project#526

Closed

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 11, 2024

t8m force-pushed the shake-mishap-fix branch from a5b3a1f to 91b3565 Compare April 11, 2024 08:41

levitte reviewed Apr 11, 2024

View reviewed changes

providers/implementations/include/prov/names.h Outdated Show resolved Hide resolved

slontis reviewed Apr 14, 2024

View reviewed changes

levitte reviewed Apr 17, 2024

View reviewed changes

slontis mentioned this pull request Apr 23, 2024

Set secure defaults for the SHAKE 128/256 digest output length. #23877

Closed

2 tasks

t8m removed the hold: discussion The community needs to establish a consensus how to move forward with the issue or PR label Apr 23, 2024

t8m requested review from levitte, paulidale and slontis April 24, 2024 16:48

tomato42 approved these changes Apr 25, 2024

View reviewed changes

paulidale approved these changes Apr 25, 2024

View reviewed changes

paulidale removed the approval: otc review pending label Apr 25, 2024

slontis reviewed Apr 25, 2024

View reviewed changes

slontis approved these changes May 14, 2024

View reviewed changes

slontis added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 14, 2024

t8m added 2 commits May 14, 2024 09:23

Intentionally break EVP_DigestFinal for SHAKE128 and SHAKE256

e34d09e

It will work only if OSSL_DIGEST_PARAM_XOFLEN is set.

Document that SHAKE-128 and SHAKE-256 have no default digest length

0ef6ba8

t8m force-pushed the shake-mishap-fix branch from 0fbb78e to 0ef6ba8 Compare May 14, 2024 07:25

t8m 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 May 15, 2024

t8m closed this May 15, 2024

openssl-machine pushed a commit that referenced this pull request May 15, 2024

Document that SHAKE-128 and SHAKE-256 have no default digest length

ad3f28c

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from #24105)

xnox mentioned this pull request May 22, 2024

test: remove the just added, but now unrealistic, shake128 OAEP tests #24463

Closed

jvdsn pushed a commit to jvdsn/openssl that referenced this pull request Jun 3, 2024

Document that SHAKE-128 and SHAKE-256 have no default digest length

8e80a9c

Reviewed-by: Paul Dale <[email protected]> Reviewed-by: Shane Lontis <[email protected]> (Merged from openssl#24105)

Uh oh!

Intentionally break EVP_DigestFinal for SHAKE128 and SHAKE256 #24105

Intentionally break EVP_DigestFinal for SHAKE128 and SHAKE256 #24105

Uh oh!

Conversation

t8m commented Apr 11, 2024

Checklist

Uh oh!

t8m commented Apr 11, 2024

Uh oh!

Uh oh!

levitte commented Apr 11, 2024

Uh oh!

beldmit commented Apr 11, 2024

Uh oh!

t8m commented Apr 12, 2024

Uh oh!

kroeckx commented Apr 12, 2024

Uh oh!

t8m commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomato42 commented Apr 12, 2024

Uh oh!

slontis left a comment

Choose a reason for hiding this comment

Uh oh!

slontis commented Apr 14, 2024

Uh oh!

t8m commented Apr 15, 2024

Uh oh!

slontis commented Apr 15, 2024

Uh oh!

t8m commented Apr 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Apr 17, 2024 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8m Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slontis commented May 14, 2024

Uh oh!

t8m commented May 14, 2024

Uh oh!

openssl-machine commented May 15, 2024

Uh oh!

t8m commented May 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

t8m commented Apr 12, 2024 •

edited

Loading

t8m Apr 26, 2024 •

edited

Loading