Feature/sha3 final patches by dghgit · Pull Request #87 · bcgit/bc-rust

dghgit · 2026-08-26T05:55:28Z

changes two things:

partial-bit squeeze returned the wrong bits, it was tested, but it appears the test was using 0xff which hid the error.
validates num_partial_bits in do_final_partial_bits so it has to be between 0 and 7. Previously 8 to 15 meant the data was just absorbed and 16 and greater caused a panic.

Just to note, I'm getting told that main won't pass the format check... the diffs have been checked though.

ounsworth

This is changing quite a bit of code that had been tested with cargo mutants. Have you redone the cargo mutants testing? I can walk you through how to do that.

ounsworth · 2026-08-26T23:44:33Z

+//!   length must be bound to the digest, include it in the message (FIPS 202 Appendix A.2). For this
+//!   reason SHAKE does not implement [`Hash`].
+//! * Once squeezing begins, no further input can be absorbed; [`XOF::absorb`] returns
+//!   [`HashError::InvalidState`] rather than silently producing an unapproved construction.


This also doesn't feel like a security consideration?

Also, I would like opinions on this because some people have pushed back on me that while FIPS 202 clearly illustrates the sponge construction as

absorb() -> absorb() -> squeeze() -> squeeze()

it doesn't technically prohibit

absorb() -> squeeze() -> absorb() -> squeeze()

and so bc-rust should relax this requirement. Unrelated to whether this is a "security consideration" or not, I would love your input on this.

It's a confusion between what is in https://eprint.iacr.org/2011/499 (the original SIG SAC paper) and what's in FIPS PUB 202. Understandable, but FIPS PUB 202 Section 6.2 defines SHAKE128(M, d) = KECCAK[256](M || 1111, d) so in the FIPS PUB the construction is actually absorb() -> pad() -> squeeze() there is no room for another absorb() after the pad. So yes, you might be able to argue that Keccak allows for this, but the SHAKE construction given in FIPS PUB 202 most definitely does not.

That said, while I don't know what the byte string might look like, I can say absorb() -> squeeze() -> absorb() -> squeeze() would also produce a CVE under CWE-682, possibly even CWE-1240 if the reporter was feeling nasty, I guess it would be a medium, not a critical, but even then I doubt any of us would enjoy the grief.

ounsworth · 2026-08-26T23:46:04Z

+//! * Once squeezing begins, no further input can be absorbed; [`XOF::absorb`] returns
+//!   [`HashError::InvalidState`] rather than silently producing an unapproved construction.
+//! * The sponge state and queue are held in [`bouncycastle_utils::secret::Secret`] and zeroized on
+//!   drop. Transient copies in registers/stack locals during the permutation are not zeroized.


Is this too much internal detail for docs? I don't think we go into this much analysis for any other algs. If we should, then we should do them all in one pass a self-contained task and make them cohesive, not do it ad-hoc.

It's been moved into the XOF usage docs. If it still seems a bit much, I wouldn't abbreviate it further until this is ready to merge (see comment at the end).

ounsworth · 2026-08-26T23:47:29Z

+//!   [`HashError::InvalidState`] rather than silently producing an unapproved construction.
+//! * The sponge state and queue are held in [`bouncycastle_utils::secret::Secret`] and zeroized on
+//!   drop. Transient copies in registers/stack locals during the permutation are not zeroized.
+//! * The implementation contains no data-dependent branches or table lookups.


This is stated as a fact, but I think at the moment it's conjecture. We have #75 to build a ct testing framework to validate this.

I feel uncomfortable making ct claims that we haven't validated.

ounsworth · 2026-08-27T00:08:11Z

-
-        let min = if output.len() >= self.output_len() { self.output_len() } else { output.len() };
-        Ok(self.keccak.squeeze(&mut output[..min]))
+        Ok(self.finalize(partial_byte, num_partial_bits, output))


Can you please explain this one to me. It's not immediately clear to me why this is an equivalent substitution.

Following rational was given:

Three things changed, none affecting behaviour:

self.output_len() → PARAMS::OUTPUT_LEN. output_len() is defined as PARAMS::OUTPUT_LEN
(see the Hash impl a few lines above), so this is the same constant, just resolved at compile
time rather than via a method call — consistent with how the rest of the impl uses PARAMS::.

output.as_mut_slice() → &mut output. Vec<u8> deref-coerces to &mut [u8]; identical.

Removed dbg_rslt_len / debug_assert_eq!(bytes_written, dbg_rslt_len). The buffer is allocated
with exactly OUTPUT_LEN bytes and do_final_partial_bits_out returns
min(output.len(), OUTPUT_LEN) = OUTPUT_LEN, so the assert was checking a tautology about a
buffer we just sized ourselves.
One thing, the corresponding do_final() still has its assert as I asked it to keep the changes focused. The assert could either be deleted there for consistency, or re-introduced back here for consistency. I'd delete the other as it appears it's also checking something that's now turned into a tautology (I suspect this wasn't always true, but as the code's evolved this has happened).

ounsworth · 2026-08-27T00:12:02Z

-        if !(1..=7).contains(&num_partial_bits) {
-            return Err(HashError::InvalidLength("must be in the range [0,7]"));
+        // Validate before shifting: `1 << num_partial_bits` on a u16 would overflow for values >= 16,
+        // and 8..=15 would silently absorb garbage. 0 is allowed and simply finalizes with no partial byte.


I find this comment more confusing than helpful.
What is the ">= 16" referring to? Because the condition we're checking is "> 7"?

This function is called absorb_last_partial_byte, so clearly trying to absorb more than 7 is no longer a "partial byte" and you should be using a different API.
I don't know what this comment is trying to tell me, but it's not that.

Also, doesn't this change behaviour? The old logic was "if !(1..=7), and the new behaviour is "if > 7". But what about 0?
I think we need to have a careful look at the API docstring and the FIPS doc and see if num_bits = 0 is a valid "partial byte" or not.
(it would make sense to me that it is, because ... why not ... but we should check. And presumably if we do it wrong, then wycheproof and bc-test-data will catch it, once we get around to wiring those up).

(either way, we should align the error message to the final choice)

The comment was pointing out that the old check was needed before (1 << num_partial_bits) on a u16 (which panics in debug / wraps in release for >= 16), but "must be 0..=7 because it's a partial byte" is the real justification. I've replaced it, although a little wary about this one as it also feels like memory - I think it's safe though, nothing is likely to role it back by accident.

It is a behaviour change as there's no longer a panic and previously SHAKE::absorb_last_partial_byte rejected 0 with the message "must be in the range [0,7]" which seemed a little odd to me.

ounsworth · 2026-08-27T00:18:30Z

+
+        // FIPS 202 Appendix B.1 (h2b): the first `num_bits` bits of an output byte are its least
+        // significant bits. This matches the input-side convention used by absorb_last_partial_byte().
+        *output = buf[0] & ((1u8 << num_bits) - 1);


Ah good catch. That seems like a legitimate bug 👍

I don't think I have done bc-test-data or wycheproof tests yet for the sha2 and sha3 crates. Presumably this would have been caught.

That said, I'm not sure that the comment is helpful since this is already stated on the new docstring comment on core::traits::Hash, right? I think this comment should be deleted.

Yes, the bug part 1.

We don't have partial byte tests, I'd be surprised if Wycheproof do either. We can get some from the ACVP though, they will generate them for both SHA3 and SHA2. To be honest, I've never seen partial bytes used, but given the strange world of small devices, I wouldn't be surprised if we'll run into them here... I'll try and get ACVP vectors into bc-test-data for the partial results.

ounsworth · 2026-08-27T00:22:33Z

        let mut buf = [0u8; 1];
-        self.keccak.squeeze(&mut buf);
-        *output = buf[0] >> 8 - num_bits;
+        self.squeeze_out(&mut buf);


This is changing behaviour and implying that we had a bug before. Great!

I don't think I have done bc-test-data or wycheproof tests yet for the sha2 and sha3 crates. Presumably this would have been caught.

(also, while this comment is helpful to explain the diff, I think we should delete the comment before merging)

See above. It was tested, but the string was 0xff, so it constituted a fully populated byte. As for the comment, see the comment at the end, I'd do a "house comment" pass before merging as the comments in the branch currently represent a mixture of "comment" and "memory".

ounsworth · 2026-08-27T00:48:06Z

+//! | Object                                  | Size (bytes) |
+//! |-----------------------------------------|--------------|
+//! | `SHA3_224` .. `SHA3_512`, `SHAKE128/256` | 440          |
+//! | Suspended state ([`Suspendable`])       | 415          |


For my curiosity, where did these numbers come from?
Where I've done this on other crates, I have, for example, a /mem_usage_benches/bench_mldsa_mem_usage.rs that prints struct sizes using rust's size_of::<> operator.

This PR has not added equivalent mem benches for SHA3, so how did you measure these numbers?

As you guessed, it was a throw away though, as I was just focusing on the review issues. Did you want me to commit in something equivalent (would probably suggest a separate branch but can do here).

I've added mem_usage_benches/bench_sha3_mem_usage.rs as well.

ounsworth · 2026-08-27T00:52:36Z

We should probably port the bc-test-data and wycheproof test harnesses over from the mldsa crate as part of this PR.

dghgit · 2026-08-27T01:08:10Z

I've tried to answer stuff inline (should start appearing in a minute), but one note, with the comments, as the branch is also been worked on by an LLM, as a general rule work like this (while on the branch) will likely include comments that may seem unnecessary or out of place. I've learned to leave this alone until the work on the branch is finished as they also form part of the LLM writing notes to itself, so deleting them early is really only useful if you want to increase the chance of an error or something being missed. On merging though, feel free to make whatever edits you want if you feel like something doesn't match the house rules (as it happens LLMs like that sort of consistency as well, just not having it introduced midway through development).

Cargo mutant was redone as well.

One other note: cargo fmt kept trying to change key_material.rs in crypto/core/src, it might need a second look at.

- lib.rs: drop internal sponge/stack detail from Memory Usage; remove the "SHAKE does not implement Hash", absorb-after-squeeze, constant-time and KeyMaterial-caveat bullets from Security Considerations (CT claim awaits #75; KeyMaterial caveat belongs on KeyMaterial itself). - sha3.rs: rename private shared finalizer finalize() -> do_final_bits_out() per naming convention; drop redundant comments. - shake.rs: replace range-check / bit-ordering comments with a one-liner each. - core traits: document that num_partial_bits = 0 is valid for do_final_partial_bits* and absorb_last_partial_byte, and explain on XOF why absorb-after-squeeze (duplex) is rejected. - tests: assert the 7-bit upper boundary is accepted by absorb_last_partial_byte (kills the shake.rs `>` -> `>=` mutant found by cargo mutants). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Replace the vendored copies in crypto/sha3/tests/data/ with the same lookup convention used by the mldsa/mlkem crates: read SHA3TestVectors.txt and SHAKETestVectors.txt from ../bc-test-data/crypto (or ../../../bc-test-data when run from the crate directory), printing a one-time warning and skipping the vector tests if the repo is not checked out. The vector files were byte-identical apart from the download URL in the header comment. Requested in PR #87 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

dghgit · 2026-08-27T02:32:12Z

I've ported the test data harness and it's now in this PR as well. Wycheproof doesn't appear to provide vectors for SHA-3/SHAKE they point people at the NIST ones. I'll update the references in Rust to point at the partial byte vectors when we manage to generate them (it's going to mean tweaking a bit of JSON).

dghgit · 2026-08-27T03:10:06Z

Latest commit adds testing for partial-byte vectors. NIST standard vectors have been added to bc-test-data/crypto/sha3

Note: latest commit also fixes a bug in Keccak concerning partial-bytes. Seems there was one last one lurking in the woods.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… cleanups Bugs: - SHAKE squeeze_partial_byte_final[_out] bypassed the SHAKE "1111" domain suffix when it was the first squeeze, returning raw Keccak output instead of SHAKE output. Now routes through squeeze_out(). - The same function returned the high bits of the output byte; FIPS 202 B.1 bit ordering (and our own input-side convention) makes the first bits the low bits. Now returns the low num_bits bits; XOF trait doc updated to match. - SHA3 do_final_partial_bits[_out] did not validate num_partial_bits: >=16 panicked with a shift overflow, 8..=15 silently hashed garbage. Now returns HashError::InvalidLength for anything above 7. - SHAKE absorb_last_partial_byte now accepts 0 partial bits (consistent with SHA3) and error strings state the actual accepted range. Cleanups: - std::marker::PhantomData -> core::marker::PhantomData (no_std goal). - Blanket `impl HashAlgParams for SHA3Internal<P>` forwarding to the params struct, replacing four hand-duplicated impls and stale commented constants. - Crate docs: added Memory Usage and Security Considerations sections, fixed typo, documented the *_NAME constants. - Removed .clone() on Copy types and redundant branch in do_final_out. - keccak_tests::test_keccak now asserts instead of printing. Regression tests added for all of the above. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…_out Mirrors the sha2 structure: do_final_out is the zero-partial-bits case of a single spec-commented finalize(), and do_final_partial_bits_out validates num_partial_bits then delegates. Removes the second hand-rolled suffix path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

- lib.rs: drop internal sponge/stack detail from Memory Usage; remove the "SHAKE does not implement Hash", absorb-after-squeeze, constant-time and KeyMaterial-caveat bullets from Security Considerations (CT claim awaits #75; KeyMaterial caveat belongs on KeyMaterial itself). - sha3.rs: rename private shared finalizer finalize() -> do_final_bits_out() per naming convention; drop redundant comments. - shake.rs: replace range-check / bit-ordering comments with a one-liner each. - core traits: document that num_partial_bits = 0 is valid for do_final_partial_bits* and absorb_last_partial_byte, and explain on XOF why absorb-after-squeeze (duplex) is rejected. - tests: assert the 7-bit upper boundary is accepted by absorb_last_partial_byte (kills the shake.rs `>` -> `>=` mutant found by cargo mutants). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Replace the vendored copies in crypto/sha3/tests/data/ with the same lookup convention used by the mldsa/mlkem crates: read SHA3TestVectors.txt and SHAKETestVectors.txt from ../bc-test-data/crypto (or ../../../bc-test-data when run from the crate directory), printing a one-time warning and skipping the vector tests if the repo is not checked out. The vector files were byte-identical apart from the download URL in the header comment. Requested in PR #87 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…tail Adds crypto/sha3/tests/cavp_tests.rs reading the SHA3VS .rsp files from ../bc-test-data/crypto/sha3/{bit-oriented,byte-oriented}/ with the same lookup/skip-with-warning convention as the other crates: SHA3 ShortMsg, LongMsg and Monte (s. 6.2.2) for SHA3-224/256/384/512, and SHAKE ShortMsg, LongMsg, VariableOut and Monte (s. 6.2.3) for SHAKE128/256 — 40 tests, ~13k message cases (~7.7k bit-length inputs, ~1.7k bit-length outputs). Bit ordering confirmed from the vectors: SHA-3 CAVP follows FIPS 202 B.1 and packs excess input and output bits in the least significant bits of the final byte (100% of partial cases have zero high bits), matching the Hash/XOF partial-bit API directly, unlike SHA-2 CAVP which is MSB-first. The harness found a bug: KeccakInternal::absorb_bits(_, 0) returned early without switching to the squeezing phase, so when 4 trailing message bits plus the SHAKE "1111" suffix exactly filled a byte, absorb_last_partial_byte left squeezing == false and the first squeeze applied the suffix a second time. Every SHAKE message with Len % 8 == 4 was wrong; the NIST example vectors (5/30/1605/1630 bits) cannot reach this case. absorb_bits(_, 0) now pads and switches phase after the usual state checks. Regression tests: the CAVP SHAKE128 Len = 4 vector in shake_tests, and a keccak unit test pinning absorb_bits' range and phase behaviour. Note: cargo mutants runs in a copied tree where ../bc-test-data does not resolve, so vector-file tests skip during mutation testing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The num_partial_bits message bits are taken from the least significant bits of partial_byte (FIPS 202 Appendix B.1) for every hash family, including SHA-2 where FIPS 180-4 defines no packing. Notes that NIST CAVP SHAVS (SHA-2) vectors pack MSB-first and need shifting, while SHA3VS vectors already use the LSB convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…A3/SHAKE Follows bench_mldsa_mem_usage / bench_mlkem_mem_usage: print_struct_sizes() reports size_of for SHA3_224..SHA3_512, SHAKE128/256 (440 bytes) and SUSPENDED_SHA3_STATE_LEN (415), which are the numbers in the crate's Memory Usage table; the remaining entry points (one-shot hash, streaming, XOF squeeze, suspend/resume) are for valgrind --tool=massif stack measurement. The crate docs now point at the bench as the source of the table. Requested in PR #87 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

ounsworth requested changes Aug 27, 2026

View reviewed changes

hubot force-pushed the feature/sha3-final-patches branch from 145a205 to 356dd7a Compare August 27, 2026 01:30

hubot force-pushed the feature/sha3-final-patches branch 2 times, most recently from 359f609 to e3384b6 Compare August 27, 2026 03:07

dghgit added a commit that referenced this pull request Aug 27, 2026

Add 0.1.3 release notes for the SHA-3 changes (PR #87)

b7fd156

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

dghgit and others added 8 commits August 27, 2026 13:55

sha3: rustfmt

989e339

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Add 0.1.3 release notes for the SHA-3 changes (PR #87)

8496d7e

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

hubot force-pushed the feature/sha3-final-patches branch from a766e18 to 28ef251 Compare August 27, 2026 04:19

dghgit changed the base branch from main to release/0.1.3alpha August 27, 2026 04:57

hubot force-pushed the feature/sha3-final-patches branch from 28ef251 to 8496d7e Compare August 27, 2026 05:07

This was referenced Aug 27, 2026

SHA3 bug fixes: around partial-byte handling #91

Open

Add NIST CAVP tests to the SHA3 crate #92

Open

Refactor/sha3 code structure #93

Open

sha3, rng cleanup #94

Open

sha3: add Memory Usage bench and Security Considerations docs #95

Open

Uh oh!

Conversation

dghgit commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ounsworth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

dghgit Aug 27, 2026 • 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!

Choose a reason for hiding this comment

Uh oh!

ounsworth commented Aug 27, 2026

Uh oh!

dghgit commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dghgit commented Aug 27, 2026

Uh oh!

dghgit commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dghgit commented Aug 26, 2026 •

edited

Loading

dghgit Aug 27, 2026 •

edited

Loading

dghgit commented Aug 27, 2026 •

edited

Loading