Speedups and doc fixes

Cargo.toml

@@ -35,7 +35,7 @@ hex = "0.4"
 rand = "0.10"
 serde_json = "1"
 
-# Criteron benches
+# Criterion benches
 [[bench]]
 name = "benches"
 harness = false

README.md

@@ -67,7 +67,7 @@ Default features flags: _none_
 
 Feature flag list:
 
-* `serialize_secret_state` — Implements `serde`'s `Serialize` and `Deserialize` traits for the `Strobe` struct. **SECURITY NOTE**: Serializing Strobe state outputs security sensitive data that MUST be kept private. Treat the data as you would a private encryption/decryption key.
+* `serialize_secret_state` — Implements `serde`'s `Serialize` and `Deserialize` traits for the `Strobe` struct. ⚠️Security warning⚠️: Do NOT use this if you don't know what you're doing. Serializing Strobe state outputs security-sensitive data that MUST be kept private. Treat the data as you would a private encryption/decryption key.
 * `kat` — Required for running known-answer tests. Use only when testing.
 
 For info on how to omit or include feature flags, see the [cargo docs on features](https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#choosing-features).
@@ -100,7 +100,7 @@ To benchmark, run
 cargo bench
 ```
 
-This will produce a summary with plots in `target/crieteron/report/index.html`. These won't be very interesting, since almost every function in STROBE has the same runtime.
+This will produce a summary with plots in `target/criterion/report/index.html`. These won't be very interesting, since almost every function in STROBE has the same runtime.
 
 License
 -------

benches/benches.rs

@@ -10,6 +10,10 @@ fn bench_nonmeta(c: &mut Criterion) {
 
     let mut s = Strobe::new(b"simplebench", SecParam::B256);
     let mut v = [0u8; 256];
+    let mut big_v = [0u8; 8192];
+    g.bench_function("8KiB send_enc", |b| {
+        b.iter(|| s.send_enc(&mut big_v, false))
+    });
     g.bench_function("send_enc", |b| b.iter(|| s.send_enc(&mut v, false)));
     g.bench_function("recv_enc", |b| b.iter(|| s.recv_enc(&mut v, false)));
     g.bench_function("send_clr", |b| b.iter(|| s.send_clr(&v, false)));

src/basic_tests.rs

@@ -20,7 +20,7 @@ s = Strobe("", security=128)
 print("[{}]".format(', '.join(map("0x{:02x}".format, s.st))))
 */
 #[test]
-fn test_init_128() {
+fn init_128() {
     let s = Strobe::new(b"", SecParam::B128);
     let initial_st = s.st.0;
     let expected_st: &[u8; 8 * KECCAK_BLOCK_SIZE] = &[
@@ -52,7 +52,7 @@ s = Strobe("", security=256)
 print("[{}]".format(', '.join(map("0x{:02x}".format, s.st))))
 */
 #[test]
-fn test_init_256() {
+fn init_256() {
     let s = Strobe::new(b"", SecParam::B256);
     let initial_st = s.st.0;
     let expected_st: &[u8; 8 * KECCAK_BLOCK_SIZE] = &[
@@ -98,7 +98,7 @@ print("[{}]".format(', '.join(map("0x{:02x}".format, s.st))))
 */
 #[cfg(feature = "kat")]
 #[test]
-fn test_seq() {
+fn seq() {
     let mut s = Strobe::new(b"seqtest", SecParam::B256);
 
     let mut buf = [0u8; 10];
@@ -165,7 +165,7 @@ print("state == [{}]".format(', '.join(map("0x{:02x}".format, s.st))))
 */
 #[cfg(feature = "kat")]
 #[test]
-fn test_metadata() {
+fn metadata() {
     // We will accumulate output over 3 operations and 3 meta-operations
     let mut s = Strobe::new(b"metadatatest", SecParam::B256);
     let mut output = std::vec::Vec::new();
@@ -256,7 +256,7 @@ s.send_mac(small_n, meta_flags=C|T|M, metadata=small_n)
 print("[{}]".format(', '.join(map("0x{:02x}".format, s.st))))
 */
 #[test]
-fn test_long_inputs() {
+fn long_inputs() {
     let mut s = Strobe::new(b"bigtest", SecParam::B256);
     const BIG_N: usize = 9823;
     const SMALL_N: usize = 65;
@@ -312,7 +312,7 @@ fn test_long_inputs() {
 // Test that streaming in data using the `more` flag works as expected
 #[cfg(feature = "kat")]
 #[test]
-fn test_streaming_correctness() {
+fn streaming_correctness() {
     // Compute a few things without breaking up their inputs
     let one_shot_st: std::vec::Vec<u8> = {
         let mut s = Strobe::new(b"streamingtest", SecParam::B256);
@@ -359,7 +359,7 @@ fn test_streaming_correctness() {
 // after the same op. In this instance, the violating operation is a nonmutating one (it's AD)
 #[test]
 #[should_panic]
-fn test_streaming_soundness_nomutate() {
+fn streaming_soundness_nomutate() {
     let mut s = Strobe::new(b"mactest", SecParam::B256);
 
     // Key with valid steps
@@ -373,7 +373,7 @@ fn test_streaming_soundness_nomutate() {
 // Same as above, but whose violating operation is a mutating one (it's send_enc)
 #[test]
 #[should_panic]
-fn test_streaming_soundness_mutate() {
+fn streaming_soundness_mutate() {
     let mut s = Strobe::new(b"mactest", SecParam::B256);
 
     // Key with valid steps
@@ -388,7 +388,7 @@ fn test_streaming_soundness_mutate() {
 // Same as above but with ratchet
 #[test]
 #[should_panic]
-fn test_streaming_soundness_ratchet() {
+fn streaming_soundness_ratchet() {
     let mut s = Strobe::new(b"mactest", SecParam::B256);
 
     // Key with valid steps
@@ -401,7 +401,7 @@ fn test_streaming_soundness_ratchet() {
 
 // Test that decrypt(encrypt(msg)) == msg
 #[test]
-fn test_enc_correctness() {
+fn enc_correctness() {
     let orig_msg = b"Hello there";
     let mut tx = Strobe::new(b"enccorrectnesstest", SecParam::B256);
     let mut rx = Strobe::new(b"enccorrectnesstest", SecParam::B256);
@@ -419,7 +419,7 @@ fn test_enc_correctness() {
 
 // Test that recv_mac(send_mac()) doesn't error, and recv_mac(otherstuff) does error
 #[test]
-fn test_mac_correctness_and_soundness() {
+fn mac_correctness_and_soundness() {
     let mut tx = Strobe::new(b"mactest", SecParam::B256);
     let mut rx = Strobe::new(b"mactest", SecParam::B256);
 
@@ -446,3 +446,39 @@ fn test_mac_correctness_and_soundness() {
     let bad_res = rx.recv_mac(&bad_mac.try_into().unwrap());
     assert!(bad_res.is_err());
 }
+
+// Regression test: `prf` and `send_mac` must overwrite the caller's buffer, not XOR into it.
+// A previous commit incorrectly XORed the PRF buffer into the state, and it was barely caught by
+// tests. This test explicitly checks that the prior value of the PRF buffer does not matter.
+#[test]
+fn output_independent_of_input_buffer() {
+    // Build up some nontrivial state to extract from
+    let mut s = Strobe::new(b"output-overwrite-regression", SecParam::B256);
+    s.key(b"secretsauce", false);
+    s.ad(b"some associated data", false);
+
+    // Clone the state twice and have it output the PRF into two buffers. One filled with zeroes
+    // and one filled with 0xAA. They should be identical after being filled.
+    {
+        let mut from_zeros = [0x00u8; 64];
+        let mut from_dirty = [0xAAu8; 64];
+        s.clone().prf(&mut from_zeros, false);
+        s.clone().prf(&mut from_dirty, false);
+        assert_eq!(
+            from_zeros, from_dirty,
+            "PRF cannot depend on the initial contents of the output buffer"
+        );
+    }
+
+    // Do the same for send_mac
+    {
+        let mut from_zeros = [0x00u8; 32];
+        let mut from_dirty = [0xAAu8; 32];
+        s.clone().send_mac(&mut from_zeros, false);
+        s.clone().send_mac(&mut from_dirty, false);
+        assert_eq!(
+            from_zeros, from_dirty,
+            "send-MAC cannot depend on the initial contents of the output buffer"
+        );
+    }
+}

src/keccak.rs

@@ -18,21 +18,21 @@ use serde_big_array::BigArray;
 /// safely convertible to a pointer to [u64; 25] (since u64 words must be 8-byte aligned)
 #[derive(Clone, Zeroize)]
 #[cfg_attr(feature = "serialize_secret_state", derive(Serialize, Deserialize))]
-#[repr(align(8))]
 pub(crate) struct AlignedKeccakState(
     #[cfg_attr(feature = "serialize_secret_state", serde(with = "BigArray"))]
     pub(crate)  [u8; 8 * KECCAK_BLOCK_SIZE],
 );
 
 /// Performs the keccakf\[1600\] permutation on a byte buffer
 // Make a little-endian copy, do the operation, then copy the bytes back. Hopefully the compiler
-// will optimize out the copy if we' re on a little endian machine. I don't feel comfortable doing
+// will optimize out the copy if we're on a little endian machine. I don't feel comfortable doing
 // a mem transmute.
 pub(crate) fn keccakf_u8(st: &mut AlignedKeccakState) {
     let mut keccak_block = [0u64; KECCAK_BLOCK_SIZE];
     LittleEndian::read_u64_into(&st.0, &mut keccak_block);
     Keccak::new().with_f1600(|f| f(&mut keccak_block));
     LittleEndian::write_u64_into(&keccak_block, &mut st.0);
+    keccak_block.zeroize();
 }
 
 /*

src/strobe.rs

@@ -108,6 +108,15 @@ impl core::fmt::Display for AuthError {
 ///
 /// Finally, `ratchet` and `meta_ratchet` take a `usize` argument instead of bytes. These functions
 /// are individually commented below.
+#[cfg_attr(
+    feature = "serialize_secret_state",
+    doc = "\n\n\
+        ⚠️Security warning⚠️ \
+        When the `serialize_secret_state` feature is enabled, `Strobe` implements \
+        `serde::Serialize`/`serde::Deserialize`. Serializing Strobe state outputs \
+        security-sensitive data that MUST be kept private. Treat the data as you would a private \
+        encryption/decryption key."
+)]
 #[derive(Clone, Zeroize, ZeroizeOnDrop)]
 #[cfg_attr(feature = "serialize_secret_state", derive(Serialize, Deserialize))]
 pub struct Strobe {
@@ -243,109 +252,115 @@ impl Strobe {
         self.pos_begin = 0;
     }
 
-    /// XORs the given data into the state. This is a special case of the `duplex` code in the
-    /// STROBE paper.
-    fn absorb(&mut self, data: &[u8]) {
-        for b in data {
-            self.st.0[self.pos] ^= *b;
+    /// Runs the duplex loop over `data`, applying `f` to each `(state_byte, data_byte)` pair and
+    /// running the permutation each time the rate boundary is reached. For simplicity's sake,
+    /// rather than implementing the entire `duplex` function from the paper, we implement this for
+    /// generic `f` and let the caller pick `f`.
+    fn duplex_mut(&mut self, data: &mut [u8], mut f: impl FnMut(&mut u8, &mut u8)) {
+        let mut data_idx = 0;
+        while data_idx < data.len() {
+            // Pick out two equal-sized slices from state and chunk. We will zip them and run `f`
+            let chunk_size = core::cmp::min(self.rate - self.pos, data.len() - data_idx);
+            let state_chunk = &mut self.st.0[self.pos..self.pos + chunk_size];
+            let data_chunk = &mut data[data_idx..data_idx + chunk_size];
+
+            for (s, d) in state_chunk.iter_mut().zip(data_chunk.iter_mut()) {
+                f(s, d);
+            }
+
+            // Update the data cursor and self cursor
+            self.pos += chunk_size;
+            data_idx += chunk_size;
 
-            self.pos += 1;
+            // If we XORed enough to exhaust the rate, then permute
             if self.pos == self.rate {
                 self.run_f();
             }
         }
     }
 
-    /// XORs the given data into the state, then sets the data equal the state.  This is a special
-    /// case of the `duplex` code in the STROBE paper.
-    fn absorb_and_set(&mut self, data: &mut [u8]) {
-        for b in data {
-            let state_byte = self.st.0.get_mut(self.pos).unwrap();
-            *state_byte ^= *b;
-            *b = *state_byte;
+    /// Identical as [`Strobe::duplex_mut`], but where `data` is read-only
+    fn duplex_const(&mut self, data: &[u8], mut f: impl FnMut(&mut u8, u8)) {
+        let mut data_idx = 0;
+        while data_idx < data.len() {
+            // Pick out two equal-sized slices from state and chunk. We will zip them and run `f`
+            let chunk_size = core::cmp::min(self.rate - self.pos, data.len() - data_idx);
+            let state_chunk = &mut self.st.0[self.pos..self.pos + chunk_size];
+            let data_chunk = &data[data_idx..data_idx + chunk_size];
+
+            for (s, &d) in state_chunk.iter_mut().zip(data_chunk.iter()) {
+                f(s, d);
+            }
 
-            self.pos += 1;
+            // Update the data cursor and self cursor
+            self.pos += chunk_size;
+            data_idx += chunk_size;
+
+            // If we XORed enough to exhaust the rate, then permute
             if self.pos == self.rate {
                 self.run_f();
             }
         }
     }
 
+    /// XORs the given data into the state. This is a special case of the `duplex` code in the
+    /// STROBE paper.
+    fn absorb(&mut self, data: &[u8]) {
+        self.duplex_const(data, |s, d| *s ^= d);
+    }
+
+    /// XORs the given data into the state, then sets the data equal the state.  This is a special
+    /// case of the `duplex` code in the STROBE paper.
+    fn absorb_and_set(&mut self, data: &mut [u8]) {
+        self.duplex_mut(data, |s, d| {
+            *s ^= *d;
+            *d = *s;
+        });
+    }
+
     /// Copies the internal state into the given buffer. This is a special case of `absorb_and_set`
     /// where `data` is all zeros.
     fn copy_state(&mut self, data: &mut [u8]) {
-        for b in data {
-            *b = self.st.0[self.pos];
-
-            self.pos += 1;
-            if self.pos == self.rate {
-                self.run_f();
-            }
-        }
+        self.duplex_mut(data, |s, d| *d = *s);
     }
 
     /// Overwrites the state with the given data while XORing the given data with the old state.
     /// This is a special case of the `duplex` code in the STROBE paper.
     fn exchange(&mut self, data: &mut [u8]) {
-        for b in data {
-            let state_byte = self.st.0.get_mut(self.pos).unwrap();
-            *b ^= *state_byte;
-            *state_byte ^= *b;
-
-            self.pos += 1;
-            if self.pos == self.rate {
-                self.run_f();
-            }
-        }
+        self.duplex_mut(data, |s, d| {
+            *d ^= *s;
+            *s ^= *d;
+        });
     }
 
     /// Overwrites the state with the given data. This is a special case of `Strobe::exchange`,
     /// where we do not want to mutate the input data.
     fn overwrite(&mut self, data: &[u8]) {
-        for b in data {
-            self.st.0[self.pos] = *b;
-
-            self.pos += 1;
-            if self.pos == self.rate {
-                self.run_f();
-            }
-        }
+        self.duplex_const(data, |s, d| *s = d);
     }
 
     /// Copies the state into the given buffer and sets the state to 0. This is a special case of
     /// `Strobe::exchange`, where `data` is assumed to be the all-zeros string. This is precisely
     /// the case when the current operation is PRF.
     fn squeeze(&mut self, data: &mut [u8]) {
-        for b in data {
-            let state_byte = self.st.0.get_mut(self.pos).unwrap();
-            *b = *state_byte;
-            *state_byte = 0;
-
-            self.pos += 1;
-            if self.pos == self.rate {
-                self.run_f();
-            }
-        }
+        self.duplex_mut(data, |s, d| {
+            *d = *s;
+            *s = 0;
+        });
     }
 
-    /// Overwrites the state with a specified number of zeros. This is a special case of
-    /// `Strobe::exchange`. More specifically, it's a special case of `Strobe::overwrite` and
-    /// `Strobe::squeeze`. It's like `squeeze` in that we assume we've been given all zeros as
-    /// input, and like `overwrite` in that we do not mutate (or take) any input.
+    /// Overwrites the state with a specified number of zeros
     fn zero_state(&mut self, mut bytes_to_zero: usize) {
         static ZEROS: [u8; 8 * KECCAK_BLOCK_SIZE] = [0u8; 8 * KECCAK_BLOCK_SIZE];
 
-        // Do the zero-writing in chunks
+        // Repeatedly `overwrite` a chunk of zeros into the state until we've written the desired
+        // number of zeros
         while bytes_to_zero > 0 {
-            let slice_len = core::cmp::min(self.rate - self.pos, bytes_to_zero);
-            self.st.0[self.pos..(self.pos + slice_len)].copy_from_slice(&ZEROS[..slice_len]);
-
-            self.pos += slice_len;
-            bytes_to_zero -= slice_len;
+            let chunk_size = core::cmp::min(bytes_to_zero, 8 * KECCAK_BLOCK_SIZE);
+            let chunk = &ZEROS[..chunk_size];
+            self.overwrite(chunk);
 
-            if self.pos == self.rate {
-                self.run_f();
-            }
+            bytes_to_zero -= chunk_size;
         }
     }