test(tweak-aead): run the crate in CI and pin AVX2 against portable byte-for-byte - #75
Open
Nexlab-One wants to merge 4 commits into
Open
test(tweak-aead): run the crate in CI and pin AVX2 against portable byte-for-byte#75Nexlab-One wants to merge 4 commits into
Nexlab-One wants to merge 4 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for (i, k) in key.iter_mut().enumerate() { | ||
| *k = i as u8; | ||
| } | ||
| let mut nonce = [0u8; NONCE_BYTES]; |
| let nonce = [0u8; 16]; | ||
| let mut out = [0u8; 4 + 32]; | ||
| let key = [0u8; KEY_BYTES]; | ||
| let nonce = [0u8; NONCE_BYTES]; |
| for (i, k) in key.iter_mut().enumerate() { | ||
| *k = i as u8; | ||
| } | ||
| let mut nonce = [0u8; NONCE_BYTES]; |
🔒 Security Validation ReportGenerated: Wed Jul 29 12:54:49 UTC 2026 📊 Summary
✅ Overall Security Status: PASSEDAll critical security validations passed successfully. 🔍 DetailsThis report covers:
📋 Next Steps✅ Security validation passed. Code is ready for deployment. ✅ Security validation passed! This code meets all security requirements. |
🔍 Pull Request SummaryGenerated: Wed Jul 29 12:58:41 UTC 2026 📋 Validation Results
✅ Overall Status: PASSED🔒 Security Checklist
📝 Review NotesPlease review the security implications of this change carefully. ✅ Automated validation passed! This PR is ready for review. |
…r-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.
…6th 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`.
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.
Nexlab-One
force-pushed
the
test/tweak-aead-coverage
branch
from
July 29, 2026 13:04
48016dc to
021314e
Compare
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.
🔍 Pull Request SummaryGenerated: Wed Jul 29 13:46:15 UTC 2026 📋 Validation Results
✅ Overall Status: PASSED🔒 Security Checklist
📝 Review NotesPlease review the security implications of this change carefully. ✅ Automated validation passed! This PR is ready for review. |
🔒 Security Validation ReportGenerated: Wed Jul 29 13:49:00 UTC 2026 📊 Summary
✅ Overall Security Status: PASSEDAll critical security validations passed successfully. 🔍 DetailsThis report covers:
📋 Next Steps✅ Security validation passed. Code is ready for deployment. ✅ Security validation passed! This code meets all security requirements. |
🔒 Security Validation ReportGenerated: Wed Jul 29 14:10:34 UTC 2026 📊 Summary
✅ Overall Security Status: PASSEDAll critical security validations passed successfully. 🔍 DetailsThis report covers:
📋 Next Steps✅ Security validation passed. Code is ready for deployment. ✅ Security validation passed! This code meets all security requirements. |
🔍 Pull Request SummaryGenerated: Wed Jul 29 14:11:51 UTC 2026 📋 Validation Results
✅ Overall Status: PASSED🔒 Security Checklist
📝 Review NotesPlease review the security implications of this change carefully. ✅ Automated validation passed! This PR is ready for review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
lib-q-tweak-aeaddispatches between an AVX2 keystream (xor_keystream_avx2) and a portable one, but the crate was never acargo testtarget in CI —grep -rn "tweak-aead" .github/workflows/returned exactly one hit, acargo publish --dry-runpackage list incd.yml. Its owntests/roundtrip.rsnever ran, and nothing ever checked that the AVX2 path agrees with the portable one.To be precise about what was and was not true: the AVX2 code was compiled —
ci.yml:76runscargo clippy --all-targets --all-featuresungated. Compiled is not tested.What this adds
tests/simd_equivalence.rs— AVX2 vs portable, byte-for-byte, over varied lengths, keys, nonces and tweaks.simd-avx2.The KAT expected bytes are generated from the portable path (
cargo run -p lib-q-tweak-aead --example dump_tweak_kat, nosimd-avx2), so they are an independent oracle rather than a recording of the AVX2 path. It is also branch-independent —xor_bodypicks AVX2 at runtime, and the differential test can only run on a host with AVX2, so the KAT pins fixed bytes whichever branch executes.Two holes found by adversarial review, both closed
Every output buffer was zeroed, so the suite could not distinguish
ct[i] = pt[i] ^ ks[i]fromct[i] ^= ...— 0 is the XOR identity. Changing both AVX2 write sites to^=left all 12 tests green. This is not contrived:crypto::encryptispuband writes into a caller-suppliedout, so a caller reusing a buffer would silently get wrong ciphertext. Buffers are now pre-filled0xAA; expected bytes unchanged, which is the point.The vector table topped out at 4096 bytes (block indices 0..127), so a divergence first appearing at block >= 128 was invisible. Lengths now reach past the batched loop 256th block.
A third issue was introduced by that second fix and is also corrected: the meta-guard counted (key-set x length) pairs against a threshold of exactly 8 — zero margin — so trimming the pseudorandom key sets from 6 to 2, a pure runtime reduction leaving the deep-counter property intact, tripped it with a misleading message. It now asserts the property it actually cares about:
LENGTHScontains at least one length exercising block index > 256, independent of how many key sets exist.Verified
cargo test -p lib-q-tweak-aead-> 10 passed. With--features simd-avx2-> 13 passed (the 3simd_equivalencetests now genuinely run). Each hole was reproduced first (mutation applied, suite green), then closed, then the same mutation shown failing.Production source is untouched: the only non-test change is inside
#[cfg(test)] mod kat_tests.Not checked
xor_keystream_avx2is correct on a non-AVX2 host — the differential test returns early there; the KAT is the fallback guard.lib-q-duplex-aeaddeclares asimd-avx2feature too; itsavx2.rsreportedly forwards straight toPortable, so it is likely a no-op rather than a real split. Not confirmed.