SHAKE128 and SHAKE256

[vcsjones]

This implements SHAKE128 and SHAKE256.

Closes #20342

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This implements SHAKE128 and SHAKE256.

Closes #20342

Author:	vcsjones
Assignees:	-
Labels:	area-System.Security, new-api-needs-documentation
Milestone:	-

[vcsjones]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[wfurt]

LGTM. seems like boilerplate from other algorithms for most part, right?

[vcsjones]

LGTM. seems like boilerplate from other algorithms for most part, right?

For the most part, yes. A lot of the hashing capabilities that were needed were already defined. The "new" things were mostly different OpenSSL APIs to deal with the final "XOF" part of the hash.

[bartonjs]

Minor feedback, no blockers.

[stephentoub]

On failure does the managed caller try to use GetLastError to retrieve failure information? I'm wondering if the call to CryptoNative_EvpMdCtxDestroy will overwrite any error information from the EVP_DigestUpdate call.

[vcsjones]

OpenSSL does this a bit differently than Win32. It maintains an error queue. Successful functions don't set the last error to "no error", they just don't add anything to the queue. So EVP_DigestUpdate error is still on the queue, then when we check the return value for failed we use ERR_peek_last_error to grab the error, then ERR_clear_error to clear the queue out.

[vcsjones]

There are some... odd... but probably relevant failures on Mono Linux ARM64. Will dig in to that.

[vcsjones]

Memory issue is a bug in OpenSSL when handling zero length digests from FinalXOF.

@bartonjs extremely grateful for you catching I didn't have a test for that initially.

openssl/openssl#9431

[bartonjs]

Looking at the new tests, I don't see one like

const int TestOutputSize = 23 // or whatever
someData = any non-empty dataset

shake.AppendData(someData);
byte[] save = shake.GetCurrentHash(TestOutputSize);
byte[] empty = shake.GetCurrentHash(0);
// Assert.Equal(0, empty.Length), if desired.

byte[] check = shake.GetCurrentHash(TestOutputSize);
Assert.Equal(save, check);

// Could do it again with shake.GetCurrentHash(empty), if desired

shake.GetHashAndReset(empty);;
shake.GetCurrentHash(check);
Assert.NotEqual(save, check);
Assert.Equal(TShake.HashData(ReadOnlySpan<byte>.Empty, TestOutputSize), check);

shake.AppendData(someData);
shake.GetCurrentHash(check);
Assert.Equal(save, check);

Where Assert.Equal is really hexified things, the AssertExtensions.SequenceEqual version, or a new crypto helper that does SequenceEqual, then on failure does an assert of the hex versions (at least I find the hex differences, and the context around them, easier to work with than both the built-in base10/ToString array version and the base10 diff report from the sequence equal one).

[vcsjones]

Looking at the new tests, I don't see one like

Added a test that shows GetHashAndReset resets even when asking for a zero-length hash.

[vcsjones]

Timeouts are being tracked by #76454. Tests pass locally and have passed in previous runs. Merging (with 👍 from @jeffhandley)

[Neustradamus]

@vcsjones: Thanks a lot for your work!

Linked to:

• Add support for SHA3 (Keccak) #20342
• SHA3 #84132