Conversation
|
At a quick first read of the PR description (which is very long, so I didn't read the whole thing), I am not really sure what problem this is solving, so I will need to take an actual read of the code diff to see what's going on. My immediate thoughts are:
in the QUALITY_AND_STYLE.md as a higher priority than turning runtime errors into compile-time errors. |
| pub trait XofOutput { | ||
| /// Produces the next `num_bytes` bytes of the output stream. | ||
| /// | ||
| /// BC Java's `Xof.doOutput(out, outOff, outLen)`. |
There was a problem hiding this comment.
I think that's extremely weird to have a comment in bc-rust that references bc-java.
I guess the design / architecture discussion that needs to be had here is whether it's a goal to align bc-rust APIs to bc-java APIs, or whether we consider this a greenfield implementation and should be free to do what makes sense, regardless of what other parts of the BC family do.
There was a problem hiding this comment.
Just a guide, I wouldn't commit this, it's still under development though and I'm using the BC implementations of SP 800-185 to guide the process and provide compatibility tests. I'll clean these up before final commit.
| /// agree on their first 32 bytes. An attacker who only needs to know that two values came from the | ||
| /// same input -- enough to break an anonymity property -- learns it from the overlap. Where that | ||
| /// matters, salt the input. | ||
| pub trait XOF: Hash { |
There was a problem hiding this comment.
My initial reaction is that I don't like this.
NIST has explicitly stated that XOFs are not hashes, and must not be used as hashes. FIPS 202 Appendix A.2 is essentially an essay on that topic, so I think this is going to get us into trouble.
I'm open to having my mind changed though.
There was a problem hiding this comment.
Understand how could happen. You'll see the issue when you get to the last paragraph of FIPS 202 Appendix A.2, it starts out like that, but by the end we can see it's all just one big happy family. The issue they talk about at the start is the motivation behind the design of cSHAKE, so I agree it would make things clearer if it was actually here.
So I've had a look further in BC, I think the only example of an XOF being used as both a hash and an XOF together is RSA-PSS. It would be possible to split the two services, although an RSA-PSS implementation would require both an XOF and a digest passed to it, as opposed to just digest that happens to be an XOF. I think everywhere else it's either digest or XOF. PSS might be a sign of things to come though, as in where the XOF is used to replace a KDF/MGF so it really does feel that having XOF extend Hash is better. The real problem we need to address is that Rust allows us to have a method which says "I'm always good" and for an XOF, update as "I'm always good" is not correct...
In that respect the current solution seems like the most "Rusty" one, as activating the XOF side of the service disables it's digest side, but I'm more than happy to entertain alternatives - XofOutput was just the only thing that the Internet, LLM, and myself could come up with that worked.
There was a problem hiding this comment.
Backing up a step: why would someone want SHAKE to behave as a hash? I guess because you want a fixed-size output at a size larger than 512 bits (cause otherwise you'd just use SHA3_512 and truncate it). But since you don't get any additional security out of that -- it's still only 256-bit collision-resistant -- I'm not entirely sure why someone would want to do that. Probably in 98% of cases where someone wants to do that, they would be better served by using the [KDF] interface that SHAKE already impls.
As you say, the proper way to turn SHAKE into a hash function is to include the output length in the input. Like
SHAKE_X( m ) = SHAKE( u32::to_bytes(X) + m, X) = <X bit output>
Oddly, this is not what cSHAKE is because it doesn't swallow the L param, so cSHAKE is still not a [Hash].
There was a problem hiding this comment.
No, believe it or not, people actually use SHAKE as a digest instead of SHA3-512 or SHA3-256. RSA-PSS is actually an example of this (using SHAKE as both a digest and a mask generator), CMS as well.
Some other interesting examples of SHAKE as a digest, SP 800-208 (LMS and XMSS) and FIPS PUB 205.
|
|
||
| /// As [`do_output`](Self::do_output), filling the caller's buffer, which is zeroized first. | ||
| /// Returns the number of bytes written. | ||
| fn do_output_out(&mut self, output: &mut [u8]) -> usize; |
There was a problem hiding this comment.
I defined the XOF trait in the language of sponge constructions (absorb / squeeze) because that's what FIPS 202 does, and sponge functions have a well-defined API.
Do you have a reference (ideally a FIPS or RFC) that defined XOFs in an abstract way using the update / output language?
There was a problem hiding this comment.
https://www.rfc-editor.org/rfc/rfc8702.html is probably the most general one. The gap in it is it misses the -len OIDs, which mean the AlgorithmIdentifier carrying the OID is also carrying an INTEGER saying how long the output should be. There's also https://www.rfc-editor.org/rfc/rfc8692.html. https://www.rfc-editor.org/rfc/rfc8419.html is an example of a standard using -len OIDs. They're starting to appear everywhere now, KMAC as well. Haven't seen much with TupleHash/ParallelHash yet, but I'm guessing it's just a matter of time. You're probably getting the picture though.
There was a problem hiding this comment.
I don't see any of those three RFCs defining SHAKE with a do_update / do_final API instead of an absorb / squeeze API. In fact, I don't see them describe an API for SHAKE at all?
I do see them using the word "digest".
To me, the word "digest" means to take an arbitrary-length input and turn it into something smaller and there are two types of cryptographic digest functions:
- Hash functions always produce the same length output.
- XOFs produce a variable-length output.
This is why I haven't used the word "digest" in bc-rust, but instead define [Hash] and [XOF] directly to encapsulate their differences in behaviour.
(btw, this kind of discussion is fun 😊 This is setting the theoretical foundations of the library)
There was a problem hiding this comment.
RFC 8702, section 1, second paragraph:
"In the SHA-3 family, two extendable-output functions (SHAKEs), SHAKE128 and SHAKE256, are defined. Four other hash function instances (SHA3-224, SHA3-256, SHA3-384, and SHA3-512) are also defined but are out of scope for this document. A SHAKE is a variable-length hash function defined as SHAKE(M, d) where the output is a d-bit-long digest of message M. The corresponding collision and second-preimage-resistance strengths for SHAKE128 are min(d/2,128) and min(d,128) bits, respectively (see Appendix A.1 of [SHA3]). And the corresponding collision and second-preimage-resistance strengths for SHAKE256 are min(d/2,256) and min(d,256) bits, respectively. In this specification, we use d=256 (for SHAKE128) and d=512 (for SHAKE256)."
The length is based on the calculations in Appendix A.1 of FIPS PUB 202. 256 and 512 give are the minimum lengths required to meet 128 bit and 256 bit security respectively, and as a result encountering an OID for either SHAKE128 or SHAKE256 means a digest of length 256 and 512 bits respectively. If there's an arbitrary length involved the -len OIDs are used. The OIDs are defined here:
https://csrc.nist.gov/projects/computer-security-objects-register/algorithm-registration#Hash
They get used as digests, we've been using them as digests in BC Java/C# since 2015, I know it might seem weird, but they're digests. As I mention somewhere else, in PSS they are even used as both digest and XOF (if SHAKE is the signature digest, the mask-generator function is not MGF1 but SHAKE). Anyway, they're digests.
As an interesting aside, the section of the extendible hashes finishes with:
"If d > r + c/2, then SHAKE128 and SHAKE256 provide more than 128 and 256 bits of preimage
resistance, respectively; moreover, if d > 1600, a preimage probably does not exist."
A very brave statement, especially for NIST!
I guess the other, cautionary note, I should add - these functions all come with padding. Defining the API in terms of squeeze() and absorb() makes it look like an internal API which is being made public, it's likely the CMVP will complain about it, especially as they'll be expecting them to look like digests with a bit extra.
| //! SHA3 offers Extendable-Output Functions in the form of SHAKE, which is accessed through the [`XOF`] trait, | ||
| //! which is implemented by [`SHAKE128`] and [`SHAKE256`]. | ||
| //! The difference from [`Hash`] is that SHAKE can produce output of any length. | ||
| //! [`XOF`] extends [`Hash`] -- SHAKE *is* a hash -- and adds the ability to choose the output length. |
There was a problem hiding this comment.
Yeah, see, that comment directly and emphatically contradicts FIPS 202 appdx. A.2, and that makes me comfortable.
There was a problem hiding this comment.
See the last paragraph of A.2. The comment is correct.
|
The branch name and PR description says that this PR implements cSHAKE from SP 800-185, but I don't see any cshake in the actual diff. I'm confused. |
b212aea to
a7dd1a4
Compare
a7dd1a4 to
9006d21
Compare
d2a9b35 to
d1dcf75
Compare
9006d21 to
e909d5e
Compare
e909d5e to
9006d21
Compare
2fec0ab to
5c45ae8
Compare
|
cSHAKE, TupleHash, ParallelHash, and KMAC now added. |
|
I'm really beginning to feel an active dislike for the way this thing manages conversations... I was just re-reading, on 2 mentioned way, way, above, XofOutput is not a wrapper, it's a generator - the idea is that the digest passes that off containing a reference to itself and then becomes a pure XoF with the original construction becoming unusable courtesy of the borrow checker. This is why I asked about Clone earlier - otherwise an XoF would become single shot, a disaster if you were in a situation where you could take advantage of pre-computation, as you wouldn't be able to re-use the previous runs of the compression function as soon as you created an XoF (which is still at the end of the day, just a fancy digest...) |
|
So, I've been re-reading SP 800-185... the situation is actually a bit more nuanced than either of us actually appreciated, especially me. It actually looks more like this: (Hash) SHA3-224, SHA3-256, SHA3-384, SHA3-512 We got away with combining hash and XOF in bc-java as SP 800-185 arrived later and we'd already implemented SHAKE (we actually got CAVP cert #2 for SHA-3). It worked for TupleHash, ParallelHash, and KMAC as BC Java switches between the hash variant and the XOF variant of the last three by the doFinal() call used, so calling doFinal() without an output length tells it to behave like a hash, calling do final and passing an output length makes it an XOF unless the length is the default one. Any calls to doOutput prior to doFinal() force XOF behavior throughout. So while KMAC128 - doFinal(0, buf)/doFinal(0, buf, 32) produce the same result - either a call to doOutput() first or a non-default-length to doFinal() run as KMACXOF128. Rust doesn't quite work like that because of lack of overrides, which is partly what lead to the use of the XofOutput generator. Anyway I've done some further refactoring which removed some duplicated code that crept in. XofOutput is now XOFOutput and simplified. Possible Decision Thing One thing that did show up though is the non-XOFOutput for SHAKE.do_final_out() fills the array it's given, rather than just writing 32 or 64 bytes. Given SHAKE really is Hash/XOF this is possibly okay, it's just an oddity of the function, the same would apply for cSHAKE. The solution would be to add an equivalent do_final_out() to XOFOutput, and then force the super trait one to only write the default number of bytes... the cSHAKE variants are okay because the actual types are present, unlike in bc-java, so the hash variant is already locked down, and the XOF will either be a hash for the default length, or some other result, with do_output on the XOF encoding "infinite output" correctly. |
|
One other approach possible approach just occurred to me - we could keep Hash and XOF separate - but we'd need to duplicate SHAKE and cSHAKE (at least the externals). It's a bit of a vibe thing - with Clone plus XOF, extending Hash works fine, the only difference is the XOFs will do a fill on do_final_out() and produce the default output length on hash(), which does actually make sense, if you think about it. We could avoid that by introducing SHAKEXOF and cSHAKEXOF, which in some ways makes less sense, even though in some ways it does. |
|
Okay, I have an answer. It looks similar to the earlier ones, but needs to incorporate one extra change. We do the Clone/swap trick as before. The XOF trait extends Hash but contains something like: Hash.do_final for the algorithms supporting Digest/XOF so that the Hash do_final* produces a digest of the correct length. This puts the oneshot hash and do_final into line, for XOF behaviour (where the generation of n bytes is required) people can use the do_final* on the XOFSqueezer to fill a byte array as expected - this will keep the Hash-side of the function consistent across all uses in Hash, while at the same time allowing for use of XOF-side behaviour when requested (so do_final() with no do_output uses right_encode(L), whereas if do_output is called first it's simply a do_output saying I'm finished). As an added advantage it will mean RSA-PSS will only need one instance of it's digest, rather than two. |
…ueezing becomes its own type
…oFinal after doOutput
… encodings and cshake CLI subcommands
…tions and the commit message style in CLAUDE.md
…tory registration and kmac CLI subcommands
…r requires Default
…yed XOFs can use it
…pdate appends one tuple element
…ting the Recommendation
…5 on the command line
… KMAC against the sample values, plus KMAC's key-type and buffer-length checks; kills the 88 mutants the SP 800-185 suites had missed
…al suite against the SHAKE types; of 29 missed mutants only the equivalent default_128_bit one survives
…orked and finished several ways from one absorbed prefix; the SP 800-185 types and the factory enums derive it, the sha2 and sha3 params traits require it, and the framework hash and XOF suites check a clone finishes like its original and diverges on different input
…E overrides them, XofOutput is renamed XOFOutput to match the spec capitalisation used everywhere else, Hash::output_len documents that a XOF's length is nominal rather than part of the function, and the BC Java asides come out of the Hash and XOF docs
…sh suite covering short, exact and over-long output buffers, for the implementors that take constructor arguments and so cannot reach test_hash
… than output_len instead of truncating, and neither they nor KMAC zeroized past the digest as the Hash and MAC contracts require; the two suites that had pinned the old behaviour are corrected and all three types now run the framework's buffer-length checks
… overrode and nothing outside their own tests called; a squeeze has nothing to finalize, so ending the stream is dropping the value, and the XOF suite now checks do_output_out zeroizes the buffer where it had checked the alias agreed with do_final
…he XOF squeezing vocabulary, so XOFOutput becomes XOFSqueezer with SHAKEOutput and XOFFactoryOutput following it, XOF::Output becomes XOF::Squeezer, into_output and into_output_partial_bits become into_squeezer and into_squeezer_partial_bits, and the one-shots hash_xof and hash_xof_out become xof and xof_out; mechanical throughout, with no behaviour change
…ut length, so XOFSqueezer gains do_final and do_final_out, KMACXOF, TupleHashXOF and ParallelHashXOF defer their right_encode(L) to the first read through a new LengthBoundSqueezer and compute the fixed-length function of SP 800-185 s. 4.3, 5.3 and 6.3 whenever do_final or a one-shot is that read, and the Hash view of every XOF, SHAKE and cSHAKE included, becomes a final read at output_len that zeroes the rest of the caller's buffer
…a bare run examines only the root package and finds nothing, the checked-in config's examine_globs silently overrides -f, crates whose mutants die in another crate's tests need --test-workspace, and without the /tmp/bc-test-data symlink the vector suites pass vacuously and their mutants all read as missed
4ebf008 to
8a46683
Compare
Executive Summary
This is a complete PR to add support for SP 800-185, it's attempting to address two things, the first being that XOF functions are treated in a lot of situations as message digests, and there's even OIDs for the same, the second is how to deal with variable length output across TupleHash, ParallelHash, and KMAC (the last one actually qualifying as a MAC and an XOF...).
After a bit of to-ing and fro-ing, partly due to the lack of overrides in Rust, the best solution to the problem seemed to be the creation of XofOutput, which is a small trait the getting of which disables the use of the parent structure - this allows do_update on the Hash trait to stay infallible, but thanks to the borrow checker, causes a compile time error if XofOutput is created and someone attempts to use the parent Hash. Most of the text here is devoted exclusively to that issue, it also includes the sample code for the trait, and the sample usage below. There's no need to look anywhere else (yet).
First question: given the constraints, does this look okay as a solution? Second question: If so, I guess it should be XOFOutput shouldn't it?
I note this will need to be re-based once #117 goes in.
Comments welcome.
Issue Link
No linked issue.
Summary
XOFnow extendsHash, so SHAKE128 and SHAKE256 are hashes and can be used wherever one iswanted; the squeezing phase becomes its own type, which turns the absorb-then-squeeze rule from a
runtime error into a compile error.
Description
Stacked on #115, so this PR is based on
feature/symmetric-cipherand shows four commits:e3c31e3XOFextendsHash; squeezing becomes its own typed02361eblock_bitlenandoutput_lenvalues, which three mutants survived19f00a0XofOutputgainsdo_final/do_final_outa7dd1a4The goal. SHAKE128 and SHAKE256 should be usable as hashes. This is the relationship BC Java
draws with
Xof extends ExtendedDigest extends Digest, and it is what lets a caller hold "somehash" and pass a SHAKE.
What blocked it.
Hash::do_updateis infallible;XOF::absorbreturnedHashError::InvalidStatefor absorb-after-squeeze. BC Java has the same rule but signals it with anunchecked
IllegalStateExceptionfromKeccakDigest.absorb, which is not an option here. So theerror had to go somewhere, and the three candidates were: make
Hash::do_updatefallibleeverywhere, panic, or remove the reachable state.
Why the third. Every absorb site in ML-KEM and ML-DSA already read
.expect("absorb before squeeze is infallible")— 117 of them. The callers were unanimous that theerror could not happen in correct code, which is exactly the case QUALITY_AND_STYLE.md says to
redesign rather than wrap in a
Result:So
XOF::into_outputconsumes the XOF and returns anXofOutput. After output has begun there isno value left to call
do_updateon, the error has nothing to report, and all 117.expect(...)calls are gone.
Err()counts in core code dropped from 314 to 307.The invariant that makes
do_updateinfallible is that aSHAKEInternala caller can name hasnever squeezed. That holds because every
KDFentry point also takesselfby value, and it ispinned by a
debug_assert!indo_updateplus acompile_faildoctest.XofOutput
Sample code — all five of these compile and their assertions pass:
And the case that no longer compiles, which is the point of the change:
Method mapping.
Digest.getDigestSize()Hash::output_len— 32 / 64,fixedOutputLength / 4asSHAKEDigest.java:84ExtendedDigest.getByteLength()Hash::block_bitlen, in bits — 1344 / 1088, the rate1600 - 2cDigest.updateHash::do_updateDigest.doFinal(out, off)Hash::do_finalXof.doOutput(out, off, len)XOF::into_outputthenXofOutput::do_outputXof.doFinal(out, off, len)afterdoOutputXofOutput::do_finalDigest.reset()selfXofOutput::do_finalis a provided method delegating todo_output.SHAKEDigest.java:92-99shows Java's
doFinal(out, off, outLen)isdoOutputplusreset(); here takingselfby valueis the reset, and dropping the handle zeroizes the sponge through
Secret'sDrop. It is a namefor "this read is my last", not new behaviour.
Suspendable. The suite suspends a squeezing SHAKE and resumes it, which under the new design
must not hand back an absorbing value.
SHAKEOutputtherefore has its ownCloneandSuspendable, and eachfrom_suspendedrejects the other's phase withInvalidData. This is abehaviour change:
SHAKE128::from_suspendednow refuses a state that was suspended mid-output.Alternatives considered. Making
Hash::do_updatefallible would have put aResulton SHA-2,SHA-3 and SM3 for a condition only XOFs can reach. Panicking is against house rules. A shared
metadata-only supertrait for
HashandXOFwas considered and rejected: it avoids thedo_updateproblem but does not give substitutability, which was the whole point.Scope and Risk
core,core-test-framework,sha3,factory,mlkem,mlkem-lowmemory,mldsa,mldsa-lowmemory,cli,mem_usage_benches.openssl dgstbyte forbyte for both variants, and the NIST CAVP bit-oriented vectors and the ML-KEM / ML-DSA
known-answer tests all pass unmodified. The one deliberate behaviour change is
SHAKE128::from_suspendedrejecting a squeezing state.this is a breaking change to
XOFand every call site moved.restart the output stream instead of continuing it. That would break the ML-KEM / ML-DSA KATs
loudly rather than silently, and it did during development.
Validation
cargo mutantsonshake.rsagainst the full workspace suite: 126 mutants, 89 caught, 4 missed,33 unviable. Three of the four survivors were real gaps — nothing pinned the actual values of
block_bitlenoroutput_len, so1600 - 2csurviving as1600 + 2cwent unnoticed.d02361eadds that test and all three now die. The fourth is a genuine equivalent mutant,
|versus^ondisjoint bit ranges, verified exhaustively over all 2048
(num_bits, partial_byte)pairs andcommented at the site. The 33 unviable are the
.cargo/mutants.tomlerror values not typecheckingagainst
HashError/KDFError/SuspendableError.AI Usage Statement
Did you use AI in creating this pull request:
If submitted code changes were generated by AI, fill in the following declaration:
Assisted-by: Claude Code:claude-opus-5