Skip to content

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

Open
ounsworth wants to merge 1 commit into
bcgit:release/0.1.3alphafrom
ounsworth:fix/sha3-shake-partial-bits
Open

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

Conversation

@ounsworth

@ounsworth ounsworth commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants