SHA3 bug fixes: around partial-byte handling by ounsworth · Pull Request #91 · bcgit/bc-rust

ounsworth · 2026-08-27T22:58:32Z

Break up of #87, so this is actually dgh's submission. So I will be reviewing / approving.

XOF::squeeze_partial_byte_final() squeezed raw Keccak, skipping the SHAKE 1111 suffix (FIPS 202 s. 6.2) when it was the first squeeze, and returned the high rather than the low num_bits bits. Now goes via squeeze_out() and masks the low bits. The old test's output byte happened to be 0xFF, which hid the high/low error.
KeccakInternal::absorb_bits() returned early for bits == 0 without switching to squeezing, so a suffix already folded into a whole byte was applied a second time. Broke every SHAKE message of bit length 4 mod 8 (6 mod 8 for SHA-3). Now accepts 0..=7 and always switches phase.
num_partial_bits was unvalidated before use as a shift amount: SHA-3 absorbed garbage for 8..15 and panicked at

= 16; SHAKE rejected 0. Both now accept 0..=7 (0 meaning the message ends on a byte boundary) and return
HashError::InvalidLength otherwise.
Hash / XOF docs: state the FIPS 202 Appendix B.1 bit ordering, note the opposite MSB-first packing in the CAVP SHAVS (SHA-2) vector files, and explain why absorb-after-squeeze is rejected (duplex, not SHAKE).
Regression tests for each fix, including the 4-bit SHAKE128 vector from the CAVP SHA3VS bit-oriented set.

… 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 bcgit#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 bcgit#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 bcgit#87 review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…artial-bit validation * `XOF::squeeze_partial_byte_final()` squeezed raw Keccak, skipping the SHAKE `1111` suffix (FIPS 202 s. 6.2) when it was the first squeeze, and returned the high rather than the low `num_bits` bits. Now goes via `squeeze_out()` and masks the low bits. The old test's output byte happened to be `0xFF`, which hid the high/low error. * `KeccakInternal::absorb_bits()` returned early for `bits == 0` without switching to squeezing, so a suffix already folded into a whole byte was applied a second time. Broke every SHAKE message of bit length 4 mod 8 (6 mod 8 for SHA-3). Now accepts 0..=7 and always switches phase. * `num_partial_bits` was unvalidated before use as a shift amount: SHA-3 absorbed garbage for 8..15 and panicked at >= 16; SHAKE rejected 0. Both now accept 0..=7 (0 meaning the message ends on a byte boundary) and return `HashError::InvalidLength` otherwise. * `Hash` / `XOF` docs: state the FIPS 202 Appendix B.1 bit ordering, note the opposite MSB-first packing in the CAVP SHAVS (SHA-2) vector files, and explain why absorb-after-squeeze is rejected (duplex, not SHAKE). * Regression tests for each fix, including the 4-bit SHAKE128 vector from the CAVP SHA3VS bit-oriented set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ounsworth · 2026-08-27T23:04:18Z

Review Tasks (for me):

Review the core bug, fix, and new tests.
Review why this bug fix is accompanied by so many lines of additional docs (esp. to core::traits)
Cut down the release_note to a single line.
Other review points as I see them.

I plan to just make changes directly to the branch rather than use github's review tool.

ounsworth · 2026-08-28T00:49:25Z

dhg says:

looks okay, with the comments, you can get it to abbreviate 2 blocks are clearly largely memory annotation. With 1076 I asked it put that one there because of the question about sticking in extra data, delete if you want is fine, it's not really necessary as the code doesn't allow the extra data, it's probably worth putting somewhere though as this has come up twice now, and we're both likely to get sick of explaining it to people.

…artial-bit validation * `XOF::squeeze_partial_byte_final()` squeezed raw Keccak, skipping the SHAKE `1111` suffix (FIPS 202 s. 6.2) when it was the first squeeze, and returned the high rather than the low `num_bits` bits. Now goes via `squeeze_out()` and masks the low bits. The old test's output byte happened to be `0xFF`, which hid the high/low error. * `KeccakInternal::absorb_bits()` returned early for `bits == 0` without switching to squeezing, so a suffix already folded into a whole byte was applied a second time. Broke every SHAKE message of bit length 4 mod 8 (6 mod 8 for SHA-3). Now accepts 0..=7 and always switches phase. * `num_partial_bits` was unvalidated before use as a shift amount: SHA-3 absorbed garbage for 8..15 and panicked at >= 16; SHAKE rejected 0. Both now accept 0..=7 (0 meaning the message ends on a byte boundary) and return `HashError::InvalidLength` otherwise. * `Hash` / `XOF` docs: state the FIPS 202 Appendix B.1 bit ordering, note the opposite MSB-first packing in the CAVP SHAVS (SHA-2) vector files, and explain why absorb-after-squeeze is rejected (duplex, not SHAKE). * Regression tests for each fix, including the 4-bit SHAKE128 vector from the CAVP SHA3VS bit-oriented set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> # Conflicts: # alpha_0.1.3_release_notes.md # crypto/sha3/src/sha3.rs

…-rust into fix/sha3-shake-partial-bits

ounsworth · 2026-09-02T05:50:19Z

I fixed the things that I noticed during my review, plus a few more that I / Claude found in the process.

ounsworth · 2026-09-02T05:53:53Z

Oh shoot. I rebased it on top of release/0.1.3alpha. Once the aes stuff gets rebased back out, all the commits on this branch will need to get cherry-picked back out.

ounsworth · 2026-09-02T06:11:15Z

Merged manually in 2f8d5a7

…offsets A cargo-mutants run over the sha3 crate after #91 left 11 survivors. Two are the documented OR/XOR equivalents in the partial-bit suffix code; the other nine are all offset or length arithmetic in the Suspendable serialization in keccak.rs: swapping or aliasing the two 1-byte KDF metadata slots (offsets 402 and 403), or enlarging KECCAK_SERIALIZED_LEN / SHA3_FAMILY_STATE_LEN, went unnoticed. The root cause is structural: the KDF entry points are one-shot and consume self, so via the public API the kdf_key_type / kdf_security_strength / kdf_entropy fields are always at their defaults (all zero) whenever a state is suspended. The integration round-trip tests can therefore never tell the slots apart, and an oversized buffer round-trips because trailing zeros are ignored. Add two tests to the keccak.rs test module: - serialized_state_lengths_are_pinned asserts the three length constants (401 / 412 / 415), which are wire-format constants in the public Suspendable contract. - sha3_family_state_round_trips_kdf_metadata drives the crate-private serialize / deserialize helpers directly with distinct non-zero values for the tag, key type, security strength and entropy, checking each byte offset and the round trip. With these, re-running mutants over the serialization region gives 0 missed (73 mutants: 64 caught, 9 unviable). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

dghgit and others added 10 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 bcgit#87)

8496d7e

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

This was referenced Aug 27, 2026

Add NIST CAVP tests to the SHA3 crate #92

Open

Refactor/sha3 code structure #93

Open

dghgit approved these changes Aug 28, 2026

View reviewed changes

npajkovsky approved these changes Aug 28, 2026

View reviewed changes

shortened the release notes

ec56486

ounsworth commented Sep 1, 2026

View reviewed changes

Comment thread crypto/core/src/traits.rs Outdated

Comment thread crypto/core/src/traits.rs Outdated

Comment thread crypto/core/src/traits.rs

Comment thread crypto/sha3/src/shake.rs Outdated

Comment thread crypto/sha3/tests/sha3_tests.rs

Comment thread crypto/sha3/tests/shake_tests.rs

ounsworth closed this Sep 1, 2026

ounsworth reopened this Sep 1, 2026

ounsworth marked this pull request as draft September 1, 2026 19:05

some work on the sha3 partial byte bug pr

f5b8c3d

dghgit force-pushed the release/0.1.3alpha branch from bea169a to c026339 Compare September 2, 2026 04:45

ounsworth added 3 commits September 2, 2026 00:28

tightened up absorb-after-squeeze behaviour for shake

0666366

tightened up absorb-after-squeeze behaviour for shake

2a517e6

SHAKE::squeeze_partial_byte now works on num_bits=0

bad45f9

ounsworth marked this pull request as ready for review September 2, 2026 05:37

ounsworth added 3 commits September 2, 2026 00:38

SHAKE::squeeze_partial_byte now works on num_bits=0

90690de

shortened the release notes

9c4b16a

ounsworth added 6 commits September 2, 2026 00:42

some work on the sha3 partial byte bug pr

7903d42

tightened up absorb-after-squeeze behaviour for shake

225e9d0

tightened up absorb-after-squeeze behaviour for shake

a265fd6

SHAKE::squeeze_partial_byte now works on num_bits=0

f7a82ad

SHAKE::squeeze_partial_byte now works on num_bits=0

dbcb020

Merge branch 'fix/sha3-shake-partial-bits' of github.com:ounsworth/bc…

04d1e45

…-rust into fix/sha3-shake-partial-bits

hubot pushed a commit that referenced this pull request Sep 2, 2026

SHA3 bug fixes: around partial-byte handling - #91

2f8d5a7

ounsworth closed this Sep 2, 2026

ounsworth mentioned this pull request Sep 2, 2026

Feature/sha3 final patches #87

Closed

Navigation Menu

Uh oh!

SHA3 bug fixes: around partial-byte handling - #91

SHA3 bug fixes: around partial-byte handling#91
ounsworth wants to merge 24 commits into
bcgit:release/0.1.3alphafrom
ounsworth:fix/sha3-shake-partial-bits

ounsworth commented Aug 27, 2026 •

edited

Loading

Uh oh!

ounsworth commented Aug 27, 2026 •

edited

Loading

Uh oh!

ounsworth commented Aug 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ounsworth commented Sep 2, 2026

Uh oh!

ounsworth commented Sep 2, 2026

Uh oh!

ounsworth commented Sep 2, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

ounsworth commented Aug 28, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ounsworth commented Sep 2, 2026

Uh oh!

ounsworth commented Sep 2, 2026

Uh oh!

ounsworth commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ounsworth commented Aug 27, 2026 •

edited

Loading

ounsworth commented Aug 27, 2026 •

edited

Loading

ounsworth commented Sep 2, 2026 •

edited

Loading