From b8edee9ee295c3f614add5f42946480d511d5f37 Mon Sep 17 00:00:00 2001 From: Nexlab-One <86677687+Nexlab-One@users.noreply.github.com> Date: Wed, 29 Jul 2026 10:53:40 +0200 Subject: [PATCH 1/4] test(tweak-aead): execute the AVX2 keystream in CI and pin it byte-for-byte `lib-q-tweak-aead`'s `simd-avx2` keystream was COMPILED on every PR (the workspace `cargo clippy --all-targets --all-features` pass in core-validation) but EXECUTED by nothing: no Cargo.toml forwards `lib-q-tweak-aead/simd-avx2`, no workflow enables it, and the repo's only `cargo test --all-features` sits behind rust-build's `run-tests` input, which every caller sets to "false". Adding a feature row alone would not have helped. Every test in tests/roundtrip.rs encrypts and then decrypts through the SAME `xor_body` branch, so CTR's self-inverse cancels any keystream error and the roundtrip still passes. The one test that pinned actual bytes, `kat_encrypt_libq_empty_ad`, has a 4-byte plaintext -- below BLOCK_BYTES=32 -- so it only ever reached the sub-block remainder path, never the AVX2 4-way batched loop. Verified by mutation: changing `block_idx + i as u64` to `block_idx + (i as u64 ^ 1)` in the batched loop left all 8 roundtrip tests AND the 4-byte KAT green. Three changes: - tests/simd_equivalence.rs (new): differential `xor_keystream_avx2` vs `Portable::xor_keystream` (plus an independent naive CTR reference) over 26 lengths x 8 key/nonce sets, all from fixed seeds so failures reproduce. Modelled on lib-q-saturnin/tests/simd_equivalence.rs, including the `if !has_avx2() { return; }` guard -- that is a SAFETY requirement, not style: `xor_keystream_avx2` is `#[target_feature(enable = "avx2")]` and calling it without the CPU feature is UB. Non-vacuity assertions (keystream not all-zero, counter advances across the batch boundary, output depends on key and on nonce) stop it passing on a degenerate keystream, and a `batched_vectors` floor stops a future edit trimming the length table back below 128 bytes, which would silently remove the only coverage that matters. - src/crypto.rs `kat_encrypt_multi_block_293`: 293 bytes = 9 full blocks + 5, which drives two batch iterations, the single-block tail and the remainder in one vector. Unlike the differential test this is branch-INDEPENDENT, so it also covers the case the differential test cannot: on a runner whose CPU lacks AVX2, simd_equivalence.rs returns early, but this KAT still pins the bytes whichever branch `xor_body` took. Expected bytes are generated from the portable path via examples/dump_tweak_kat.rs (extended here), so the KAT is an independent oracle for the AVX2 path rather than a recording of it. - ci.yml: two `test-matrix` rows so the crate is a real `cargo test` target on every PR, with and without `simd-avx2`. The plain row duplicates what the whole-workspace "std" shard already covers, deliberately: that shard names no package, so `grep tweak-aead .github/workflows/` found nothing and the crate read as untested. That invisibility is what produced this audit. No production code changed. Severity is latent, not live: `simd-avx2` is opt-in and nothing in the workspace enables it, so today's blast radius is zero. This is regression insurance on a path that was shipping unexercised. --- .github/workflows/ci.yml | 14 ++ lib-q-tweak-aead/examples/dump_tweak_kat.rs | 41 +++- lib-q-tweak-aead/src/crypto.rs | 60 ++++++ lib-q-tweak-aead/tests/simd_equivalence.rs | 195 ++++++++++++++++++++ 4 files changed, 305 insertions(+), 5 deletions(-) create mode 100644 lib-q-tweak-aead/tests/simd_equivalence.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 846b6c7a..422530a4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -254,6 +254,20 @@ jobs: - name: "lib-q-prf" features: "alloc" package: "lib-q-prf" + # lib-q-tweak-aead's `simd-avx2` keystream was COMPILED on every PR (the workspace + # `cargo clippy --all-targets --all-features` pass in core-validation) but EXECUTED by + # nothing: no Cargo.toml forwards `lib-q-tweak-aead/simd-avx2` and no job enabled it, and + # the only `cargo test --all-features` in the repo (rust-build) is behind `run-tests`, + # which every caller sets to "false". The simd-avx2 row is the one that is genuinely new. + # The plain row is deliberate redundancy with the whole-workspace "std" shard: that shard + # names no package, so `grep tweak-aead .github/workflows/` found nothing and the crate + # read as untested. Making the coverage greppable is the point. + - name: "lib-q-tweak-aead" + features: "std" + package: "lib-q-tweak-aead" + - name: "lib-q-tweak-aead simd-avx2" + features: "std,simd-avx2" + package: "lib-q-tweak-aead" - name: "lib-q-ring-sig pilot-insecure-prf-transcript" features: "pilot-insecure-prf-transcript" package: "lib-q-ring-sig" diff --git a/lib-q-tweak-aead/examples/dump_tweak_kat.rs b/lib-q-tweak-aead/examples/dump_tweak_kat.rs index f741f2cc..d327524c 100644 --- a/lib-q-tweak-aead/examples/dump_tweak_kat.rs +++ b/lib-q-tweak-aead/examples/dump_tweak_kat.rs @@ -1,12 +1,43 @@ -//! Dev-only: print hex for KAT (`cargo run -p lib-q-tweak-aead --example dump_tweak_kat`). +//! Dev-only: print hex for the KATs in `src/crypto.rs` +//! (`cargo run -p lib-q-tweak-aead --example dump_tweak_kat`). +//! +//! Run this WITHOUT `--features simd-avx2` when regenerating: the expected bytes must come from +//! the portable path so that the KAT is an independent oracle for the AVX2 path, not a recording +//! of it. +use lib_q_tweak_aead::params::{ + KEY_BYTES, + NONCE_BYTES, + TAG_BYTES, +}; + fn to_hex(b: &[u8]) -> String { b.iter().map(|x| format!("{:02x}", x)).collect() } +/// Inputs for `kat_encrypt_multi_block_293`, kept in one place so the test and this generator +/// cannot drift apart. See that test for why 293 bytes. +fn multi_block_inputs() -> ([u8; KEY_BYTES], [u8; NONCE_BYTES], &'static [u8], Vec) { + let mut key = [0u8; KEY_BYTES]; + for (i, k) in key.iter_mut().enumerate() { + *k = i as u8; + } + let mut nonce = [0u8; NONCE_BYTES]; + for (i, n) in nonce.iter_mut().enumerate() { + *n = 0x10 + i as u8; + } + let pt: Vec = (0..293).map(|i| i as u8).collect(); + (key, nonce, b"lib-q tweak-aead KAT", pt) +} + fn main() { - let key = [0u8; 32]; - let nonce = [0u8; 16]; - let mut out = [0u8; 4 + 32]; + let key = [0u8; KEY_BYTES]; + let nonce = [0u8; NONCE_BYTES]; + let mut out = [0u8; 4 + TAG_BYTES]; lib_q_tweak_aead::crypto::encrypt(&key, &nonce, b"", b"libQ", &mut out).unwrap(); - println!("{}", to_hex(&out)); + println!("kat_encrypt_libq_empty_ad = {}", to_hex(&out)); + + let (key, nonce, ad, pt) = multi_block_inputs(); + let mut out = vec![0u8; pt.len() + TAG_BYTES]; + lib_q_tweak_aead::crypto::encrypt(&key, &nonce, ad, &pt, &mut out).unwrap(); + println!("kat_encrypt_multi_block_293 = {}", to_hex(&out)); } diff --git a/lib-q-tweak-aead/src/crypto.rs b/lib-q-tweak-aead/src/crypto.rs index f609b094..8c4078ec 100644 --- a/lib-q-tweak-aead/src/crypto.rs +++ b/lib-q-tweak-aead/src/crypto.rs @@ -166,6 +166,11 @@ fn compute_tag( #[cfg(test)] mod kat_tests { use super::encrypt; + use crate::params::{ + KEY_BYTES, + NONCE_BYTES, + TAG_BYTES, + }; #[test] fn kat_encrypt_libq_empty_ad() { @@ -182,4 +187,59 @@ mod kat_tests { .as_slice() ); } + + /// Multi-block KAT: 293 bytes = 9 full 32-byte blocks + a 5-byte remainder. + /// + /// This length is chosen to drive **every** loop of the AVX2 keystream in one shot: + /// two iterations of the 4-way batched loop (blocks 0..4 and 4..8), one iteration of the + /// single-block tail loop (block 8), and the sub-block remainder (block 9). The pre-existing + /// `kat_encrypt_libq_empty_ad` vector is 4 bytes, so it only ever reached the remainder path + /// — it cannot detect a batched-loop defect. + /// + /// Why a KAT and not only `tests/simd_equivalence.rs`: this test is **branch-independent**. + /// `xor_body` picks AVX2 vs portable at runtime, and the differential test can only run when + /// the host CPU actually has AVX2 (it returns early otherwise). This one pins fixed bytes + /// whichever branch runs, so a build that takes the AVX2 branch must produce exactly the + /// portable answer or fail here. Regenerate with + /// `cargo run -p lib-q-tweak-aead --example dump_tweak_kat` (no `simd-avx2`, so the expected + /// bytes stay an independent oracle rather than a recording of the AVX2 path). + #[test] + fn kat_encrypt_multi_block_293() { + let mut key = [0u8; KEY_BYTES]; + for (i, k) in key.iter_mut().enumerate() { + *k = i as u8; + } + let mut nonce = [0u8; NONCE_BYTES]; + for (i, n) in nonce.iter_mut().enumerate() { + *n = 0x10 + i as u8; + } + let ad = b"lib-q tweak-aead KAT"; + let mut pt = [0u8; 293]; + for (i, p) in pt.iter_mut().enumerate() { + *p = i as u8; + } + + let mut out = [0u8; 293 + TAG_BYTES]; + encrypt(&key, &nonce, ad, &pt, &mut out).unwrap(); + + let expected = hex::decode(concat!( + "394feaffac29c1b3eb0b999fd7915ebae93b036d675a4829cac0c823eabf8b0cd35eaf556d6f60a7", + "b81a1d87d5cac535d9338ae11bffac70912a498436240736c865f2c75f7277b3278eb2fba75c0920", + "dd07dbd0ca8f9605f8630447de31b33ccd9970d50ed8497ae9de95753ef3a5a03c75300a178859b1", + "3da26b8a0c1e60046fd0275b8c6b4c1711978cfeee6a54b3c893eb8546f9dcadbd061257d27337dc", + "e57145a13903fa215fae05118c49ed9ead64e404adbdcee09be5cf9749fbda26493e58cdf04acc2b", + "f42bfae1e8f27a6e6d70ffbc553108687a00469f36b9d9de48d5ce5aaa24b8999e953b134b8b380a", + "2ae1fb58bc9612471967abe0e1798c7dcf5beb371c0e570e156e23ac52532a6eb5a0ff0045467082", + "c9000e6e71fd3bafe22deb597f5cb1345ef0fb0301b6f65f43ee1138064c57033a7e79826ad7831b", + "c4980fd1ce", + )) + .unwrap(); + assert_eq!(out.len(), expected.len()); + assert_eq!( + out.as_slice(), + expected.as_slice(), + "multi-block ciphertext+tag mismatch; first differing byte at index {:?}", + out.iter().zip(expected.iter()).position(|(a, b)| a != b) + ); + } } diff --git a/lib-q-tweak-aead/tests/simd_equivalence.rs b/lib-q-tweak-aead/tests/simd_equivalence.rs new file mode 100644 index 00000000..9b4db4ed --- /dev/null +++ b/lib-q-tweak-aead/tests/simd_equivalence.rs @@ -0,0 +1,195 @@ +//! Differential test: the AVX2 keystream must equal the portable keystream, byte for byte. +//! +//! `crypto::xor_body` chooses between `simd::avx2::xor_keystream_avx2` and +//! `Portable::xor_keystream` at runtime. Byte-for-byte equality of the two IS the AVX2 path's +//! entire correctness contract, and before this file nothing in the workspace tested it. +//! +//! `tests/roundtrip.rs` structurally cannot: every one of its tests encrypts and then decrypts +//! through the *same* `xor_body` branch, so CTR self-inverse cancels any keystream error and the +//! roundtrip still passes on a completely wrong keystream. (Verified: injecting a counter fault +//! into the batched loop left all 8 roundtrip tests green.) +//! +//! Lengths >= `4 * BLOCK_BYTES` (128) are the ones that carry the signal — only they enter the +//! AVX2 4-way batched loop. Shorter inputs fall through to the same scalar `keystream_block` +//! tail the portable implementation uses, so they can never disagree. +//! +//! All data is generated from FIXED seeds (no clock, no system entropy) so any failure +//! reproduces exactly from the printed set index and length. +#![cfg(all(feature = "simd-avx2", target_arch = "x86_64"))] + +use lib_q_tweak_aead::block::keystream_block; +use lib_q_tweak_aead::params::{ + BLOCK_BYTES, + KEY_BYTES, + NONCE_BYTES, +}; +use lib_q_tweak_aead::simd::avx2::xor_keystream_avx2; +use lib_q_tweak_aead::simd::runtime::has_avx2; +use lib_q_tweak_aead::simd::{ + Portable, + TweakAeadStreamOps, +}; + +/// Every length at which `xor_keystream_avx2` changes which of its three loops runs: empty, +/// sub-block, exact block multiples, block +/- 1, the 4-block batch boundary (128) and its +/// neighbours, several whole batches, and a long buffer (4096 = 32 batches). +const LENGTHS: &[usize] = &[ + 0, 1, 2, 15, 31, 32, 33, 63, 64, 65, 95, 96, 97, 127, 128, 129, 130, 159, 160, 161, 255, 256, + 257, 384, 1000, 4096, +]; + +/// Deterministic filler (fixed seed in, same bytes out — never clock- or entropy-seeded). +fn fill_deterministic(seed: u64, out: &mut [u8]) { + let mut x = seed; + for b in out { + x = x + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + *b = (x >> 56) as u8; + } +} + +/// (key, nonce) pairs: both degenerate ends (all-zero, all-0xFF) plus six pseudorandom sets. +fn key_nonce_sets() -> Vec<([u8; KEY_BYTES], [u8; NONCE_BYTES])> { + let mut sets = vec![ + ([0x00u8; KEY_BYTES], [0x00u8; NONCE_BYTES]), + ([0xFFu8; KEY_BYTES], [0xFFu8; NONCE_BYTES]), + ]; + for i in 0..6u64 { + let mut key = [0u8; KEY_BYTES]; + let mut nonce = [0u8; NONCE_BYTES]; + fill_deterministic(0x1000 + i, &mut key); + fill_deterministic(0x2000 + i, &mut nonce); + sets.push((key, nonce)); + } + sets +} + +/// Third reference, written straight from the CTR definition (block index = byte offset / +/// `BLOCK_BYTES`) and sharing no loop structure with either implementation under test. +fn naive_xor_keystream(key: &[u8; KEY_BYTES], nonce: &[u8; NONCE_BYTES], pt: &[u8], ct: &mut [u8]) { + for (b, chunk) in pt.chunks(BLOCK_BYTES).enumerate() { + let ks = keystream_block(key, nonce, b as u64); + for (j, &p) in chunk.iter().enumerate() { + ct[b * BLOCK_BYTES + j] = p ^ ks[j]; + } + } +} + +fn first_diff(a: &[u8], b: &[u8]) -> Option { + a.iter().zip(b.iter()).position(|(x, y)| x != y) +} + +#[test] +fn avx2_keystream_matches_portable_over_lengths_and_keys() { + // SAFETY GATE: `xor_keystream_avx2` is `#[target_feature(enable = "avx2")]`; calling it on a + // CPU without AVX2 is undefined behaviour. Same idiom as + // `lib-q-saturnin/tests/simd_equivalence.rs` and `lib-q-hqc/tests/vect_mul_equivalence.rs`. + if !has_avx2() { + return; + } + + let mut batched_vectors = 0usize; + for (set_idx, (key, nonce)) in key_nonce_sets().iter().enumerate() { + for &len in LENGTHS { + let mut pt = vec![0u8; len]; + fill_deterministic(0x3000 + (set_idx as u64) * 8192 + len as u64, &mut pt); + + let mut avx2_out = vec![0u8; len]; + // SAFETY: guarded by the `has_avx2()` check above; `pt` and `avx2_out` are equal length. + unsafe { + xor_keystream_avx2(key, nonce, &pt, &mut avx2_out); + } + + let mut portable_out = vec![0u8; len]; + ::xor_keystream(key, nonce, &pt, &mut portable_out); + + let mut naive_out = vec![0u8; len]; + naive_xor_keystream(key, nonce, &pt, &mut naive_out); + + assert_eq!( + avx2_out, + portable_out, + "AVX2 != portable (key/nonce set {}, len {}, first differing byte {:?})", + set_idx, + len, + first_diff(&avx2_out, &portable_out) + ); + assert_eq!( + portable_out, + naive_out, + "portable != naive CTR reference (key/nonce set {}, len {}, first differing byte \ + {:?})", + set_idx, + len, + first_diff(&portable_out, &naive_out) + ); + + if len >= 4 * BLOCK_BYTES { + batched_vectors += 1; + } + } + } + + // Guard against a future edit trimming `LENGTHS` back below the batch boundary, which would + // leave this test green while testing nothing that the scalar tail does not already cover. + assert!( + batched_vectors >= 32, + "vector table no longer exercises the AVX2 4-way batched loop: only {} vectors are >= {} \ + bytes", + batched_vectors, + 4 * BLOCK_BYTES + ); +} + +#[test] +fn avx2_keystream_is_not_degenerate() { + // SAFETY GATE: see `avx2_keystream_matches_portable_over_lengths_and_keys`. + if !has_avx2() { + return; + } + + let key = [0x5Au8; KEY_BYTES]; + let nonce = [0xA5u8; NONCE_BYTES]; + // 8 blocks = two full 4-way batches, so the counter also has to advance ACROSS a batch. + let zeros = vec![0u8; 8 * BLOCK_BYTES]; + + let mut ks = vec![0u8; zeros.len()]; + // SAFETY: guarded by the `has_avx2()` check above; buffers are equal length. + unsafe { + xor_keystream_avx2(&key, &nonce, &zeros, &mut ks); + } + + assert!( + ks.iter().any(|&b| b != 0), + "keystream is all-zero: XOR would be the identity and every equivalence assertion above \ + would pass vacuously" + ); + for b in 1..8 { + assert_ne!( + ks[(b - 1) * BLOCK_BYTES..b * BLOCK_BYTES], + ks[b * BLOCK_BYTES..(b + 1) * BLOCK_BYTES], + "block {} equals block {}: the block counter is not advancing", + b, + b - 1 + ); + } + + let mut nonce2 = nonce; + nonce2[0] ^= 1; + let mut ks_other_nonce = vec![0u8; zeros.len()]; + // SAFETY: guarded by the `has_avx2()` check above; buffers are equal length. + unsafe { + xor_keystream_avx2(&key, &nonce2, &zeros, &mut ks_other_nonce); + } + assert_ne!(ks, ks_other_nonce, "keystream does not depend on the nonce"); + + let mut key2 = key; + key2[0] ^= 1; + let mut ks_other_key = vec![0u8; zeros.len()]; + // SAFETY: guarded by the `has_avx2()` check above; buffers are equal length. + unsafe { + xor_keystream_avx2(&key2, &nonce, &zeros, &mut ks_other_key); + } + assert_ne!(ks, ks_other_key, "keystream does not depend on the key"); +} From 98d791130f4f17e36feae5f8ae5c3fa8e4dc156f Mon Sep 17 00:00:00 2001 From: Nexlab-One <86677687+Nexlab-One@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:18:10 +0200 Subject: [PATCH 2/4] test(tweak-aead): raise the vector ceiling past the batched loop's 256th block, and stop testing XOR against zeroed buffers d5aee03 made `xor_keystream_avx2` a real test target and pinned it against the portable keystream. That claim holds, but the suite it added had two blind spots that mutation testing walks straight through. Both were reproduced before being fixed, and each fix was re-checked against the same mutation. HOLE 1 -- the vector table had no ceiling. `LENGTHS` topped out at 4096 bytes = 128 blocks, and the 293-byte KAT reaches block 9, so no test drove the block counter above index 127. Truncating the batched loop's counter to 8 bits (`block_idx + i as u64` -> `((block_idx + i as u64) as u8) as u64`, src/simd/avx2.rs:44) left all 12 tests green. That exact mutation is contrived -- the counter is a plain `u64` handed to a scalar setup fn -- but the property is not: a vectorised-counter rewrite (broadcasting four lanes of `block_idx` into a `__m256i` instead of calling `setup_state_pre_f1600` four times) is precisely where a narrowed counter would appear, and the table could not have caught it. The obvious fix is wrong, which is why the constant carries the arithmetic in a doc comment. "Add a length above 8192 so block index >= 256 is exercised" does not work: at 8256 bytes (258 blocks) the batched loop runs `while block_idx + 4 <= 258`, so it exits at block_idx = 256 and hands blocks 256 and 257 to the SCALAR TAIL -- which calls the unmutated `keystream_block`. The counter reaches 257 and the mutation still hides. Verified: with 8256 in the table the whole suite stayed green under the mutation. The batched loop only touches index 256 once `full_blocks >= 260`. So the added vector is 8357 = 261 blocks + 5 (batch covers 256..259, tail takes 260, remainder takes 261), plus 8192 for the boundary itself (highest batched index exactly 255). The new `deep_counter_vectors` guard is phrased as `batched_blocks(len) > 256` rather than a byte threshold, so a future edit cannot satisfy it with a length that only reaches 256 via the tail -- the trap this commit just fell into. HOLE 2 -- every output buffer started zeroed, so nothing pinned assignment. Zero is the identity for XOR, so `ct[i] = pt[i] ^ ks[i]` and `ct[i] ^= ...` are byte-identical on a fresh buffer. Changing BOTH AVX2 write sites to `^=` left all 12 tests green. Unlike hole 1 this is not contrived: `crypto::encrypt` is `pub` in a `pub mod crypto` and writes into a caller-supplied `out`, so a caller reusing one buffer across messages -- the obvious way to avoid reallocating -- would get each ciphertext XORed with the previous one. For a stream cipher that hands the attacker the XOR of two plaintexts. Three separate pins, deliberately overlapping: - the differential loop now prefills each of the three buffers with a DIFFERENT pattern (0xAA / 0x55 / 0x33), so an accumulate defect in any one of them carries its own pattern out and diverges from the other two; - `avx2_output_is_assigned_not_xor_accumulated` states the property directly and without reference to the portable path -- same input into a clean and a dirty buffer must give the same bytes -- over 0xAA/0x55/0xFF and a length that hits all three write sites; - both KATs in src/crypto.rs now encrypt into 0xAA-filled buffers. Expected bytes unchanged; that IS the assertion. These run through the public `encrypt`, so they hold whichever branch `xor_body` takes -- including on runners without AVX2, where tests/simd_equivalence.rs returns early. Also widened the plaintext seed stride from 8192 to 100_000: with lengths now exceeding 8192, `set_idx * 8192 + len` was no longer injective, so a reported "set N, len L" could have named two different plaintexts. Test-side only. src/simd/avx2.rs is untouched -- the mutations above were temporary probes, reverted before commit; the src/crypto.rs change is confined to `#[cfg(test)] mod kat_tests`. --- lib-q-tweak-aead/src/crypto.rs | 15 +- lib-q-tweak-aead/tests/simd_equivalence.rs | 171 ++++++++++++++++++++- 2 files changed, 177 insertions(+), 9 deletions(-) diff --git a/lib-q-tweak-aead/src/crypto.rs b/lib-q-tweak-aead/src/crypto.rs index 8c4078ec..7eb2031a 100644 --- a/lib-q-tweak-aead/src/crypto.rs +++ b/lib-q-tweak-aead/src/crypto.rs @@ -178,7 +178,10 @@ mod kat_tests { let nonce = [0u8; 16]; let ad = b""; let pt = b"libQ"; - let mut out = [0u8; 4 + 32]; + // Deliberately DIRTY (see `kat_encrypt_multi_block_293`): `encrypt` must assign into the + // caller's buffer, not XOR into whatever it already held. A zeroed buffer cannot tell the + // two apart, since 0 is the XOR identity. + let mut out = [0xAAu8; 4 + 32]; encrypt(&key, &nonce, ad, pt, &mut out).unwrap(); assert_eq!( out.as_slice(), @@ -219,7 +222,15 @@ mod kat_tests { *p = i as u8; } - let mut out = [0u8; 293 + TAG_BYTES]; + // Deliberately DIRTY, not zeroed. `encrypt` is `pub` and writes into a caller-supplied + // `out`; a caller reusing one buffer across messages is the obvious way to avoid + // reallocating. A zeroed buffer cannot distinguish `ct[i] = pt[i] ^ ks[i]` from + // `ct[i] ^= pt[i] ^ ks[i]` (0 is the XOR identity), and every output buffer in this + // suite used to be zeroed: changing both AVX2 write sites to `^=` left all 12 tests + // green. Prefilling here pins assignment semantics on whichever branch `xor_body` takes, + // so it holds on AVX2-less runners too. The expected bytes are unchanged — that is the + // point. + let mut out = [0xAAu8; 293 + TAG_BYTES]; encrypt(&key, &nonce, ad, &pt, &mut out).unwrap(); let expected = hex::decode(concat!( diff --git a/lib-q-tweak-aead/tests/simd_equivalence.rs b/lib-q-tweak-aead/tests/simd_equivalence.rs index 9b4db4ed..d5ff89c6 100644 --- a/lib-q-tweak-aead/tests/simd_equivalence.rs +++ b/lib-q-tweak-aead/tests/simd_equivalence.rs @@ -13,6 +13,15 @@ //! AVX2 4-way batched loop. Shorter inputs fall through to the same scalar `keystream_block` //! tail the portable implementation uses, so they can never disagree. //! +//! Two properties are pinned here that a differential test does NOT get for free, and that this +//! file did not have when it was first written: +//! +//! 1. **The counter is driven past block index 255** (`DEEP_COUNTER_BYTES`). A table that stops +//! at 4096 bytes only ever reaches index 127, so any narrowing of the block counter below +//! 64 bits is invisible. +//! 2. **Output buffers start dirty.** With a zeroed `ct`, assignment and XOR-accumulation are +//! indistinguishable, because 0 is the XOR identity. +//! //! All data is generated from FIXED seeds (no clock, no system entropy) so any failure //! reproduces exactly from the printed set index and length. #![cfg(all(feature = "simd-avx2", target_arch = "x86_64"))] @@ -30,12 +39,75 @@ use lib_q_tweak_aead::simd::{ TweakAeadStreamOps, }; +/// Number of blocks the 4-way BATCHED loop handles for a buffer of `len` bytes. +/// +/// `xor_keystream_avx2` runs `while block_idx + 4 <= full_blocks`, so the batched loop covers +/// blocks `0..batched_blocks(len)` and everything above that is handed to the scalar tail +/// (which calls the same `keystream_block` the portable path does). Getting this distinction +/// right is what `DEEP_COUNTER_BYTES` turns on: a length can drive the *counter* past 255 while +/// the *batched loop* never sees an index above 255. +fn batched_blocks(len: usize) -> usize { + (len / BLOCK_BYTES) / 4 * 4 +} + +/// Smallest interesting length whose 4-way BATCHED loop is entered with `block_idx` = 256, i.e. +/// the only place a block counter narrowed below 64 bits can be caught. +/// +/// 8357 = 261 full blocks + 5. `batched_blocks(8357)` = 260, so the final batch covers blocks +/// 256..259 — inside the batched loop — then the scalar tail takes block 260 and the remainder +/// takes 261. +/// +/// The arithmetic matters, and the obvious value is wrong. 8256 bytes (258 blocks) *does* drive +/// the counter to 257, but `batched_blocks(8256)` = 256: indices 256 and 257 are handled by the +/// scalar tail, not the batched loop. Truncating `block_idx + i as u64` to `u8` at +/// `src/simd/avx2.rs:44` is therefore still invisible at 8256 — verified, the whole suite stayed +/// green. The batched loop needs `full_blocks >= 260` before it touches index 256 at all. +/// +/// This is not decoration. The table used to top out at 4096 = 128 blocks, so *any* defect that +/// first shows above block index 127 was invisible — including the whole class of "the counter +/// is materialised in something narrower than u64", which is what a vectorised-counter rewrite +/// (broadcasting four lanes of `block_idx` into a `__m256i` instead of calling the scalar +/// `setup_state_pre_f1600` four times) would introduce. +const DEEP_COUNTER_BYTES: usize = 8357; + +/// The boundary itself: `batched_blocks(8192)` = 256, so the batched loop's highest index is +/// exactly 255. Pairs with `DEEP_COUNTER_BYTES` to separate an off-by-one at the boundary from a +/// wholesale wrap past it. +const BOUNDARY_COUNTER_BYTES: usize = 8192; + /// Every length at which `xor_keystream_avx2` changes which of its three loops runs: empty, /// sub-block, exact block multiples, block +/- 1, the 4-block batch boundary (128) and its -/// neighbours, several whole batches, and a long buffer (4096 = 32 batches). +/// neighbours, several whole batches, a long buffer (4096 = 32 batches), and finally the two +/// lengths that push the block counter to and past index 255 (see `DEEP_COUNTER_BYTES`). const LENGTHS: &[usize] = &[ - 0, 1, 2, 15, 31, 32, 33, 63, 64, 65, 95, 96, 97, 127, 128, 129, 130, 159, 160, 161, 255, 256, - 257, 384, 1000, 4096, + 0, + 1, + 2, + 15, + 31, + 32, + 33, + 63, + 64, + 65, + 95, + 96, + 97, + 127, + 128, + 129, + 130, + 159, + 160, + 161, + 255, + 256, + 257, + 384, + 1000, + 4096, + BOUNDARY_COUNTER_BYTES, + DEEP_COUNTER_BYTES, ]; /// Deterministic filler (fixed seed in, same bytes out — never clock- or entropy-seeded). @@ -90,21 +162,30 @@ fn avx2_keystream_matches_portable_over_lengths_and_keys() { } let mut batched_vectors = 0usize; + let mut deep_counter_vectors = 0usize; for (set_idx, (key, nonce)) in key_nonce_sets().iter().enumerate() { for &len in LENGTHS { let mut pt = vec![0u8; len]; - fill_deterministic(0x3000 + (set_idx as u64) * 8192 + len as u64, &mut pt); + // Stride > max(LENGTHS) so (set_idx, len) -> seed stays injective: a reported + // "set N, len L" always names exactly one plaintext. + fill_deterministic(0x3000 + (set_idx as u64) * 100_000 + len as u64, &mut pt); - let mut avx2_out = vec![0u8; len]; + // Every output buffer starts DIRTY, with a different pattern per implementation. + // A zeroed buffer cannot tell `ct[i] = pt[i] ^ ks[i]` apart from `ct[i] ^= ...`, + // because 0 is the XOR identity — and `crypto::encrypt` is `pub` and takes a + // caller-supplied `out`, so a reused or uninitialised buffer is a real caller. With + // distinct prefills, any implementation that accumulates instead of assigning carries + // its own pattern out and diverges from the other two. + let mut avx2_out = vec![0xAAu8; len]; // SAFETY: guarded by the `has_avx2()` check above; `pt` and `avx2_out` are equal length. unsafe { xor_keystream_avx2(key, nonce, &pt, &mut avx2_out); } - let mut portable_out = vec![0u8; len]; + let mut portable_out = vec![0x55u8; len]; ::xor_keystream(key, nonce, &pt, &mut portable_out); - let mut naive_out = vec![0u8; len]; + let mut naive_out = vec![0x33u8; len]; naive_xor_keystream(key, nonce, &pt, &mut naive_out); assert_eq!( @@ -128,6 +209,9 @@ fn avx2_keystream_matches_portable_over_lengths_and_keys() { if len >= 4 * BLOCK_BYTES { batched_vectors += 1; } + if batched_blocks(len) > 256 { + deep_counter_vectors += 1; + } } } @@ -140,6 +224,79 @@ fn avx2_keystream_matches_portable_over_lengths_and_keys() { batched_vectors, 4 * BLOCK_BYTES ); + + // Guard the table's CEILING, not just its floor. The `batched_vectors` floor above only + // proves the batched loop is entered; it says nothing about how far the block counter is + // driven inside it. With a 4096-byte maximum the batched loop never saw an index above 127, + // so a counter narrowed to 8 bits was indistinguishable from the correct `u64`. + // + // Deliberately phrased in `batched_blocks`, not bytes: a length can drive the counter past + // 255 while leaving every index above 255 to the scalar tail, where the mutation this guard + // exists for does not live. See `DEEP_COUNTER_BYTES`. + assert!( + deep_counter_vectors >= 8, + "vector table no longer drives the AVX2 BATCHED loop past block index 255: only {} \ + vectors reach it, so a block counter narrowed below 64 bits would pass unnoticed \ + (needs len >= {} bytes, i.e. >= 260 full blocks)", + deep_counter_vectors, + 260 * BLOCK_BYTES + ); +} + +/// `xor_keystream_avx2` must **assign** into `ct`, never XOR-accumulate into it. +/// +/// The distinction is invisible to any test whose output buffer starts zeroed, because 0 is the +/// identity for XOR: `0 ^ x == x`, so `ct[i] = x` and `ct[i] ^= x` agree byte for byte. Every +/// buffer in this crate's test suite used to be freshly zeroed, and changing both AVX2 write +/// sites to `^=` left all 12 tests green. +/// +/// It matters because the buffer is the CALLER's. `crypto::encrypt` is `pub` in a `pub mod +/// crypto` and writes into a caller-supplied `out`; a caller that reuses one buffer across +/// messages — the obvious thing to do to avoid reallocating — would get ciphertext silently +/// XORed with the previous message's, which for a stream cipher leaks the XOR of two plaintexts. +/// +/// Stated without reference to the portable path on purpose: encrypting the same input into a +/// clean and a dirty buffer must give the same bytes, whatever those bytes are. +#[test] +fn avx2_output_is_assigned_not_xor_accumulated() { + // SAFETY GATE: see `avx2_keystream_matches_portable_over_lengths_and_keys`. + if !has_avx2() { + return; + } + + let key = [0x11u8; KEY_BYTES]; + let nonce = [0x22u8; NONCE_BYTES]; + + // 297 bytes = 9 full blocks + 9: two iterations of the 4-way batched loop (blocks 0..8), one + // iteration of the single-block tail (block 8) and a 9-byte remainder (block 9), so all + // three of the function's write sites run over dirty memory. + let mut pt = vec![0u8; 297]; + fill_deterministic(0x0000_D147, &mut pt); + + let mut from_clean = vec![0u8; pt.len()]; + // SAFETY: guarded by the `has_avx2()` check above; buffers are equal length. + unsafe { + xor_keystream_avx2(&key, &nonce, &pt, &mut from_clean); + } + + // 0xAA / 0x55 are complements, so between them every bit position is set in one run: an + // accumulate defect cannot cancel out for both. + for dirt in [0xAAu8, 0x55u8, 0xFFu8] { + let mut from_dirty = vec![dirt; pt.len()]; + // SAFETY: guarded by the `has_avx2()` check above; buffers are equal length. + unsafe { + xor_keystream_avx2(&key, &nonce, &pt, &mut from_dirty); + } + assert_eq!( + from_dirty, + from_clean, + "output depends on what was already in `ct` (prefill {:#04x}, first differing byte \ + {:?}): the AVX2 path is XOR-accumulating into the caller's buffer instead of \ + assigning", + dirt, + first_diff(&from_dirty, &from_clean) + ); + } } #[test] From 021314ef9a70dba312e76daaf4cfd5cb28878d9c Mon Sep 17 00:00:00 2001 From: Nexlab-One <86677687+Nexlab-One@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:09:24 +0200 Subject: [PATCH 3/4] test(tweak-aead): decouple deep-counter guard from key/nonce set count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit deep_counter_vectors counted (key/nonce set, length) PAIRS, not lengths in LENGTHS: with 8 key/nonce sets and the single qualifying length (8357, the only entry whose batched_blocks() exceeds 256), the count was exactly 8 against an `>= 8` threshold — zero margin. Any unrelated trim of key_nonce_sets()'s loop range (fewer key/nonce sets, nothing to do with which lengths are tested) would trip the guard, and its message ("only N vectors reach it... needs len >= N bytes") would misdescribe the cause as lost length/coverage rather than fewer key/nonce sets. Assert directly on LENGTHS instead: at least one entry drives batched_blocks(len) past 256. This is the actual property the guard exists to protect, and it no longer depends on key_nonce_sets()'s cardinality. Verified the reduction (0..6u64 -> 0..2u64) still passes under the new assertion. --- lib-q-tweak-aead/tests/simd_equivalence.rs | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/lib-q-tweak-aead/tests/simd_equivalence.rs b/lib-q-tweak-aead/tests/simd_equivalence.rs index d5ff89c6..f6317fba 100644 --- a/lib-q-tweak-aead/tests/simd_equivalence.rs +++ b/lib-q-tweak-aead/tests/simd_equivalence.rs @@ -162,7 +162,6 @@ fn avx2_keystream_matches_portable_over_lengths_and_keys() { } let mut batched_vectors = 0usize; - let mut deep_counter_vectors = 0usize; for (set_idx, (key, nonce)) in key_nonce_sets().iter().enumerate() { for &len in LENGTHS { let mut pt = vec![0u8; len]; @@ -209,9 +208,6 @@ fn avx2_keystream_matches_portable_over_lengths_and_keys() { if len >= 4 * BLOCK_BYTES { batched_vectors += 1; } - if batched_blocks(len) > 256 { - deep_counter_vectors += 1; - } } } @@ -230,15 +226,17 @@ fn avx2_keystream_matches_portable_over_lengths_and_keys() { // driven inside it. With a 4096-byte maximum the batched loop never saw an index above 127, // so a counter narrowed to 8 bits was indistinguishable from the correct `u64`. // - // Deliberately phrased in `batched_blocks`, not bytes: a length can drive the counter past - // 255 while leaving every index above 255 to the scalar tail, where the mutation this guard - // exists for does not live. See `DEEP_COUNTER_BYTES`. + // This checks `LENGTHS` itself, not how many (key/nonce set, length) pairs it produces: the + // property that matters is that at least one length reaches past block index 255, and that + // does not get any truer or falser depending on how many key/nonce sets `key_nonce_sets()` + // happens to return. Counting pairs coupled the guard to `key_nonce_sets()`'s cardinality for + // no reason — trimming that function's loop for runtime would trip this assertion while + // `LENGTHS` and its deep-counter coverage stayed exactly as they were. assert!( - deep_counter_vectors >= 8, - "vector table no longer drives the AVX2 BATCHED loop past block index 255: only {} \ - vectors reach it, so a block counter narrowed below 64 bits would pass unnoticed \ - (needs len >= {} bytes, i.e. >= 260 full blocks)", - deep_counter_vectors, + LENGTHS.iter().any(|&len| batched_blocks(len) > 256), + "no length in LENGTHS drives the AVX2 BATCHED loop past block index 255: a block counter \ + narrowed below 64 bits would pass unnoticed (needs a length with batched_blocks(len) > \ + 256, i.e. >= 260 full blocks == {} bytes)", 260 * BLOCK_BYTES ); } From 81422fdab64c8643413829534c89ced6649406f6 Mon Sep 17 00:00:00 2001 From: Nexlab-One <86677687+Nexlab-One@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:15:38 +0200 Subject: [PATCH 4/4] test(tweak-aead): mark the KAT nonces as deliberate for CodeQL CodeQL's rust/hard-coded-cryptographic-value fired at critical severity on three sites added by this branch: the multi-block KAT's nonce in crypto.rs and the two in the regeneration example. All three are known-answer test vectors -- a KAT pins fixed inputs to fixed outputs, so a fresh nonce would make the expected bytes unverifiable, which defeats the purpose. None is reachable from a production path; the example only regenerates the expected bytes and its inputs must match the test exactly or the two drift apart. Suppress narrowly, per site, each with the reason stated inline rather than silencing the query repo-wide or excluding tests from analysis -- the query is right in general and should keep firing everywhere else. Note the pre-existing kat_encrypt_libq_empty_ad vector does the same thing with `let nonce = [0u8; 16]`; CodeQL flags the loop-constructed form but not the bare literal, which is why this surfaced only now. Verified: 13 passed with --features simd-avx2, fmt clean, clippy clean. --- lib-q-tweak-aead/examples/dump_tweak_kat.rs | 6 ++++++ lib-q-tweak-aead/src/crypto.rs | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/lib-q-tweak-aead/examples/dump_tweak_kat.rs b/lib-q-tweak-aead/examples/dump_tweak_kat.rs index d327524c..92fe8ae0 100644 --- a/lib-q-tweak-aead/examples/dump_tweak_kat.rs +++ b/lib-q-tweak-aead/examples/dump_tweak_kat.rs @@ -21,6 +21,9 @@ fn multi_block_inputs() -> ([u8; KEY_BYTES], [u8; NONCE_BYTES], &'static [u8], V for (i, k) in key.iter_mut().enumerate() { *k = i as u8; } + // Fixed by construction: this example regenerates the KAT expected bytes, so its inputs must + // match `kat_encrypt_multi_block_293` exactly. Never used to encrypt real data. + // codeql[rust/hard-coded-cryptographic-value] let mut nonce = [0u8; NONCE_BYTES]; for (i, n) in nonce.iter_mut().enumerate() { *n = 0x10 + i as u8; @@ -30,7 +33,10 @@ fn multi_block_inputs() -> ([u8; KEY_BYTES], [u8; NONCE_BYTES], &'static [u8], V } fn main() { + // Same fixed inputs as the pre-existing `kat_encrypt_libq_empty_ad` vector, for the same + // reason: this regenerates that KAT, it does not encrypt anything real. let key = [0u8; KEY_BYTES]; + // codeql[rust/hard-coded-cryptographic-value] let nonce = [0u8; NONCE_BYTES]; let mut out = [0u8; 4 + TAG_BYTES]; lib_q_tweak_aead::crypto::encrypt(&key, &nonce, b"", b"libQ", &mut out).unwrap(); diff --git a/lib-q-tweak-aead/src/crypto.rs b/lib-q-tweak-aead/src/crypto.rs index 7eb2031a..8a48109a 100644 --- a/lib-q-tweak-aead/src/crypto.rs +++ b/lib-q-tweak-aead/src/crypto.rs @@ -212,6 +212,10 @@ mod kat_tests { for (i, k) in key.iter_mut().enumerate() { *k = i as u8; } + // A known-answer test pins fixed inputs to fixed outputs; a fresh nonce would make the + // expected bytes below unverifiable, which is the whole point of a KAT. This value never + // leaves the test module and is not reachable from any production path. + // codeql[rust/hard-coded-cryptographic-value] let mut nonce = [0u8; NONCE_BYTES]; for (i, n) in nonce.iter_mut().enumerate() { *n = 0x10 + i as u8;