From 7f47ff6071b27a1df06661af16829d900f3d53bc Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 21:12:56 -0400 Subject: [PATCH 01/17] Commit Claude audit --- AUDIT.md | 144 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 AUDIT.md diff --git a/AUDIT.md b/AUDIT.md new file mode 100644 index 0000000..82a425c --- /dev/null +++ b/AUDIT.md @@ -0,0 +1,144 @@ +# strobe-rs Code Audit + +**Scope:** `strobe-rs` v0.13.0 — pure-Rust implementation of the STROBE protocol framework (Keccak-f[1600]). +**Files reviewed:** `src/strobe.rs`, `src/keccak.rs`, `src/lib.rs`, tests, benches, examples, `Cargo.toml`. +**Date:** 2026-08-02 + +> Context: this is a small (~600 LOC of real logic), well-structured, `no_std` crypto primitive with strong known-answer-test (KAT) coverage against the reference Python implementation. The README already states it is unaudited. Overall the implementation is clean and the cryptographically sensitive paths (MAC verification) are handled correctly. The findings below are mostly defense-in-depth, hygiene, and performance items — I found no memory-safety bugs and no exploitable timing side channel. + +--- + +## Summary + +| # | Category | Severity | Item | +|---|----------|----------|------| +| S1 | Security | Low–Med | Full secret state left in un-zeroized stack buffer on every permutation | +| S2 | Security | Info | `serialize_secret_state` emits keys in cleartext | +| S3 | Security | Info | Misuse causes `panic!` (fail-closed, but an availability consideration) | +| S4 | Security | ✅ Positive | Constant-time MAC check; no secret-dependent branching | +| P1 | Misoptimization | Med | Byte-at-a-time duplex loops with a per-byte branch | +| P2 | Misoptimization | Low | `keccakf_u8` copies the 200-byte state in and out on every call | +| C1 | Code smell | Low | `#[repr(align(8))]` rationale is obsolete/misleading | +| C2 | Code smell | Low | Dead asserts in `new()` | +| C3 | Code smell | Low | Dead `OpFlags::K` branch in `begin_op` | +| C4 | Code smell | Low | Inconsistent `.get_mut().unwrap()` vs direct indexing | +| C5 | Code smell | Trivial | Typos in comments/identifiers/docs | + +--- + +## Security + +### S1 — Secret state is left in an un-zeroized stack buffer on every permutation (Low–Medium) + +`src/keccak.rs:31` + +```rust +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` holds a complete copy of the secret Keccak state (which after `key()` is derived from key material) and is **not** zeroized when it goes out of scope. This runs on essentially every operation, so a fresh plaintext copy of the secret state is repeatedly left on the stack. + +The crate goes to real trouble elsewhere to protect secrets — `Strobe` derives `ZeroizeOnDrop`, and `generalized_recv_mac` explicitly zeroizes its temporary MAC copy — so this is an inconsistency that partially undermines that guarantee. An attacker with a stack-memory disclosure (core dump, cold-boot, uninitialized-memory-reuse bug in a caller) could recover state material. + +**Fix:** zeroize the temporary before returning, e.g. + +```rust +use zeroize::Zeroize; +// ... +LittleEndian::write_u64_into(&keccak_block, &mut st.0); +keccak_block.zeroize(); +``` + +Note this only covers *this* crate's copy; the upstream `keccak` crate may also keep the state in registers/stack. Still worth closing the copy we own. + +### S2 — `serialize_secret_state` serializes keys in cleartext (Informational) + +The optional `serialize_secret_state` feature derives `Serialize`/`Deserialize` on `Strobe`, whose `st` field *is* the secret keystream state. This is by design and gated behind a non-default feature, but the produced blob is unencrypted key-equivalent material. Worth an explicit doc warning that the serialized output must be stored/transmitted with the same protection as a raw key (it currently has no such caveat at the feature/type level). + +### S3 — Misuse is handled with `panic!` (Informational / availability) + +`operate`, `operate_no_mutate`, and `validate_streaming` `panic!` / `assert!` on misuse (the unimplemented `K` flag, and improper use of the `more` streaming flag). This is a reasonable fail-closed choice for a crypto primitive, and the offending inputs are developer-chosen (flags come from the typed API, not attacker bytes), so it is not an attacker-triggered DoS in normal use. Flagging only so downstream integrators know these paths abort the process rather than return an error. + +### S4 — Positive findings ✅ + +- **Constant-time MAC verification** (`generalized_recv_mac`, `src/strobe.rs:449`) uses `subtle::ConstantTimeEq` and accumulates with `&=`, only branching on the final aggregated `Choice`. This is the standard correct pattern, and the comparison length is a public const generic. The temporary MAC copy is zeroized. Good. +- **No secret-dependent control flow.** Every duplex loop iterates over `data.len()` (a public message length); there are no branches or table lookups keyed on secret bytes, and the underlying `keccak` f1600 is constant-time. I did not find a timing side channel on secret data. +- No `unsafe`, no `transmute`, no raw pointers. The only `.unwrap()`s in non-test code (`src/strobe.rs:263,291,320,364`) are statically unreachable given the surrounding invariants. + +--- + +## Misoptimizations + +### P1 — Byte-at-a-time duplex loops (Medium) + +`absorb`, `absorb_and_set`, `copy_state`, `exchange`, `overwrite`, and `squeeze` (`src/strobe.rs:248–329`) all follow this shape: + +```rust +for b in data { + self.st.0[self.pos] ^= *b; + self.pos += 1; + if self.pos == self.rate { + self.run_f(); + } +} +``` + +Every single byte incurs a bounds-checked index and a `pos == rate` branch. Between permutations there are up to `rate` (134–166) contiguous bytes that could be processed as a slice. `zero_state` (`src/strobe.rs:335`) already does exactly this chunking — the same pattern should be applied to the others. This is the dominant throughput cost for bulk `send_enc`/`recv_enc`/`prf`, and refactoring to slice-at-a-time (`copy_from_slice` / a chunked XOR over `st.0[pos..pos+n]`) would let the compiler autovectorize the XOR and drop the per-byte branch. Correctness is easy to hold constant thanks to the existing KATs. + +### P2 — `keccakf_u8` round-trips the whole state every call (Low) + +`src/keccak.rs:31` copies all 200 bytes into a `[u64; 25]`, permutes, then copies back — on every `run_f`. The comment ("Hopefully the compiler will optimize out the copy if we're on a little endian machine") is optimistic: `read_u64_into`/`write_u64_into` are real byte-shuffling copies and won't vanish. The clean fix is to store the state natively as `[u64; 25]` and only convert at the byte-oriented boundaries (state init, KAT comparisons, serde), removing the per-permutation copy entirely. That's a larger refactor (`AlignedKeccakState` is currently a byte array threaded through all the duplex code), so it's a Low-priority structural improvement rather than a quick win. + +--- + +## Code smells + +### C1 — `#[repr(align(8))]` rationale is obsolete (`src/keccak.rs:17–25`) + +The doc comment says the 8-byte alignment exists "to make pointers to it safely convertible to a pointer to `[u64; 25]`". No such conversion happens anywhere — the code uses `read_u64_into`/`write_u64_into` byte copies precisely to avoid a transmute (see the `keccak.rs:28` comment: "I don't feel comfortable doing a mem transmute"). The alignment is harmless (and marginally helps the copies) but the stated justification is misleading. Either restore the intended zero-copy path (see P2) or update the comment to reflect why the alignment is actually kept. + +### C2 — Dead asserts in `new()` (`src/strobe.rs:170–171`) + +```rust +assert!(rate >= 1); +assert!(rate < 254); +``` + +`rate` is fully determined by `SecParam`, which has exactly two variants → `rate` is 166 (B128) or 134 (B256). These asserts can never fire. Harmless, but they read as guarding attacker input when they don't. Fine to keep as documentation, but worth a comment noting they're structurally unreachable. + +### C3 — Dead `OpFlags::K` branch (`src/strobe.rs:374`) + +```rust +let force_f = flags.contains(OpFlags::C) || flags.contains(OpFlags::K); +``` + +`K` triggers `panic!("Op flag K not implemented")` in both `operate` and `operate_no_mutate` before `begin_op` runs, and the only other caller (`generalized_ratchet`) never sets `K`. The `|| flags.contains(OpFlags::K)` term is therefore unreachable. Drop it or leave a note that it's forward-looking for when `K` lands. + +### C4 — Inconsistent element access (`src/strobe.rs`) + +`absorb_and_set` (263), `exchange` (291), and `squeeze` (320) use `self.st.0.get_mut(self.pos).unwrap()`, while the structurally identical `absorb` (250), `copy_state` (278), and `overwrite` (305) use direct indexing `self.st.0[self.pos]`. Both forms panic identically on out-of-range and compile to the same bounds check; the mix is just visual noise. Pick one (direct indexing is the more idiomatic and reads cleaner here). + +### C5 — Typos (Trivial) + +- `src/keccak.rs:29` — "if we' re on a little endian machine". +- `Cargo.toml:41` — "Criteron benches" (→ Criterion). +- `README.md` — "target/crieteron/report" (→ criterion). +- `benches/benches.rs` — `"rachet 16"`, `"meta_rachet 16"` (→ ratchet). + +--- + +## Suggested priority order + +1. **S1** — zeroize `keccak_block` (tiny, closes a real defense-in-depth gap consistent with the crate's own posture). +2. **P1** — chunk the duplex loops (best perf/effort ratio; KATs de-risk it). +3. **C1–C3** — remove/annotate dead code and the stale alignment rationale. +4. **P2** — consider a native `[u64; 25]` state representation (larger refactor). +5. **S2** — add a security caveat to the `serialize_secret_state` docs. +6. **C4/C5** — cosmetic cleanup. + +*No memory-safety or correctness defects were found; the KAT suite passes against the reference implementation and covers streaming, metadata, long inputs, and boundary cases.* From 16c71228d724a2682c8273c7c5fe09f93a0e4b40 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 21:17:16 -0400 Subject: [PATCH 02/17] Add 8KiB send_enc to benches --- benches/benches.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/benches/benches.rs b/benches/benches.rs index a4909ee..f5940e4 100644 --- a/benches/benches.rs +++ b/benches/benches.rs @@ -1,6 +1,6 @@ use strobe_rs::{SecParam, Strobe}; -use criterion::{Criterion, criterion_group, criterion_main}; +use criterion::{criterion_group, criterion_main, Criterion}; // Literally all these functions (besides ratchet) should have the same runtime. But a benchmark // can't hurt, I suppose @@ -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))); From 3b7912c3ef978932dad3e428a3cfc8ab84674060 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 21:20:42 -0400 Subject: [PATCH 03/17] Zeroize keccak block after every keccak-f --- benches/benches.rs | 2 +- src/keccak.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/benches/benches.rs b/benches/benches.rs index f5940e4..aff0d5e 100644 --- a/benches/benches.rs +++ b/benches/benches.rs @@ -1,6 +1,6 @@ use strobe_rs::{SecParam, Strobe}; -use criterion::{criterion_group, criterion_main, Criterion}; +use criterion::{Criterion, criterion_group, criterion_main}; // Literally all these functions (besides ratchet) should have the same runtime. But a benchmark // can't hurt, I suppose diff --git a/src/keccak.rs b/src/keccak.rs index 6be654d..badd12c 100644 --- a/src/keccak.rs +++ b/src/keccak.rs @@ -33,6 +33,7 @@ pub(crate) fn keccakf_u8(st: &mut AlignedKeccakState) { 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(); } /* From 0db053040bcc9107bb110189bb40c7be89090313 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 21:49:07 -0400 Subject: [PATCH 04/17] First pass at chunk-based duplex ops rather than byte-by-byte; 60% speedups across the board --- src/strobe.rs | 160 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 158 insertions(+), 2 deletions(-) diff --git a/src/strobe.rs b/src/strobe.rs index 1b65e6f..7190db8 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -1,4 +1,4 @@ -use crate::keccak::{AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE, keccakf_u8}; +use crate::keccak::{keccakf_u8, AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE}; use bitflags::bitflags; use subtle::{self, ConstantTimeEq}; @@ -50,7 +50,7 @@ impl<'de> Deserialize<'de> for OpFlags { impl Zeroize for OpFlags { fn zeroize(&mut self) { - self.0.0.zeroize(); + self.0 .0.zeroize(); } } @@ -246,6 +246,29 @@ impl Strobe { /// 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]) { + let mut data_idx = 0; + loop { + let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); + let remaining_state = &mut self.st.0[self.pos..self.rate]; + + for (s, b) in remaining_state.iter_mut().zip(data.iter().skip(data_idx)) { + *s ^= b; + } + + // Move the data cursor and self cursor + data_idx += num_to_xor; + self.pos += num_to_xor; + + // If we XORed enough to exhaust the rate, then permute + if self.pos == self.rate { + self.run_f(); + } + + if data_idx == data.len() { + break; + } + } + /* for b in data { self.st.0[self.pos] ^= *b; @@ -254,11 +277,39 @@ impl Strobe { 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]) { + let mut data_idx = 0; + loop { + let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); + let remaining_state = &mut self.st.0[self.pos..self.rate]; + + for (s, b) in remaining_state + .iter_mut() + .zip(data.iter_mut().skip(data_idx)) + { + *s ^= *b; + *b = *s; + } + + // Move the data cursor and self cursor + data_idx += num_to_xor; + self.pos += num_to_xor; + + // If we XORed enough to exhaust the rate, then permute + if self.pos == self.rate { + self.run_f(); + } + + if data_idx == data.len() { + break; + } + } + /* for b in data { let state_byte = self.st.0.get_mut(self.pos).unwrap(); *state_byte ^= *b; @@ -269,11 +320,35 @@ impl Strobe { self.run_f(); } } + */ } /// 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]) { + let mut data_idx = 0; + loop { + let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); + let remaining_state = &mut self.st.0[self.pos..self.rate]; + + for (s, b) in remaining_state.iter().zip(data.iter_mut().skip(data_idx)) { + *b = *s; + } + + // Move the data cursor and self cursor + data_idx += num_to_xor; + self.pos += num_to_xor; + + // If we XORed enough to exhaust the rate, then permute + if self.pos == self.rate { + self.run_f(); + } + + if data_idx == data.len() { + break; + } + } + /* for b in data { *b = self.st.0[self.pos]; @@ -282,11 +357,39 @@ impl Strobe { self.run_f(); } } + */ } /// 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]) { + let mut data_idx = 0; + loop { + let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); + let remaining_state = &mut self.st.0[self.pos..self.rate]; + + for (s, b) in remaining_state + .iter_mut() + .zip(data.iter_mut().skip(data_idx)) + { + *b ^= *s; + *s ^= *b; + } + + // Move the data cursor and self cursor + data_idx += num_to_xor; + self.pos += num_to_xor; + + // If we XORed enough to exhaust the rate, then permute + if self.pos == self.rate { + self.run_f(); + } + + if data_idx == data.len() { + break; + } + } + /* for b in data { let state_byte = self.st.0.get_mut(self.pos).unwrap(); *b ^= *state_byte; @@ -297,11 +400,35 @@ impl Strobe { self.run_f(); } } + */ } /// 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]) { + let mut data_idx = 0; + loop { + let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); + let remaining_state = &mut self.st.0[self.pos..self.rate]; + + for (s, b) in remaining_state.iter_mut().zip(data.iter().skip(data_idx)) { + *s = *b; + } + + // Move the data cursor and self cursor + data_idx += num_to_xor; + self.pos += num_to_xor; + + // If we XORed enough to exhaust the rate, then permute + if self.pos == self.rate { + self.run_f(); + } + + if data_idx == data.len() { + break; + } + } + /* for b in data { self.st.0[self.pos] = *b; @@ -310,12 +437,40 @@ impl Strobe { self.run_f(); } } + */ } /// 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]) { + let mut data_idx = 0; + loop { + let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); + let remaining_state = &mut self.st.0[self.pos..self.rate]; + + for (s, b) in remaining_state + .iter_mut() + .zip(data.iter_mut().skip(data_idx)) + { + *b ^= *s; + *s = 0; + } + + // Move the data cursor and self cursor + data_idx += num_to_xor; + self.pos += num_to_xor; + + // If we XORed enough to exhaust the rate, then permute + if self.pos == self.rate { + self.run_f(); + } + + if data_idx == data.len() { + break; + } + } + /* for b in data { let state_byte = self.st.0.get_mut(self.pos).unwrap(); *b = *state_byte; @@ -326,6 +481,7 @@ impl Strobe { self.run_f(); } } + */ } /// Overwrites the state with a specified number of zeros. This is a special case of From 29735e1b98216077857746ceed83f75b337ee7dd Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 22:15:38 -0400 Subject: [PATCH 05/17] Simplify chunked code --- src/strobe.rs | 241 ++++++++++---------------------------------------- 1 file changed, 49 insertions(+), 192 deletions(-) diff --git a/src/strobe.rs b/src/strobe.rs index 7190db8..5cf070c 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -243,245 +243,102 @@ 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]) { + /// 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. Data is processed in + /// contiguous chunks of up to `rate - pos` bytes, so the inner loop autovectorizes. This is + /// the shared driver for the mutating specializations of the `duplex` code in the STROBE + /// paper. + fn duplex_mut(&mut self, data: &mut [u8], mut f: impl FnMut(&mut u8, &mut u8)) { let mut data_idx = 0; - loop { + while data_idx < data.len() { + // Pick out two equal-sized slices from state and chunk. We will zip them and run `f` let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let remaining_state = &mut self.st.0[self.pos..self.rate]; + let state = &mut self.st.0[self.pos..self.pos + num_to_xor]; + let chunk = &mut data[data_idx..data_idx + num_to_xor]; - for (s, b) in remaining_state.iter_mut().zip(data.iter().skip(data_idx)) { - *s ^= b; + for (s, d) in state.iter_mut().zip(chunk.iter_mut()) { + f(s, d); } - // Move the data cursor and self cursor - data_idx += num_to_xor; + // Update the data cursor and self cursor self.pos += num_to_xor; + data_idx += num_to_xor; // If we XORed enough to exhaust the rate, then permute if self.pos == self.rate { self.run_f(); } - - if data_idx == data.len() { - break; - } - } - /* - for b in data { - self.st.0[self.pos] ^= *b; - - self.pos += 1; - 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]) { + /// 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; - loop { + while data_idx < data.len() { + // Pick out two equal-sized slices from state and chunk. We will zip them and run `f` let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let remaining_state = &mut self.st.0[self.pos..self.rate]; - - for (s, b) in remaining_state - .iter_mut() - .zip(data.iter_mut().skip(data_idx)) - { - *s ^= *b; - *b = *s; + let state = &mut self.st.0[self.pos..self.pos + num_to_xor]; + let chunk = &data[data_idx..data_idx + num_to_xor]; + + for (s, &d) in state.iter_mut().zip(chunk.iter()) { + f(s, d); } - // Move the data cursor and self cursor - data_idx += num_to_xor; + // Update the data cursor and self cursor self.pos += num_to_xor; + data_idx += num_to_xor; // If we XORed enough to exhaust the rate, then permute if self.pos == self.rate { self.run_f(); } - - if data_idx == data.len() { - break; - } } - /* - for b in data { - let state_byte = self.st.0.get_mut(self.pos).unwrap(); - *state_byte ^= *b; - *b = *state_byte; + } - self.pos += 1; - 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]) { - let mut data_idx = 0; - loop { - let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let remaining_state = &mut self.st.0[self.pos..self.rate]; - - for (s, b) in remaining_state.iter().zip(data.iter_mut().skip(data_idx)) { - *b = *s; - } - - // Move the data cursor and self cursor - data_idx += num_to_xor; - self.pos += num_to_xor; - - // If we XORed enough to exhaust the rate, then permute - if self.pos == self.rate { - self.run_f(); - } - - if data_idx == data.len() { - break; - } - } - /* - 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]) { - let mut data_idx = 0; - loop { - let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let remaining_state = &mut self.st.0[self.pos..self.rate]; - - for (s, b) in remaining_state - .iter_mut() - .zip(data.iter_mut().skip(data_idx)) - { - *b ^= *s; - *s ^= *b; - } - - // Move the data cursor and self cursor - data_idx += num_to_xor; - self.pos += num_to_xor; - - // If we XORed enough to exhaust the rate, then permute - if self.pos == self.rate { - self.run_f(); - } - - if data_idx == data.len() { - break; - } - } - /* - 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]) { - let mut data_idx = 0; - loop { - let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let remaining_state = &mut self.st.0[self.pos..self.rate]; - - for (s, b) in remaining_state.iter_mut().zip(data.iter().skip(data_idx)) { - *s = *b; - } - - // Move the data cursor and self cursor - data_idx += num_to_xor; - self.pos += num_to_xor; - - // If we XORed enough to exhaust the rate, then permute - if self.pos == self.rate { - self.run_f(); - } - - if data_idx == data.len() { - break; - } - } - /* - 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]) { - let mut data_idx = 0; - loop { - let num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let remaining_state = &mut self.st.0[self.pos..self.rate]; - - for (s, b) in remaining_state - .iter_mut() - .zip(data.iter_mut().skip(data_idx)) - { - *b ^= *s; - *s = 0; - } - - // Move the data cursor and self cursor - data_idx += num_to_xor; - self.pos += num_to_xor; - - // If we XORed enough to exhaust the rate, then permute - if self.pos == self.rate { - self.run_f(); - } - - if data_idx == data.len() { - break; - } - } - /* - 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 From 1c16095f8fe8ce3951de58f7b002b3b88e9e5f97 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 22:38:56 -0400 Subject: [PATCH 06/17] Fix typo when simplifying squeeze() --- src/strobe.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/strobe.rs b/src/strobe.rs index 5cf070c..ff4a602 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -336,7 +336,7 @@ impl Strobe { /// the case when the current operation is PRF. fn squeeze(&mut self, data: &mut [u8]) { self.duplex_mut(data, |s, d| { - *d ^= *s; + *d = *s; *s = 0; }); } From 23aa66418fb5eac18ae913f5b935246187ed0426 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 23:12:51 -0400 Subject: [PATCH 07/17] Add regression test for bug in 0db0530 --- src/basic_tests.rs | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/src/basic_tests.rs b/src/basic_tests.rs index 11dce8f..6a25633 100644 --- a/src/basic_tests.rs +++ b/src/basic_tests.rs @@ -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 test_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" + ); + } +} From 5ec645b667de4a415066421f5ac4bca5e7a352a4 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 23:14:07 -0400 Subject: [PATCH 08/17] Remove annoying test_* prefix from every test name --- src/basic_tests.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/basic_tests.rs b/src/basic_tests.rs index 6a25633..aadd482 100644 --- a/src/basic_tests.rs +++ b/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 = { 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); @@ -451,7 +451,7 @@ fn test_mac_correctness_and_soundness() { // 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 test_output_independent_of_input_buffer() { +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); From 5ee599042a40856c782796bd8b10ad733461b8d9 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 23:14:12 -0400 Subject: [PATCH 09/17] cargo fmt --- src/strobe.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/strobe.rs b/src/strobe.rs index ff4a602..3fd0ef5 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -1,4 +1,4 @@ -use crate::keccak::{keccakf_u8, AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE}; +use crate::keccak::{AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE, keccakf_u8}; use bitflags::bitflags; use subtle::{self, ConstantTimeEq}; @@ -50,7 +50,7 @@ impl<'de> Deserialize<'de> for OpFlags { impl Zeroize for OpFlags { fn zeroize(&mut self) { - self.0 .0.zeroize(); + self.0.0.zeroize(); } } From a24fe43c96aff911b0784d0cfbbcfd8050a0b007 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 23:23:45 -0400 Subject: [PATCH 10/17] Simplify zero_state --- src/strobe.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/strobe.rs b/src/strobe.rs index 3fd0ef5..f87efef 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -341,24 +341,18 @@ impl Strobe { }); } - /// 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]); + let chunk_size = core::cmp::min(bytes_to_zero, 8 * KECCAK_BLOCK_SIZE); + let chunk = &ZEROS[..chunk_size]; + self.overwrite(chunk); - self.pos += slice_len; - bytes_to_zero -= slice_len; - - if self.pos == self.rate { - self.run_f(); - } + bytes_to_zero -= chunk_size; } } From 9410cf16cd969829105f2519394f901929c5ca72 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 23:26:01 -0400 Subject: [PATCH 11/17] Typos --- Cargo.toml | 2 +- README.md | 2 +- src/keccak.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bde9e31..dec20ed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -35,7 +35,7 @@ hex = "0.4" rand = "0.10" serde_json = "1" -# Criteron benches +# Criterion benches [[bench]] name = "benches" harness = false diff --git a/README.md b/README.md index d715ece..eec2a03 100644 --- a/README.md +++ b/README.md @@ -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 ------- diff --git a/src/keccak.rs b/src/keccak.rs index badd12c..e0b35d2 100644 --- a/src/keccak.rs +++ b/src/keccak.rs @@ -26,7 +26,7 @@ pub(crate) struct AlignedKeccakState( /// 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]; From 5b065b9a2b6c9710b5513239e8fbf47eb46423cf Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 23:39:52 -0400 Subject: [PATCH 12/17] Add better comments about serialize_secret_state --- README.md | 2 +- src/strobe.rs | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index eec2a03..c51e858 100644 --- a/README.md +++ b/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). diff --git a/src/strobe.rs b/src/strobe.rs index f87efef..5cdebe4 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -1,4 +1,4 @@ -use crate::keccak::{AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE, keccakf_u8}; +use crate::keccak::{keccakf_u8, AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE}; use bitflags::bitflags; use subtle::{self, ConstantTimeEq}; @@ -50,7 +50,7 @@ impl<'de> Deserialize<'de> for OpFlags { impl Zeroize for OpFlags { fn zeroize(&mut self) { - self.0.0.zeroize(); + self.0 .0.zeroize(); } } @@ -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 { From 41d7fdf6347a4fbac7c6b22d905921879e705d5b Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sun, 2 Aug 2026 23:40:57 -0400 Subject: [PATCH 13/17] cargo fmt --- src/strobe.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/strobe.rs b/src/strobe.rs index 5cdebe4..6dd0ae2 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -1,4 +1,4 @@ -use crate::keccak::{keccakf_u8, AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE}; +use crate::keccak::{AlignedKeccakState, KECCAK_BLOCK_BITLEN_STR, KECCAK_BLOCK_SIZE, keccakf_u8}; use bitflags::bitflags; use subtle::{self, ConstantTimeEq}; @@ -50,7 +50,7 @@ impl<'de> Deserialize<'de> for OpFlags { impl Zeroize for OpFlags { fn zeroize(&mut self) { - self.0 .0.zeroize(); + self.0.0.zeroize(); } } From b3bb4ecbb791d3b85876de6855823f8b4efbfca5 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Mon, 3 Aug 2026 01:56:10 -0400 Subject: [PATCH 14/17] Renames for clarity --- src/strobe.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/strobe.rs b/src/strobe.rs index 6dd0ae2..c1c0baa 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -261,17 +261,17 @@ impl Strobe { 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 num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let state = &mut self.st.0[self.pos..self.pos + num_to_xor]; - let chunk = &mut data[data_idx..data_idx + num_to_xor]; + 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.iter_mut().zip(chunk.iter_mut()) { + 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 += num_to_xor; - data_idx += num_to_xor; + self.pos += chunk_size; + data_idx += chunk_size; // If we XORed enough to exhaust the rate, then permute if self.pos == self.rate { @@ -285,17 +285,17 @@ impl Strobe { 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 num_to_xor = core::cmp::min(self.rate - self.pos, data.len() - data_idx); - let state = &mut self.st.0[self.pos..self.pos + num_to_xor]; - let chunk = &data[data_idx..data_idx + num_to_xor]; + 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.iter_mut().zip(chunk.iter()) { + for (s, &d) in state_chunk.iter_mut().zip(data_chunk.iter()) { f(s, d); } // Update the data cursor and self cursor - self.pos += num_to_xor; - data_idx += num_to_xor; + self.pos += chunk_size; + data_idx += chunk_size; // If we XORed enough to exhaust the rate, then permute if self.pos == self.rate { From 5632e15db3bef9e3b30da4fdedf0ac9288937bf9 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Mon, 3 Aug 2026 01:56:28 -0400 Subject: [PATCH 15/17] Remove claude audit --- AUDIT.md | 144 ------------------------------------------------------- 1 file changed, 144 deletions(-) delete mode 100644 AUDIT.md diff --git a/AUDIT.md b/AUDIT.md deleted file mode 100644 index 82a425c..0000000 --- a/AUDIT.md +++ /dev/null @@ -1,144 +0,0 @@ -# strobe-rs Code Audit - -**Scope:** `strobe-rs` v0.13.0 — pure-Rust implementation of the STROBE protocol framework (Keccak-f[1600]). -**Files reviewed:** `src/strobe.rs`, `src/keccak.rs`, `src/lib.rs`, tests, benches, examples, `Cargo.toml`. -**Date:** 2026-08-02 - -> Context: this is a small (~600 LOC of real logic), well-structured, `no_std` crypto primitive with strong known-answer-test (KAT) coverage against the reference Python implementation. The README already states it is unaudited. Overall the implementation is clean and the cryptographically sensitive paths (MAC verification) are handled correctly. The findings below are mostly defense-in-depth, hygiene, and performance items — I found no memory-safety bugs and no exploitable timing side channel. - ---- - -## Summary - -| # | Category | Severity | Item | -|---|----------|----------|------| -| S1 | Security | Low–Med | Full secret state left in un-zeroized stack buffer on every permutation | -| S2 | Security | Info | `serialize_secret_state` emits keys in cleartext | -| S3 | Security | Info | Misuse causes `panic!` (fail-closed, but an availability consideration) | -| S4 | Security | ✅ Positive | Constant-time MAC check; no secret-dependent branching | -| P1 | Misoptimization | Med | Byte-at-a-time duplex loops with a per-byte branch | -| P2 | Misoptimization | Low | `keccakf_u8` copies the 200-byte state in and out on every call | -| C1 | Code smell | Low | `#[repr(align(8))]` rationale is obsolete/misleading | -| C2 | Code smell | Low | Dead asserts in `new()` | -| C3 | Code smell | Low | Dead `OpFlags::K` branch in `begin_op` | -| C4 | Code smell | Low | Inconsistent `.get_mut().unwrap()` vs direct indexing | -| C5 | Code smell | Trivial | Typos in comments/identifiers/docs | - ---- - -## Security - -### S1 — Secret state is left in an un-zeroized stack buffer on every permutation (Low–Medium) - -`src/keccak.rs:31` - -```rust -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` holds a complete copy of the secret Keccak state (which after `key()` is derived from key material) and is **not** zeroized when it goes out of scope. This runs on essentially every operation, so a fresh plaintext copy of the secret state is repeatedly left on the stack. - -The crate goes to real trouble elsewhere to protect secrets — `Strobe` derives `ZeroizeOnDrop`, and `generalized_recv_mac` explicitly zeroizes its temporary MAC copy — so this is an inconsistency that partially undermines that guarantee. An attacker with a stack-memory disclosure (core dump, cold-boot, uninitialized-memory-reuse bug in a caller) could recover state material. - -**Fix:** zeroize the temporary before returning, e.g. - -```rust -use zeroize::Zeroize; -// ... -LittleEndian::write_u64_into(&keccak_block, &mut st.0); -keccak_block.zeroize(); -``` - -Note this only covers *this* crate's copy; the upstream `keccak` crate may also keep the state in registers/stack. Still worth closing the copy we own. - -### S2 — `serialize_secret_state` serializes keys in cleartext (Informational) - -The optional `serialize_secret_state` feature derives `Serialize`/`Deserialize` on `Strobe`, whose `st` field *is* the secret keystream state. This is by design and gated behind a non-default feature, but the produced blob is unencrypted key-equivalent material. Worth an explicit doc warning that the serialized output must be stored/transmitted with the same protection as a raw key (it currently has no such caveat at the feature/type level). - -### S3 — Misuse is handled with `panic!` (Informational / availability) - -`operate`, `operate_no_mutate`, and `validate_streaming` `panic!` / `assert!` on misuse (the unimplemented `K` flag, and improper use of the `more` streaming flag). This is a reasonable fail-closed choice for a crypto primitive, and the offending inputs are developer-chosen (flags come from the typed API, not attacker bytes), so it is not an attacker-triggered DoS in normal use. Flagging only so downstream integrators know these paths abort the process rather than return an error. - -### S4 — Positive findings ✅ - -- **Constant-time MAC verification** (`generalized_recv_mac`, `src/strobe.rs:449`) uses `subtle::ConstantTimeEq` and accumulates with `&=`, only branching on the final aggregated `Choice`. This is the standard correct pattern, and the comparison length is a public const generic. The temporary MAC copy is zeroized. Good. -- **No secret-dependent control flow.** Every duplex loop iterates over `data.len()` (a public message length); there are no branches or table lookups keyed on secret bytes, and the underlying `keccak` f1600 is constant-time. I did not find a timing side channel on secret data. -- No `unsafe`, no `transmute`, no raw pointers. The only `.unwrap()`s in non-test code (`src/strobe.rs:263,291,320,364`) are statically unreachable given the surrounding invariants. - ---- - -## Misoptimizations - -### P1 — Byte-at-a-time duplex loops (Medium) - -`absorb`, `absorb_and_set`, `copy_state`, `exchange`, `overwrite`, and `squeeze` (`src/strobe.rs:248–329`) all follow this shape: - -```rust -for b in data { - self.st.0[self.pos] ^= *b; - self.pos += 1; - if self.pos == self.rate { - self.run_f(); - } -} -``` - -Every single byte incurs a bounds-checked index and a `pos == rate` branch. Between permutations there are up to `rate` (134–166) contiguous bytes that could be processed as a slice. `zero_state` (`src/strobe.rs:335`) already does exactly this chunking — the same pattern should be applied to the others. This is the dominant throughput cost for bulk `send_enc`/`recv_enc`/`prf`, and refactoring to slice-at-a-time (`copy_from_slice` / a chunked XOR over `st.0[pos..pos+n]`) would let the compiler autovectorize the XOR and drop the per-byte branch. Correctness is easy to hold constant thanks to the existing KATs. - -### P2 — `keccakf_u8` round-trips the whole state every call (Low) - -`src/keccak.rs:31` copies all 200 bytes into a `[u64; 25]`, permutes, then copies back — on every `run_f`. The comment ("Hopefully the compiler will optimize out the copy if we're on a little endian machine") is optimistic: `read_u64_into`/`write_u64_into` are real byte-shuffling copies and won't vanish. The clean fix is to store the state natively as `[u64; 25]` and only convert at the byte-oriented boundaries (state init, KAT comparisons, serde), removing the per-permutation copy entirely. That's a larger refactor (`AlignedKeccakState` is currently a byte array threaded through all the duplex code), so it's a Low-priority structural improvement rather than a quick win. - ---- - -## Code smells - -### C1 — `#[repr(align(8))]` rationale is obsolete (`src/keccak.rs:17–25`) - -The doc comment says the 8-byte alignment exists "to make pointers to it safely convertible to a pointer to `[u64; 25]`". No such conversion happens anywhere — the code uses `read_u64_into`/`write_u64_into` byte copies precisely to avoid a transmute (see the `keccak.rs:28` comment: "I don't feel comfortable doing a mem transmute"). The alignment is harmless (and marginally helps the copies) but the stated justification is misleading. Either restore the intended zero-copy path (see P2) or update the comment to reflect why the alignment is actually kept. - -### C2 — Dead asserts in `new()` (`src/strobe.rs:170–171`) - -```rust -assert!(rate >= 1); -assert!(rate < 254); -``` - -`rate` is fully determined by `SecParam`, which has exactly two variants → `rate` is 166 (B128) or 134 (B256). These asserts can never fire. Harmless, but they read as guarding attacker input when they don't. Fine to keep as documentation, but worth a comment noting they're structurally unreachable. - -### C3 — Dead `OpFlags::K` branch (`src/strobe.rs:374`) - -```rust -let force_f = flags.contains(OpFlags::C) || flags.contains(OpFlags::K); -``` - -`K` triggers `panic!("Op flag K not implemented")` in both `operate` and `operate_no_mutate` before `begin_op` runs, and the only other caller (`generalized_ratchet`) never sets `K`. The `|| flags.contains(OpFlags::K)` term is therefore unreachable. Drop it or leave a note that it's forward-looking for when `K` lands. - -### C4 — Inconsistent element access (`src/strobe.rs`) - -`absorb_and_set` (263), `exchange` (291), and `squeeze` (320) use `self.st.0.get_mut(self.pos).unwrap()`, while the structurally identical `absorb` (250), `copy_state` (278), and `overwrite` (305) use direct indexing `self.st.0[self.pos]`. Both forms panic identically on out-of-range and compile to the same bounds check; the mix is just visual noise. Pick one (direct indexing is the more idiomatic and reads cleaner here). - -### C5 — Typos (Trivial) - -- `src/keccak.rs:29` — "if we' re on a little endian machine". -- `Cargo.toml:41` — "Criteron benches" (→ Criterion). -- `README.md` — "target/crieteron/report" (→ criterion). -- `benches/benches.rs` — `"rachet 16"`, `"meta_rachet 16"` (→ ratchet). - ---- - -## Suggested priority order - -1. **S1** — zeroize `keccak_block` (tiny, closes a real defense-in-depth gap consistent with the crate's own posture). -2. **P1** — chunk the duplex loops (best perf/effort ratio; KATs de-risk it). -3. **C1–C3** — remove/annotate dead code and the stale alignment rationale. -4. **P2** — consider a native `[u64; 25]` state representation (larger refactor). -5. **S2** — add a security caveat to the `serialize_secret_state` docs. -6. **C4/C5** — cosmetic cleanup. - -*No memory-safety or correctness defects were found; the KAT suite passes against the reference implementation and covers streaming, metadata, long inputs, and boundary cases.* From cba7438148cb43e9c98a68eaacbd0a6cb2dd613d Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Mon, 3 Aug 2026 23:13:36 -0400 Subject: [PATCH 16/17] Fix comment on duplex_mut --- src/strobe.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/strobe.rs b/src/strobe.rs index c1c0baa..1d8165b 100644 --- a/src/strobe.rs +++ b/src/strobe.rs @@ -253,10 +253,9 @@ impl Strobe { } /// 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. Data is processed in - /// contiguous chunks of up to `rate - pos` bytes, so the inner loop autovectorizes. This is - /// the shared driver for the mutating specializations of the `duplex` code in the STROBE - /// paper. + /// 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() { From 1f0d38ef8aa9e2e3ea3e27faf8073100fe1fc380 Mon Sep 17 00:00:00 2001 From: Michael Rosenberg Date: Sat, 8 Aug 2026 03:53:59 -0400 Subject: [PATCH 17/17] Remove vestigial repr on keccak state --- src/keccak.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/keccak.rs b/src/keccak.rs index e0b35d2..d135b1b 100644 --- a/src/keccak.rs +++ b/src/keccak.rs @@ -18,7 +18,6 @@ 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],