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.

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

ounsworth · 2026-09-01T17:15:24Z

+    /// shifted right by `8 - num_partial_bits` before being passed here; the SHA3VS files already use
+    /// the LSB convention.
+    /// `num_partial_bits` must be in `0..=7`; 0 is valid and means the message ends on a byte
+    /// boundary (equivalent to [`Hash::do_final`]). Larger values return [`HashError::InvalidLength`].


This is implementation detail that, IMO, does not belong on an abstract trait.

Also, this seems to be saying that we are going to force the SHA3 behaviour on SHA2 even though the SHA2 CAVP vectors want SHA2 to behave a different way.

I think we have a few design options here:

Define a standard behaviour so that there is consistency across all hash functions (although maybe not with other libraries).

Say here that the details of which bits are taken (and which order they are taken in), is up to the underlying hash function implementation. That means that implementations of this function will have different behaviours between different hash functions, but at least each of those would be consistent with their specs / test vectors, and hopefully with implementations in other libraries.
I am going to replace this with a "See implementing hash primitive for details", and make sure this comment is on SHA3 / SHA2 instead.

Maybe this is actually an indication that partial bit inputs / outputs are not generic to all hash functions -- not all hash functions will implement them, and the ones that do may implement it differently -- so the right answer is to remove the partial_bits functions from this trait and instead impl them as "bells & whistles" functions directly on the SHA2 and SHA3 structs.

@dghgit and @npajkovsky I would value your opinions. I think I'm leaning towards #3

CAVP test vectors have no relationship to real-world use, they're just for testing. The issue here really is that SHA-2 does not define a method of absorbing last bits (as in a format), SHA-3 on the other hand does explicitly say least significant bit first in big endian order (or at least it appears to FIPS PUB 202 Appendix B.1 algorithm 10, see step 4 in particular). So the approach taken is to make SHA-3's explicit definition the default. Realistically the only thing that matters is it is consistent, I can't really comment on whether the Appendix B.1 ordering is more common or not than providing a byte where the extra bits are the high end and absorbed in the reverse order.

So 3 is not correct. I don't think I've seen a function yet which does not support partial bytes, but this is the first time I think I've seen anyone implement it - if you want to be consistent with most libraries, you'd just drop it. My initial inclination was actually to suggest that the functionality was removed, but I kind of warmed to it and using partial bytes is the kind of thing that you encounter in constrained environments, so I'd suggest we just keep implementing it. Providing support is not really that hard (at least anymore).

Ok, David set me straight here.

This is essentially greenfield; the FIPSs say "accepts a message of d bits", how we design an API for that is entirely our prerogative. In fact, the only wrong choice here is to make it different for each hash function.

I'll keep the current behaviour, but adjust the docstring comment so that it describes the behaviour without referencing SHA2 / SHA3. Keep the bit that "unimplemented!" is valid (although maybe that needs to be an actual runtime error so that it passes through the factory properly?)

Also, if this is going to be generic behaviour that we expect on all hash functions, then I'll add some sort of test to core-test-framework::hash; maybe the only real thing that I can test is left vs right bit ordering, which I can test the negative: if you set num_bits = 4, then you're free to twiddle the half you don't care about without changing the resulting hash output.

ounsworth · 2026-09-01T18:33:09Z

+/// defines SHAKE as a function of a single, complete message; the sponge's absorb/squeeze phases are
+/// internal to computing it. Interleaving absorb → squeeze → absorb → squeeze is the *duplex*
+/// construction, which is a different (unapproved) primitive whose output is not the SHAKE of any
+/// message and is not reproducible by other SHAKE implementations, so it is deliberately rejected.


This is a docstring on core::traits::XOF, which is supposed to be agnostic to which algorithm it's sitting on top of, so it's not appropriate to be talking about SHAKE here. This implementation detail about SHAKE should be moved into the SHAKE crate. The comment here

I suppose this comment should say something about absorb-after-squeeze. Is there an abstract mathematical definition of an XOF (ie not tied to SHA3), and in the abstract is absorb-after-squeeze allowed? I think in general it is -- you're perfectly allowed to do that on raw KECCAK, for example, and we could check the specs for other sponge constructions like ASCON-hash or other things from the CEASAR competition.

I'm also not sure that this description is even true for SHAKE.. I certainly agree that absorb-after-squeeze is not the way that SHAKE is presented in FIPS 202, but I'm not convinced that it's actually forbidden. If it's not, then we should support it because it's a potentially useful feature.

Sure. Feel free to edit. absorb-after-squeeze is most definitely forbidden, it's not allowed, and the definition of the function is quite explicit in that respect. We could do something around the original Keccak construction if people are really excited about this, but given that it is already not allowed in FIPS, unless someone can provide analysis suggesting otherwise, I would suggest doing it will put us in CVE territory - section 6.3 of FIPS PUB 202 is quite explicit that the padding is to ensure domain separation, allowing injection of extra bits appears to be a clear violation of that and I suspect third party reviewers will react accordingly.

ounsworth · 2026-09-01T18:34:06Z

+    /// The bits are returned in the least significant `num_bits` bits of the returned u8, with the
+    /// remaining high bits zero. This follows the FIPS 202 Appendix B.1 bit-string convention
+    /// (the first bit of a byte is its least significant bit) and matches the input convention of
+    /// [`XOF::absorb_last_partial_byte`].


Related to the above discussion on partial_bit stuff -- whatever we do for the partial byte absorb should also be done here, which likely means moving this comment onto the implementation of this function in shake.rs.

ounsworth · 2026-09-01T18:55:13Z

    ) -> Result<(), HashError> {
        if !(1..=7).contains(&num_bits) {
-            return Err(HashError::InvalidLength("must be in the range [0,7]"));
+            return Err(HashError::InvalidLength("num_bits must be in the range [1,7]"));


This is currently mismatched with absorb_last_partial_byte above since that one allows num_partial_bits = 0 but here num_bits = 0 results in an error.

Which way makes more sense? I think I lean towards making 0 allowed in both cases (in which case this behaves the same as calling the equivalent whole-byte function ... could add a unit test for that.

I suspect that either way is compliant with FIPS 202 since it only requires that you can squeeze d bits but does not explicitly specify this API.

We should also align the parameter names of the absorb and squeeze partial byte functions to both be num_bits.

Zero allowed is fine with me. It would certainly be better if it was consistent.

ounsworth · 2026-09-01T18:58:59Z

+        let mut h = SHA3_256::new();
+        h.do_update(b"abc");
+        assert_eq!(h.do_final_partial_bits(0xFF, 0).unwrap(), SHA3_256::new().hash(b"abc"));
+    }


Add a test that calling do_final_partial_bits with num_bits = 0 behaves the same is simply calling do_final.

Agreed. This isn't my PR though now.

ounsworth · 2026-09-01T18:59:44Z

+        b.absorb_last_partial_byte(0x7F, 7).unwrap();
+        assert_ne!(b.squeeze(32), SHAKE128::new().hash_xof(b"abc", 32));
+    }
+


Depending on the resolution to the above discussion about how to handle num_bits = 0, we probably need to add tests for that.

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

ounsworth closed this Sep 1, 2026

ounsworth reopened this Sep 1, 2026

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

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

Uh oh!

ounsworth commented Aug 28, 2026

Uh oh!

ounsworth Sep 1, 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!

dghgit Sep 1, 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!

Choose a reason for hiding this comment

Uh oh!

dghgit Sep 1, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ounsworth commented Aug 27, 2026 •

edited

Loading

ounsworth Sep 1, 2026 •

edited

Loading

dghgit Sep 1, 2026 •

edited

Loading

dghgit Sep 1, 2026 •

edited

Loading