Refactor/sha3 code structure

alpha_0.1.3_release_notes.md

@@ -3,3 +3,33 @@
 ## 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).
+
+SHA-3 internals:
+
+* SHA-3 finalization is now a single private `do_final_bits_out()` shared by `do_final_out()` and
+  `do_final_partial_bits_out()`, so the domain-separation suffix, padding and output truncation are applied in exactly
+  one place.
+* `HashAlgParams` for the SHA-3 types is now forwarded from the `*Params` structs, so `OUTPUT_LEN` / `BLOCK_LEN` are
+  defined once. Removed misleading leftover SHA-2 block-size comments.

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/lib.rs

@@ -89,7 +89,7 @@
 //! [`KDF`] acts on [`KeyMaterial`] objects as both the input and output values.
 //! In the case of SHA3, the [`KDF`] interfaces are simple wrapper functions around the underlying SHA3 or SHAKE
 //! primitive that correctly maintains the length and entropy metadata of the key material that it is acting on.
-//! This is intended to act as a developer ait to prevent  some classes of developer mistakes, such as
+//! This is intended to act as a developer aid to prevent some classes of developer mistakes, such as
 //! deriving a cryptographic key from uninitialized (aka zeroized) input key material, or using low-entropy
 //! input key material to derive a MAC, symmetric, or asymmetric key.
 //!
@@ -160,17 +160,17 @@ mod sha3;
 mod shake;
 
 /*** String constants ***/
-///
+/// Algorithm name string for SHA3-224, as used by the factories and CLI.
 pub const SHA3_224_NAME: &str = "SHA3-224";
-///
+/// Algorithm name string for SHA3-256, as used by the factories and CLI.
 pub const SHA3_256_NAME: &str = "SHA3-256";
-///
+/// Algorithm name string for SHA3-384, as used by the factories and CLI.
 pub const SHA3_384_NAME: &str = "SHA3-384";
-///
+/// Algorithm name string for SHA3-512, as used by the factories and CLI.
 pub const SHA3_512_NAME: &str = "SHA3-512";
-///
+/// Algorithm name string for SHAKE128, as used by the factories and CLI.
 pub const SHAKE128_NAME: &str = "SHAKE128";
-///
+/// Algorithm name string for SHAKE256, as used by the factories and CLI.
 pub const SHAKE256_NAME: &str = "SHAKE256";
 
 /*** pub types ***/
@@ -205,11 +205,13 @@ trait SHA3Params: HashAlgParams {
 
 // TODO: it would probably be more elegant to macro these.
 
-impl HashAlgParams for SHA3_224 {
-    const OUTPUT_LEN: usize = 28;
-    // const BLOCK_LEN: usize = 64;
-    const BLOCK_LEN: usize = 144; // FIPS 202 Table 3
+/// The public hash types expose the same parameters as their `*Params` marker, so the constants
+/// are defined exactly once (on the params struct) and forwarded here.
+impl<PARAMS: SHA3Params> HashAlgParams for SHA3Internal<PARAMS> {
+    const OUTPUT_LEN: usize = PARAMS::OUTPUT_LEN;
+    const BLOCK_LEN: usize = PARAMS::BLOCK_LEN;
 }
+
 /// The parameters for SHA3_224.
 #[derive(Clone)]
 pub struct SHA3_224Params;
@@ -219,7 +221,6 @@ impl Algorithm for SHA3_224Params {
 }
 impl HashAlgParams for SHA3_224Params {
     const OUTPUT_LEN: usize = 28;
-    // const BLOCK_LEN: usize = 64;
     const BLOCK_LEN: usize = 144; // FIPS 202 Table 3
 }
 impl SHA3Params for SHA3_224Params {
@@ -233,11 +234,6 @@ impl AlgorithmOID for SHA3_224 {
         &[0x06, 0x09, 0x60, 0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x07];
 }
 
-impl HashAlgParams for SHA3_256 {
-    const OUTPUT_LEN: usize = 32;
-    // const BLOCK_LEN: usize = 64;
-    const BLOCK_LEN: usize = 136; // FIPS 202 Table 3
-}
 /// The parameters for SHA3_256.
 #[derive(Clone)]
 pub struct SHA3_256Params;
@@ -247,7 +243,6 @@ impl Algorithm for SHA3_256Params {
 }
 impl HashAlgParams for SHA3_256Params {
     const OUTPUT_LEN: usize = 32;
-    // const BLOCK_LEN: usize = 64;
     const BLOCK_LEN: usize = 136; // FIPS 202 Table 3
 }
 impl SHA3Params for SHA3_256Params {
@@ -263,18 +258,12 @@ impl AlgorithmOID for SHA3_256 {
 /// The parameters for SHA3_384.
 #[derive(Clone)]
 pub struct SHA3_384Params;
-impl HashAlgParams for SHA3_384 {
-    const OUTPUT_LEN: usize = 48;
-    // const BLOCK_LEN: usize = 128;
-    const BLOCK_LEN: usize = 104; // FIPS 202 Table 3
-}
 impl Algorithm for SHA3_384Params {
     const ALG_NAME: &'static str = SHA3_384_NAME;
     const MAX_SECURITY_STRENGTH: SecurityStrength = SecurityStrength::_192bit;
 }
 impl HashAlgParams for SHA3_384Params {
     const OUTPUT_LEN: usize = 48;
-    // const BLOCK_LEN: usize = 128;
     const BLOCK_LEN: usize = 104; // FIPS 202 Table 3
 }
 impl SHA3Params for SHA3_384Params {
@@ -290,18 +279,12 @@ impl AlgorithmOID for SHA3_384 {
 /// The parameters for SHA3_512.
 #[derive(Clone)]
 pub struct SHA3_512Params;
-impl HashAlgParams for SHA3_512 {
-    const OUTPUT_LEN: usize = 64;
-    // const BLOCK_LEN: usize = 128;
-    const BLOCK_LEN: usize = 72; // FIPS 202 Table 3
-}
 impl Algorithm for SHA3_512Params {
     const ALG_NAME: &'static str = SHA3_512_NAME;
     const MAX_SECURITY_STRENGTH: SecurityStrength = SecurityStrength::_256bit;
 }
 impl HashAlgParams for SHA3_512Params {
     const OUTPUT_LEN: usize = 64;
-    // const BLOCK_LEN: usize = 128;
     const BLOCK_LEN: usize = 72; // FIPS 202 Table 3
 }
 impl SHA3Params for SHA3_512Params {