diff --git a/alpha_0.1.3_release_notes.md b/alpha_0.1.3_release_notes.md index 210a5aeb..52d9c1f1 100644 --- a/alpha_0.1.3_release_notes.md +++ b/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). diff --git a/crypto/core/src/traits.rs b/crypto/core/src/traits.rs index 7e23d516..356ab65b 100644 --- a/crypto/core/src/traits.rs +++ b/crypto/core/src/traits.rs @@ -310,7 +310,15 @@ 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, @@ -318,7 +326,15 @@ pub trait Hash: Algorithm + Default { ) -> Result, 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; @@ -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; diff --git a/crypto/sha3/src/keccak.rs b/crypto/sha3/src/keccak.rs index 10f85f44..e0a18db2 100644 --- a/crypto/sha3/src/keccak.rs +++ b/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() diff --git a/crypto/sha3/src/sha3.rs b/crypto/sha3/src/sha3.rs index 3da20b0b..45c57b2c 100644 --- a/crypto/sha3/src/sha3.rs +++ b/crypto/sha3/src/sha3.rs @@ -208,6 +208,10 @@ impl Hash for SHA3Internal { num_partial_bits: usize, output: &mut [u8], ) -> Result { + // 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. diff --git a/crypto/sha3/src/shake.rs b/crypto/sha3/src/shake.rs index 6ba2a882..2661d2c0 100644 --- a/crypto/sha3/src/shake.rs +++ b/crypto/sha3/src/shake.rs @@ -304,8 +304,9 @@ impl XOF for SHAKEInternal { 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 XOF for SHAKEInternal { 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(()) } diff --git a/crypto/sha3/tests/sha3_tests.rs b/crypto/sha3/tests/sha3_tests.rs index 306d8816..55ddf6c8 100644 --- a/crypto/sha3/tests/sha3_tests.rs +++ b/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"; diff --git a/crypto/sha3/tests/shake_tests.rs b/crypto/sha3/tests/shake_tests.rs index 260e6e1d..aa231ab0 100644 --- a/crypto/sha3/tests/shake_tests.rs +++ b/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