diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index a059ac01a..ffe0b5938 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -39,6 +39,8 @@ jobs: - ws_registry_record - mq_registry_blob - redis_rdb_load + - term_fst_sidecar + - acl_keyspec steps: - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@nightly @@ -103,6 +105,8 @@ jobs: - ws_registry_record - mq_registry_blob - redis_rdb_load + - term_fst_sidecar + - acl_keyspec steps: - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@nightly diff --git a/.gitignore b/.gitignore index 37d61d64c..c3ca3ad88 100644 --- a/.gitignore +++ b/.gitignore @@ -79,7 +79,14 @@ moon_*.log ssh .qdrant-initialized libnull.rlib -fuzz +# Ignore fuzzing BUILD OUTPUT, not the fuzzers themselves. A bare `fuzz` silently +# ignored every newly-added target under fuzz/fuzz_targets/ — already-tracked +# files are unaffected by .gitignore, so the 17 existing targets stayed visible +# and the trap only sprang on the 18th (moon#576): the target committed clean +# locally, then CI failed with "no such fuzz target". +fuzz/target/ +fuzz/corpus/ +fuzz/artifacts/ shard-*/ .serena/ console/tsconfig.app.tsbuildinfo diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ec41f055..808de512c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security +- **Fuzz the shared key-position walker, and stop a bare `.gitignore` entry from hiding new + fuzzers** (#576). `acl::keyspec::command_key_positions` parses attacker-controlled argv on behalf + of three consumers — ACL key-pattern enforcement, client-side cache invalidation, and command + introspection — so one bounds bug is a remote panic in three places at once. PR #571's review had + already found exactly that: a `numkeys` `usize` overflow that wrapped `first + nk` in release + builds and sliced `&args[1..0]`, reachable by any key-restricted authenticated user. The new + `acl_keyspec` target asserts the properties those callers rely on — every reported position + indexes `args`; `At` is never empty; and, the security-relevant one, `Unknown` and + `AtPlusComputed` must reach ACL as `Indeterminate`, so a `~pattern` user can never be granted a + key whose name is computed at runtime (`SORT k BY w_*`). Proven non-vacuous by reverting the + `checked_add` guard: the target reproduces the #571 crash from the seed corpus alone, minimizing + to the original attack string `LMPOP 18446744073709551615 a LEFT`. Clean over 3.27M executions + after restoring it. Two coverage gaps closed alongside: `.gitignore` matched a bare `fuzz`, which + ignores only NEW files (already-tracked ones are unaffected), so the 17 existing targets stayed + visible while the 18th would have committed clean locally and failed CI as "no such fuzz target"; + and `term_fst_sidecar` had been present in the tree but listed in neither CI matrix, so it had + never actually run. + ### Fixed - **`CLIENT TRACKING` never invalidated for movablekeys commands, so client-side caches went permanently stale** (#582). Commands whose keys are not at a fixed argument position carry diff --git a/CLAUDE.md b/CLAUDE.md index b6f96de20..4c753dafa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -188,7 +188,7 @@ orb run -m moon-dev bash -c 'sudo apt-get update -qq && sudo apt-get install -y - Every new command needs at least one unit test and one consistency test entry. - Integration tests use real server instances — no mocking. - Benchmarks use Criterion with `black_box()` on inputs and outputs. -- **Fuzzing:** 12 `cargo-fuzz` targets in `fuzz/fuzz_targets/`. Any new parser, decoder, or deserialization function MUST have a fuzz target. CI runs 15 min/target on PRs and 6h nightly. +- **Fuzzing:** 18 `cargo-fuzz` targets in `fuzz/fuzz_targets/`. Any new parser, decoder, or deserialization function MUST have a fuzz target, AND an entry in BOTH matrices in `.github/workflows/fuzz.yml` — a target that exists but is not listed never runs (`term_fst_sidecar` sat unlisted until moon#576). CI runs 15 min/target on PRs and 6h nightly. - **Loom:** model tests in `tests/loom_response_slot.rs` for lock-free data structures. Any new atomic state machine MUST have a loom model. ### Module Structure @@ -263,7 +263,7 @@ legs run locally via `scripts/ci-local.sh` before every push, and again on Actio - Client compat (self-hosted, real redis-server oracle) - Check (macOS), Check (Windows), Check (console feature) -**Scheduled:** fuzz nightly 6h (12 targets, nightly compiler, `rust-toolchain.toml` removed for the job), Crash Matrix nightly + weekly soak, CodeQL weekly, supply-chain weekly. +**Scheduled:** fuzz nightly 6h (18 targets, nightly compiler, `rust-toolchain.toml` removed for the job), Crash Matrix nightly + weekly soak, CodeQL weekly, supply-chain weekly. ### Local CI (the merge bar) diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 2b6520a63..5930fee14 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -108,3 +108,8 @@ doc = false name = "term_fst_sidecar" path = "fuzz_targets/term_fst_sidecar.rs" doc = false + +[[bin]] +name = "acl_keyspec" +path = "fuzz_targets/acl_keyspec.rs" +doc = false diff --git a/fuzz/corpus/acl_keyspec/del b/fuzz/corpus/acl_keyspec/del new file mode 100644 index 000000000..ccb909816 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/del differ diff --git a/fuzz/corpus/acl_keyspec/empty_argv b/fuzz/corpus/acl_keyspec/empty_argv new file mode 100644 index 000000000..5bfff2416 --- /dev/null +++ b/fuzz/corpus/acl_keyspec/empty_argv @@ -0,0 +1 @@ +GET \ No newline at end of file diff --git a/fuzz/corpus/acl_keyspec/eval b/fuzz/corpus/acl_keyspec/eval new file mode 100644 index 000000000..e0083a3a1 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/eval differ diff --git a/fuzz/corpus/acl_keyspec/eval_zero b/fuzz/corpus/acl_keyspec/eval_zero new file mode 100644 index 000000000..2a7f64cf3 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/eval_zero differ diff --git a/fuzz/corpus/acl_keyspec/ft_search b/fuzz/corpus/acl_keyspec/ft_search new file mode 100644 index 000000000..48f77756b Binary files /dev/null and b/fuzz/corpus/acl_keyspec/ft_search differ diff --git a/fuzz/corpus/acl_keyspec/georad_missing_store b/fuzz/corpus/acl_keyspec/georad_missing_store new file mode 100644 index 000000000..c998b8be2 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/georad_missing_store differ diff --git a/fuzz/corpus/acl_keyspec/georadius b/fuzz/corpus/acl_keyspec/georadius new file mode 100644 index 000000000..a1b85e57d Binary files /dev/null and b/fuzz/corpus/acl_keyspec/georadius differ diff --git a/fuzz/corpus/acl_keyspec/key_is_int b/fuzz/corpus/acl_keyspec/key_is_int new file mode 100644 index 000000000..2670a904a Binary files /dev/null and b/fuzz/corpus/acl_keyspec/key_is_int differ diff --git a/fuzz/corpus/acl_keyspec/key_is_null b/fuzz/corpus/acl_keyspec/key_is_null new file mode 100644 index 000000000..acdae99d7 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/key_is_null differ diff --git a/fuzz/corpus/acl_keyspec/lmpop b/fuzz/corpus/acl_keyspec/lmpop new file mode 100644 index 000000000..6e0a87e5b Binary files /dev/null and b/fuzz/corpus/acl_keyspec/lmpop differ diff --git a/fuzz/corpus/acl_keyspec/lmpop_zero b/fuzz/corpus/acl_keyspec/lmpop_zero new file mode 100644 index 000000000..eca2eac3a Binary files /dev/null and b/fuzz/corpus/acl_keyspec/lmpop_zero differ diff --git a/fuzz/corpus/acl_keyspec/memory b/fuzz/corpus/acl_keyspec/memory new file mode 100644 index 000000000..de67dada9 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/memory differ diff --git a/fuzz/corpus/acl_keyspec/mset b/fuzz/corpus/acl_keyspec/mset new file mode 100644 index 000000000..c39f00f3a Binary files /dev/null and b/fuzz/corpus/acl_keyspec/mset differ diff --git a/fuzz/corpus/acl_keyspec/numkeys_int b/fuzz/corpus/acl_keyspec/numkeys_int new file mode 100644 index 000000000..263240a7d Binary files /dev/null and b/fuzz/corpus/acl_keyspec/numkeys_int differ diff --git a/fuzz/corpus/acl_keyspec/numkeys_max b/fuzz/corpus/acl_keyspec/numkeys_max new file mode 100644 index 000000000..66e533af6 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/numkeys_max differ diff --git a/fuzz/corpus/acl_keyspec/numkeys_maxm1 b/fuzz/corpus/acl_keyspec/numkeys_maxm1 new file mode 100644 index 000000000..3383e1b23 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/numkeys_maxm1 differ diff --git a/fuzz/corpus/acl_keyspec/numkeys_neg b/fuzz/corpus/acl_keyspec/numkeys_neg new file mode 100644 index 000000000..bbf5663cb Binary files /dev/null and b/fuzz/corpus/acl_keyspec/numkeys_neg differ diff --git a/fuzz/corpus/acl_keyspec/object b/fuzz/corpus/acl_keyspec/object new file mode 100644 index 000000000..8ce6c85be Binary files /dev/null and b/fuzz/corpus/acl_keyspec/object differ diff --git a/fuzz/corpus/acl_keyspec/object_help b/fuzz/corpus/acl_keyspec/object_help new file mode 100644 index 000000000..7517cd4f3 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/object_help differ diff --git a/fuzz/corpus/acl_keyspec/ping b/fuzz/corpus/acl_keyspec/ping new file mode 100644 index 000000000..c181ae6dd --- /dev/null +++ b/fuzz/corpus/acl_keyspec/ping @@ -0,0 +1 @@ +PING \ No newline at end of file diff --git a/fuzz/corpus/acl_keyspec/sintercard b/fuzz/corpus/acl_keyspec/sintercard new file mode 100644 index 000000000..2b69eb4f6 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/sintercard differ diff --git a/fuzz/corpus/acl_keyspec/sort_by b/fuzz/corpus/acl_keyspec/sort_by new file mode 100644 index 000000000..9d10599a0 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/sort_by differ diff --git a/fuzz/corpus/acl_keyspec/sort_get b/fuzz/corpus/acl_keyspec/sort_get new file mode 100644 index 000000000..16bdd25cc Binary files /dev/null and b/fuzz/corpus/acl_keyspec/sort_get differ diff --git a/fuzz/corpus/acl_keyspec/sort_store b/fuzz/corpus/acl_keyspec/sort_store new file mode 100644 index 000000000..dcda01adb Binary files /dev/null and b/fuzz/corpus/acl_keyspec/sort_store differ diff --git a/fuzz/corpus/acl_keyspec/xread b/fuzz/corpus/acl_keyspec/xread new file mode 100644 index 000000000..5f85e93f0 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/xread differ diff --git a/fuzz/corpus/acl_keyspec/xread_odd b/fuzz/corpus/acl_keyspec/xread_odd new file mode 100644 index 000000000..597b6d900 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/xread_odd differ diff --git a/fuzz/corpus/acl_keyspec/xreadgroup b/fuzz/corpus/acl_keyspec/xreadgroup new file mode 100644 index 000000000..8864157d8 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/xreadgroup differ diff --git a/fuzz/corpus/acl_keyspec/zadd_store b/fuzz/corpus/acl_keyspec/zadd_store new file mode 100644 index 000000000..f1a6f5d69 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/zadd_store differ diff --git a/fuzz/corpus/acl_keyspec/zdiff b/fuzz/corpus/acl_keyspec/zdiff new file mode 100644 index 000000000..3c618e616 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/zdiff differ diff --git a/fuzz/corpus/acl_keyspec/zmpop b/fuzz/corpus/acl_keyspec/zmpop new file mode 100644 index 000000000..78ffc2f32 Binary files /dev/null and b/fuzz/corpus/acl_keyspec/zmpop differ diff --git a/fuzz/fuzz_targets/acl_keyspec.rs b/fuzz/fuzz_targets/acl_keyspec.rs new file mode 100644 index 000000000..6ea92750b --- /dev/null +++ b/fuzz/fuzz_targets/acl_keyspec.rs @@ -0,0 +1,138 @@ +#![no_main] +use libfuzzer_sys::fuzz_target; + +use bytes::Bytes; +use moon::acl::keyspec::{CommandKeys, KeyPositions, command_key_positions, command_keys}; +use moon::protocol::Frame; +use moon::tracking::invalidation; + +/// Fuzz the shared key-position walker (moon#576). +/// +/// `command_key_positions` parses attacker-controlled argv on behalf of THREE +/// consumers — ACL key-pattern checks, client-side cache invalidation, and +/// command introspection — so a single bounds bug is a remote panic in three +/// places at once. PR #571's review already found one of exactly this class: a +/// `numkeys` usize overflow that produced `&args[1..0]` (fixed in d0423747). +/// +/// Beyond "never panics", the properties below are the contract the callers +/// rely on. Two of them are security-relevant, not merely tidy: +/// +/// * every reported position must index `args` — the bounds property; +/// * `Unknown` and `AtPlusComputed` must reach ACL as `Indeterminate`. +/// `AtPlusComputed` means at least one key name is computed at runtime +/// (`SORT k BY w_*`), so a `~pattern` user could otherwise reach keys the +/// pattern was never meant to cover. Cache invalidation deliberately does +/// the opposite with the same value, which is why the walker reports facts +/// and each caller applies its own policy. +const MAX_ARGS: usize = 256; + +/// Decode `data` into a command name and its argv. +/// +/// Fields are NUL-separated: the first is the command name, the rest are the +/// arguments (which EXCLUDE the command name, matching the walker's contract). +/// A leading tag byte picks the frame type so the fuzzer can reach the +/// non-string branches — a key position holding an `Integer` is a malformed +/// invocation the walker still has to survive. +fn decode(data: &[u8]) -> Option<(Vec, Vec)> { + let mut fields = data.split(|&b| b == 0); + let cmd = fields.next()?.to_vec(); + let args = fields + .take(MAX_ARGS) + .map(|f| match f.split_first() { + Some((0x01, rest)) => { + // Reach the numkeys/count walkers with values they must clamp: + // 0, 1, usize::MAX and its neighbours all live here. + let mut n = [0u8; 8]; + let take = rest.len().min(8); + n[..take].copy_from_slice(&rest[..take]); + Frame::Integer(i64::from_le_bytes(n)) + } + Some((0x02, _)) => Frame::Null, + Some((0x03, rest)) => Frame::SimpleString(Bytes::copy_from_slice(rest)), + _ => Frame::BulkString(Bytes::copy_from_slice(f)), + }) + .collect(); + Some((cmd, args)) +} + +fuzz_target!(|data: &[u8]| { + let Some((cmd, args)) = decode(data) else { + return; + }; + let argc = args.len(); + + let positions = command_key_positions(&cmd, &args); + + // Bounds: the property whose violation is a remote panic downstream. + match &positions { + KeyPositions::At(idx) => { + assert!(!idx.is_empty(), "At is documented as never empty"); + for &i in idx.iter() { + assert!(i < argc, "At position {i} out of bounds for argc {argc}"); + } + } + KeyPositions::AtPlusComputed(idx) => { + for &i in idx.iter() { + assert!( + i < argc, + "AtPlusComputed position {i} out of bounds for argc {argc}" + ); + } + } + KeyPositions::None | KeyPositions::Unknown => {} + } + + // The walker is a pure function of its inputs; a consumer that calls it + // twice (ACL then tracking, on the same command) must see the same answer. + let again = command_key_positions(&cmd, &args); + assert_eq!( + std::mem::discriminant(&positions), + std::mem::discriminant(&again), + "walker is not deterministic" + ); + + // --- consumer 1: ACL, which must fail CLOSED --- + let acl = command_keys(&cmd, &args); + match (&positions, &acl) { + (KeyPositions::None, CommandKeys::None) => {} + (KeyPositions::None, other) => { + panic!("provably keyless command reached ACL as {other:?}") + } + // A named position that is not a string cannot be checked against a + // key pattern, so `At` is allowed to degrade to Indeterminate. + (KeyPositions::At(_), CommandKeys::Keys(k)) => { + assert!(!k.is_empty(), "Keys is documented as never empty"); + } + (KeyPositions::At(_), CommandKeys::Indeterminate) => {} + (KeyPositions::At(_), CommandKeys::None) => { + panic!("command with key positions reached ACL as keyless") + } + // The security property: neither of these may ever name keys to ACL. + (KeyPositions::AtPlusComputed(_) | KeyPositions::Unknown, CommandKeys::Indeterminate) => {} + (KeyPositions::AtPlusComputed(_) | KeyPositions::Unknown, other) => { + panic!("unenumerable argv must deny, reached ACL as {other:?}") + } + } + + // --- consumer 2: cache invalidation, which must NOT fail closed --- + let tracked = invalidation::command_keys(&cmd, &args); + match &positions { + KeyPositions::None | KeyPositions::Unknown => { + assert!( + tracked.is_empty(), + "nothing to invalidate, got {} keys", + tracked.len() + ); + } + KeyPositions::At(idx) | KeyPositions::AtPlusComputed(idx) => { + // Non-string positions are skipped, so this is a ceiling, not an + // equality — but inventing a key would be an over-invalidation bug. + assert!( + tracked.len() <= idx.len(), + "invalidated {} keys from {} positions", + tracked.len(), + idx.len() + ); + } + } +}); diff --git a/src/acl/keyspec.rs b/src/acl/keyspec.rs index aed00530b..87bba70ae 100644 --- a/src/acl/keyspec.rs +++ b/src/acl/keyspec.rs @@ -53,11 +53,11 @@ use crate::protocol::Frame; /// Borrowed key slices. Four inline slots cover every fixed-arity keyed /// command in the registry; only variadic forms (`DEL a b c d e`) spill. -pub(crate) type KeyVec<'a> = SmallVec<[&'a [u8]; 4]>; +pub type KeyVec<'a> = SmallVec<[&'a [u8]; 4]>; /// Outcome of key extraction. See the module docs for the contract. #[derive(Debug)] -pub(crate) enum CommandKeys<'a> { +pub enum CommandKeys<'a> { /// The command provably names no key. None, /// The keys this invocation touches (never empty). @@ -84,7 +84,7 @@ const UNREGISTERED_KEYLESS: &[&[u8]] = &[ ]; /// Zero-based positions in `args` of the keys an invocation touches. -pub(crate) type KeyIdx = SmallVec<[usize; 4]>; +pub type KeyIdx = SmallVec<[usize; 4]>; /// Where the keys of an argv live, without interpreting them. /// @@ -92,7 +92,7 @@ pub(crate) type KeyIdx = SmallVec<[usize; 4]>; /// FACTS; each caller applies its own POLICY, because the callers legitimately /// disagree — see [`AtPlusComputed`](KeyPositions::AtPlusComputed). #[derive(Debug)] -pub(crate) enum KeyPositions { +pub enum KeyPositions { /// The command provably names no key. None, /// The positions of the keys this invocation touches (never empty). @@ -111,7 +111,7 @@ pub(crate) enum KeyPositions { /// Locate the key arguments of `cmd`/`args`. `args` EXCLUDES the command name, /// so registry key-spec index `N` maps to `args[N - 1]`. -pub(crate) fn command_key_positions(cmd: &[u8], args: &[Frame]) -> KeyPositions { +pub fn command_key_positions(cmd: &[u8], args: &[Frame]) -> KeyPositions { // Layouts a fixed first/last/step spec cannot express come first: some of // them (SORT, OBJECT, ZUNIONSTORE) DO have a spec, but it describes only // part of the truth. @@ -177,7 +177,7 @@ pub(crate) fn command_key_positions(cmd: &[u8], args: &[Frame]) -> KeyPositions /// cannot be fully enumerated — including an argv that also reaches /// runtime-computed key names — is [`CommandKeys::Indeterminate`], so the /// caller denies. -pub(crate) fn command_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> CommandKeys<'a> { +pub fn command_keys<'a>(cmd: &[u8], args: &'a [Frame]) -> CommandKeys<'a> { let idx = match command_key_positions(cmd, args) { KeyPositions::None => return CommandKeys::None, KeyPositions::At(idx) => idx, diff --git a/src/acl/mod.rs b/src/acl/mod.rs index e1bac956b..1951c3639 100644 --- a/src/acl/mod.rs +++ b/src/acl/mod.rs @@ -1,5 +1,9 @@ pub mod io; -pub(crate) mod keyspec; +// `pub` rather than `pub(crate)`: the key-position walker is a contract shared +// by ACL, cache invalidation and command introspection, and it parses attacker +// -controlled argv. `fuzz/fuzz_targets/acl_keyspec.rs` drives it directly +// (moon#576) — a bounds bug here is a remote panic in three places at once. +pub mod keyspec; pub mod log; pub mod rules; pub mod table;