SHA3 bug fixes: around partial-byte handling

alpha_0.1.3_release_notes.md

@@ -3,3 +3,25 @@
 ## Major features
 
 ## Minor features / bug fixes
+
+Bit-oriented messages:
+
+* `Hash::do_final_partial_bits()` / `do_final_partial_bits_out()` accept `num_partial_bits` in 0..=7 (0 meaning the
+  message ends on a byte boundary); larger values return `HashError::InvalidLength` instead of panicking. The convention
+  is the same for every hash family: the trailing bits are in the least significant bits of `partial_byte` (FIPS 202
+  Appendix B.1) -- see the `Hash` trait docs, including the note on the MSB-first packing used by the NIST CAVP SHA-2
+  vector files.
+
+SHA-3 / SHAKE bug fixes:
+
+* Fixed `XOF::squeeze_partial_byte_final()`: when it was the first squeeze it bypassed the SHAKE `1111` domain suffix
+  and returned raw Keccak output, and it returned the *high* rather than the low `num_bits` bits of the output byte.
+  The existing test used `0xFF`, which masked the second error.
+* Fixed `XOF::absorb_last_partial_byte()` for `num_partial_bits == 4`: the 4 message bits plus the `1111` suffix
+  exactly filled a byte and the sponge did not switch to squeezing, so the first squeeze appended the suffix a second
+  time. Every SHAKE message with a bit length of 4 mod 8 was affected. Found while building the CAVP SHA3VS harness.
+* `absorb_last_partial_byte()` and `do_final_partial_bits*()` now validate `num_partial_bits` before use; previously
+  SHA-3 accepted 8..15 and absorbed garbage, panicked for >= 16, and SHAKE rejected 0 with an error message claiming
+  `[0,7]`.
+* Interleaving absorb -> squeeze -> absorb remains rejected with `HashError::InvalidState`; the `XOF` trait docs now
+  explain why (it is the duplex construction, not SHAKE).

crypto/core/src/traits.rs

@@ -310,15 +310,31 @@ pub trait Hash: Algorithm + Default {
     fn do_final_out(self, output: &mut [u8]) -> usize;
 
     /// The same as [`Hash::do_final`], but allows for supplying a partial byte as the last input.
-    /// Assumes that the input is in the least significant bits (big endian).
+    /// The `num_partial_bits` message bits are taken from the least significant bits of
+    /// `partial_byte`, in order (bit 0 of `partial_byte` is the first message bit). This is the
+    /// FIPS 202 Appendix B.1 convention and is used uniformly for every hash family in this library,
+    /// including SHA-2, for which FIPS 180-4 defines no bit-to-byte packing. Note that the NIST CAVP
+    /// SHAVS (SHA-2) test vector files pack trailing bits MSB-first (left-justified) and must be
+    /// 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`].
     fn do_final_partial_bits(
         self,
         partial_byte: u8,
         num_partial_bits: usize,
     ) -> Result<Vec<u8>, HashError>;
 
     /// The same as [`Hash::do_final_out`], but allows for supplying a partial byte as the last input.
-    /// Assumes that the input is in the least significant bits (big endian).
+    /// The `num_partial_bits` message bits are taken from the least significant bits of
+    /// `partial_byte`, in order (bit 0 of `partial_byte` is the first message bit). This is the
+    /// FIPS 202 Appendix B.1 convention and is used uniformly for every hash family in this library,
+    /// including SHA-2, for which FIPS 180-4 defines no bit-to-byte packing. Note that the NIST CAVP
+    /// SHAVS (SHA-2) test vector files pack trailing bits MSB-first (left-justified) and must be
+    /// 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_out`]). Larger values return [`HashError::InvalidLength`].
     /// will be placed in the first [`Hash::output_len`] bytes.
     /// The entire output buffer is zeroized before the hash output is written.
     /// The return value is the number of bytes written.
@@ -1057,6 +1073,14 @@ pub trait SignatureVerifier<
 /// to break anonymity-preserving technology.
 /// Applications that require the arbitrary-length output of an XOF, but also care about these
 /// distinguishing attacks should consider adding a cryptographic salt to diversify the inputs.
+///
+/// # Absorbing after squeezing
+/// Once squeezing has begun, further calls to [`XOF::absorb`] / [`XOF::absorb_last_partial_byte`]
+/// return [`HashError::InvalidState`] and leave the object usable for further squeezing. FIPS 202
+/// 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.
 pub trait XOF: Default {
     /// A static one-shot API that digests the input data and produces `result_len` bytes of output.
     fn hash_xof(self, data: &[u8], result_len: usize) -> Vec<u8>;
@@ -1069,7 +1093,9 @@ pub trait XOF: Default {
     /// Absorb some amount of input.
     fn absorb(&mut self, data: &[u8]) -> Result<(), HashError>;
 
-    /// Switches to squeezing.
+    /// Absorbs the final `num_partial_bits` (`0..=7`, least significant bits of `partial_byte`) of the
+    /// message and switches to squeezing. 0 is valid and means the message ends on a byte boundary.
+    /// Values above 7 return [`HashError::InvalidLength`].
     fn absorb_last_partial_byte(
         &mut self,
         partial_byte: u8,
@@ -1084,8 +1110,11 @@ pub trait XOF: Default {
     /// The entire output buffer is zeroized before the output is written.
     fn squeeze_out(&mut self, output: &mut [u8]) -> usize;
 
-    /// Squeezes a partial byte from the XOF.
-    /// Output will be in the top `num_bits` bits of the returned u8 (ie Big Endian).
+    /// Squeezes a partial byte (`num_bits` in `1..=7`) from the XOF.
+    /// 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`].
     /// This is a final call and consumes self.
     fn squeeze_partial_byte_final(self, num_bits: usize) -> Result<u8, HashError>;
 

crypto/sha3/src/keccak.rs

@@ -250,12 +250,14 @@ impl KeccakInternal {
         }
     }
 
+    /// Absorbs the final `bits` (0..=7, in the least significant bits of `data`) of the message and
+    /// switches the sponge to the squeezing phase. `bits == 0` means "no further bits": the sponge is
+    /// padded and switched to squeezing without absorbing anything. Callers that have already applied a
+    /// domain-separation suffix rely on this — if the switch did not happen here, a later squeeze would
+    /// see `squeezing == false` and apply the suffix a second time.
     pub(super) fn absorb_bits(&mut self, data: u8, bits: usize) -> Result<(), HashError> {
-        if bits == 0 {
-            return Ok(());
-        }
-        if !(1..=7).contains(&bits) {
-            return Err(HashError::InvalidLength("bits must be in the range 1 to 7"));
+        if bits > 7 {
+            return Err(HashError::InvalidLength("bits must be in the range 0 to 7"));
         }
         if (self.bits_in_queue & 7) != 0 {
             return Err(HashError::InvalidState("attempt to absorb with odd length queue"));
@@ -264,11 +266,13 @@ impl KeccakInternal {
             return Err(HashError::InvalidState("attempt to absorb while squeezing"));
         }
 
-        let mask = (1 << bits) - 1;
-        self.data_queue[self.bits_in_queue >> 3] = data & mask;
+        if bits != 0 {
+            let mask = (1 << bits) - 1;
+            self.data_queue[self.bits_in_queue >> 3] = data & mask;
 
-        // NOTE: After this, bits_in_queue is no longer a multiple of 8, so no more absorbs will work
-        self.bits_in_queue += bits;
+            // NOTE: After this, bits_in_queue is no longer a multiple of 8, so no more absorbs will work
+            self.bits_in_queue += bits;
+        }
         self.pad_and_switch_to_squeezing_phase();
         Ok(())
     }
@@ -514,6 +518,27 @@ mod keccak_tests {
         println!("n2: {:x?}", &out);
     }
 
+    /// absorb_bits(): 0..=7 bits are accepted and always switch the sponge to squeezing (0 bits
+    /// included — see the doc comment); 8+ bits are rejected; a second call is rejected as squeezing.
+    #[test]
+    fn absorb_bits_range_and_phase() {
+        for bits in 0..=7usize {
+            let mut d = KeccakInternal::new(KeccakSize::_256);
+            d.absorb(b"abc");
+            d.absorb_bits(0xFF, bits).unwrap();
+            assert!(d.squeezing, "bits={bits}: must switch to squeezing");
+            assert!(matches!(d.absorb_bits(0, 1), Err(HashError::InvalidState(_))));
+        }
+        for bits in [8usize, 9, 16, usize::MAX] {
+            let mut d = KeccakInternal::new(KeccakSize::_256);
+            assert!(
+                matches!(d.absorb_bits(0, bits), Err(HashError::InvalidLength(_))),
+                "bits={bits}"
+            );
+            assert!(!d.squeezing, "rejected call must not change phase");
+        }
+    }
+
     /// Regression test for from_serialized_state's validation of a not-yet-squeezing queue: a corrupt
     /// state whose bits_in_queue is not byte-aligned, or equals/exceeds the rate, must be rejected as
     /// InvalidData rather than deserialized into a value that later trips the debug_assert in absorb()

crypto/sha3/src/sha3.rs

@@ -208,6 +208,10 @@ impl<PARAMS: SHA3Params> Hash for SHA3Internal<PARAMS> {
         num_partial_bits: usize,
         output: &mut [u8],
     ) -> Result<usize, HashError> {
+        // A partial byte has at most 7 bits; 0 means the message ends on a byte boundary.
+        if num_partial_bits > 7 {
+            return Err(HashError::InvalidLength("num_partial_bits must be in the range [0,7]"));
+        }
         output.fill(0);
 
         // Mutants note: This is just bit-setting into empty space.

crypto/sha3/src/shake.rs

@@ -304,8 +304,9 @@ impl<PARAMS: SHAKEParams> XOF for SHAKEInternal<PARAMS> {
         if self.keccak.squeezing {
             return Err(HashError::InvalidState("cannot absorb after squeezing has begun"));
         }
-        if !(1..=7).contains(&num_partial_bits) {
-            return Err(HashError::InvalidLength("must be in the range [0,7]"));
+        // A partial byte has at most 7 bits; 0 means the message ends on a byte boundary.
+        if num_partial_bits > 7 {
+            return Err(HashError::InvalidLength("num_partial_bits must be in the range [0,7]"));
         }
         // Mutants note: This is just bit-setting into empty space.
         // It works the same regardless of whether it's OR or XOR.
@@ -355,14 +356,16 @@ impl<PARAMS: SHAKEParams> XOF for SHAKEInternal<PARAMS> {
         output: &mut u8,
     ) -> 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]"));
         }
 
         *output = 0;
 
+        // Via squeeze_out() so the SHAKE "1111" suffix (FIPS 202 s. 6.2) is applied on a first squeeze.
         let mut buf = [0u8; 1];
-        self.keccak.squeeze(&mut buf);
-        *output = buf[0] >> 8 - num_bits;
+        self.squeeze_out(&mut buf);
+
+        *output = buf[0] & ((1u8 << num_bits) - 1);
         Ok(())
     }
 

crypto/sha3/tests/sha3_tests.rs

@@ -1,6 +1,7 @@
 #[cfg(test)]
 mod sha3_tests {
     use super::sha3_test_helpers::*;
+    use bouncycastle_core::errors::HashError;
     use bouncycastle_core::key_material;
     use bouncycastle_core::key_material::{
         KeyMaterial, KeyMaterial256, KeyMaterial512, KeyMaterialTrait, KeyType,
@@ -142,6 +143,28 @@ mod sha3_tests {
         assert_eq!(output, expected_output[..SHA3_224::OUTPUT_LEN - 1]);
     }
 
+    /// do_final_partial_bits() must validate num_partial_bits before shifting: 0 is equivalent to
+    /// do_final(), 8+ is rejected with InvalidLength rather than panicking (16+ used to overflow a shift).
+    #[test]
+    fn partial_bits_range_is_validated() {
+        for bad in [8usize, 9, 15, 16, 64, usize::MAX] {
+            let mut h = SHA3_256::new();
+            h.do_update(b"abc");
+            assert!(
+                matches!(h.do_final_partial_bits(0xFF, bad), Err(HashError::InvalidLength(_))),
+                "num_partial_bits={bad}"
+            );
+            let mut out = [0u8; 32];
+            assert!(matches!(
+                SHA3_256::new().do_final_partial_bits_out(0xFF, bad, &mut out),
+                Err(HashError::InvalidLength(_))
+            ));
+        }
+        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"));
+    }
+
     #[test]
     fn test_do_final_out_truncation() {
         let expected_output =  b"\xFE\x51\xC5\xD7\x62\x48\xE1\xE9\xD3\x01\x29\x6A\xE8\xAB\x94\x69\xD2\x86\x34\xB4\xAD\x3E\x9E\x78\xC8\xB0\x9D\x47";

crypto/sha3/tests/shake_tests.rs

@@ -3,6 +3,7 @@ extern crate core;
 #[cfg(test)]
 mod shake_tests {
     use super::shake_test_helpers::*;
+    use bouncycastle_core::errors::HashError;
     use bouncycastle_core::key_material::{
         KeyMaterial, KeyMaterial256, KeyMaterial512, KeyMaterialTrait, KeyType,
     };
@@ -62,6 +63,77 @@ mod shake_tests {
         assert_eq!(out, 0x01);
     }
 
+    /// Regression: squeeze_partial_byte_final() as the *first* squeeze must apply the SHAKE "1111"
+    /// domain suffix (previously it bypassed it and returned raw Keccak output), and must return the
+    /// low `num_bits` bits of the next output byte (FIPS 202 B.1 bit ordering), zero-extended.
+    #[test]
+    fn partial_bit_output_as_first_squeeze_matches_full_output() {
+        let msg = b"abc";
+        for skip in [0usize, 1, 5] {
+            let mut shake = SHAKE256::new();
+            shake.absorb(msg).unwrap();
+            let full = shake.squeeze(skip + 1)[skip];
+            // pick a byte that is not all-ones/all-zeros so bit selection is actually tested
+            assert!(
+                full != 0x00 && full != 0xFF,
+                "test vector byte must be non-uniform: {full:#x}"
+            );
+
+            for n in 1..=7usize {
+                let mut shake = SHAKE256::new();
+                shake.absorb(msg).unwrap();
+                if skip > 0 {
+                    _ = shake.squeeze(skip);
+                }
+                let got = shake.squeeze_partial_byte_final(n).unwrap();
+                assert_eq!(got, full & ((1u8 << n) - 1), "skip={skip} n={n}");
+                assert_eq!(got >> n, 0, "high bits must be zero");
+            }
+        }
+    }
+
+    /// Regression: when the 4 trailing message bits plus the SHAKE "1111" suffix exactly fill a byte,
+    /// the sponge must still switch to squeezing, otherwise the first squeeze appended a second suffix.
+    /// Vector: NIST CAVP SHA3VS SHAKE128ShortMsg (bit-oriented), Len = 4, Msg = 08.
+    #[test]
+    fn absorb_last_partial_byte_four_bits() {
+        let mut shake = SHAKE128::new();
+        shake.absorb_last_partial_byte(0x08, 4).unwrap();
+        assert_eq!(
+            shake.squeeze(16),
+            bouncycastle_hex::decode("d40238024b040a954d9c2c89daf480e5").unwrap(),
+            "SHAKE128 of the 4-bit message 0001"
+        );
+    }
+
+    /// absorb_last_partial_byte() must validate num_partial_bits before shifting: 0 is allowed
+    /// (finalize with no partial byte), 8+ is rejected with InvalidLength rather than panicking.
+    #[test]
+    fn absorb_last_partial_byte_validates_range() {
+        for bad in [8usize, 9, 15, 16, 64, usize::MAX] {
+            let mut shake = SHAKE128::new();
+            shake.absorb(b"abc").unwrap();
+            assert!(
+                matches!(
+                    shake.absorb_last_partial_byte(0xFF, bad),
+                    Err(HashError::InvalidLength(_))
+                ),
+                "num_partial_bits={bad}"
+            );
+        }
+        let mut a = SHAKE128::new();
+        a.absorb(b"abc").unwrap();
+        a.absorb_last_partial_byte(0xFF, 0).unwrap();
+        assert_eq!(a.squeeze(32), SHAKE128::new().hash_xof(b"abc", 32));
+
+        // Upper boundary: 7 bits is the largest valid partial byte and must be accepted, and must
+        // actually change the output relative to the byte-aligned message.
+        let mut b = SHAKE128::new();
+        b.absorb(b"abc").unwrap();
+        b.absorb_last_partial_byte(0x7F, 7).unwrap();
+        assert_ne!(b.squeeze(32), SHAKE128::new().hash_xof(b"abc", 32));
+    }
+
     /// Once squeezing has begun, a SHAKE cannot return to absorbing (FIPS 202 defines SHAKE as a
     /// single function of the whole message). Both absorb entry points must reject a post-squeeze call
     /// with `HashError::InvalidState` rather than panicking, and a rejected call must leave the sponge