diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml index 045e6e28d..1e81e8a01 100644 --- a/.github/workflows/fuzz.yml +++ b/.github/workflows/fuzz.yml @@ -42,6 +42,7 @@ jobs: - term_fst_sidecar - dump_payload - acl_keyspec + - ft_create_args steps: - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@nightly @@ -109,6 +110,7 @@ jobs: - term_fst_sidecar - dump_payload - acl_keyspec + - ft_create_args steps: - uses: actions/checkout@v7 - uses: dtolnay/rust-toolchain@nightly diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a89407c0..1b1f28fd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -190,6 +190,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 as an answer. That shard also logs a warning, because cold keys with no manifest will not survive a restart — `rebuild_from_manifest` is the only thing that re-indexes them. ### Fixed +- **A truncated `FT.CREATE` no longer aborts the server** (#681). `FT.CREATE idx ON HASH + PREFIX 1 d: SCHEMA v VECTOR HNSW` — the argument list cut off right after the algorithm + keyword — indexed one past the end of argv and panicked. The panic ran on a shard + thread, and moon deliberately escalates a shard panic to a whole-process abort rather + than serve on with a dead shard, so one short line from any client took the entire + server down: every database, every other connection. No auth and no large payload + required. + + The parameter loop below the fault already guarded both ends + (`*pos + 1 < param_end && *pos + 1 < args.len()`), so the value read for every keyword + was safe; the parameter *count* read was the single unguarded one. That was measured, + not assumed — six truncation shapes were probed against freshly spawned, + listener-PID-checked servers, and only this one killed the process. It reports + `ERR invalid param count`, the same error an unparseable count already produced, so the + two ways of failing to supply a count are indistinguishable to a client. There is no + redis oracle for the string: the `redis-server` checked against has no query engine, so + `FT.CREATE` is `unknown command` there. + + `FT.CREATE` argument parsing had **no fuzz target** for its whole life, which is how a + one-line remote crash survived in it; `fuzz/fuzz_targets/ft_create_args.rs` now drives + the real entry point with arbitrary argv and is listed in both matrices in + `fuzz.yml` (an unlisted target never runs — #576). + - **Lua script errors reached the client as an unparseable RESP frame** (#672). mlua's `Display` carries a multi-line Lua traceback, and a RESP *simple* error may not contain CR or LF anywhere — so every runtime error (`redis.call('INCR', k)` on a string, diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 66f1ab731..9b069d218 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -118,3 +118,8 @@ doc = false name = "acl_keyspec" path = "fuzz_targets/acl_keyspec.rs" doc = false + +[[bin]] +name = "ft_create_args" +path = "fuzz_targets/ft_create_args.rs" +doc = false diff --git a/fuzz/fuzz_targets/ft_create_args.rs b/fuzz/fuzz_targets/ft_create_args.rs new file mode 100644 index 000000000..4a62d4802 --- /dev/null +++ b/fuzz/fuzz_targets/ft_create_args.rs @@ -0,0 +1,156 @@ +#![no_main] +use libfuzzer_sys::fuzz_target; + +use bytes::Bytes; +use moon::command::vector_search::ft_create; +use moon::protocol::Frame; +use moon::text::store::TextStore; +use moon::vector::store::VectorStore; + +/// Fuzz `FT.CREATE` argument parsing (moon#681). +/// +/// This parser had no fuzz target for its whole life, and that is exactly how +/// a one-line remote crash survived in it: `FT.CREATE idx ON HASH PREFIX 1 d: +/// SCHEMA v VECTOR HNSW` -- truncated right after the algorithm keyword -- +/// indexed one past the end of argv and panicked. The panic ran on a shard +/// thread, and moon escalates a shard panic to a whole-process abort, so a +/// single short line from any unauthenticated client took the server down with +/// every database and every other connection on it. +/// +/// The property is simply "never panics". `FT.CREATE` walks attacker-supplied +/// argv with a hand-rolled cursor (`*pos += 1` then read), and every such +/// cursor is a bounds bug waiting to happen -- the fix for #681 added the one +/// missing guard, and this target exists so the next one is found here rather +/// than in production. +/// +/// Both `VectorStore` and `TextStore` are rebuilt per input so a create that +/// succeeds cannot make a later input take a different path; each run sees an +/// empty registry, which is the state a fresh server is in. +const MAX_ARGS: usize = 128; + +/// The vocabulary `FT.CREATE` actually branches on. +/// +/// **This table is why the target works.** Two earlier drafts failed, and each +/// failure was measured against a deliberately un-fixed parser rather than +/// assumed: +/// +/// 1. Fully-arbitrary argv: 1.2M execs, nothing. `ft_create` demands the +/// preamble `idx ON HASH PREFIX 1 d: SCHEMA v VECTOR` before the vector +/// parser is reached, and random mutation does not synthesize a +/// nine-keyword sequence. It fuzzed the preamble and never got past it. +/// 2. Valid skeleton + byte-level tail: 871K execs, still nothing. Reaching +/// the parser is not enough -- the crash needs the literal ASCII `HNSW` +/// in argv, and inventing a specific four-byte string by mutation is a +/// 2^32 search. +/// +/// So the fuzzer picks *keywords*, not bytes: one input byte selects one argv +/// element from this table. Now `HNSW` is one byte away and the search is over +/// keyword sequences -- which is the actual state space the parser walks. +const VOCAB: &[&[u8]] = &[ + b"HNSW", + b"FLAT", + b"TYPE", + b"FLOAT32", + b"DIM", + b"DISTANCE_METRIC", + b"L2", + b"COSINE", + b"IP", + b"M", + b"EF_CONSTRUCTION", + b"EF_RUNTIME", + b"COMPACT_THRESHOLD", + b"QUANTIZATION", + b"TQ4", + b"SQ8", + b"FP32", + b"BUILD_MODE", + b"MERGE_MODE", + b"GRAPH_UNION", + b"KEEP_RAW", + b"WEIGHTED", + b"0", + b"1", + b"2", + b"4", + b"6", + b"8", + b"16", + b"768", + b"-1", + b"99999999999999999999", + b"", + b"NOT_A_KEYWORD", +]; + +fn bulk(s: &[u8]) -> Frame { + Frame::BulkString(Bytes::copy_from_slice(s)) +} + +/// A well-formed `FT.CREATE` up to and including the `VECTOR` keyword, so the +/// fuzzed tail lands exactly where `parse_vector_field_params` starts reading. +fn skeleton() -> Vec { + [ + b"idx".as_slice(), + b"ON", + b"HASH", + b"PREFIX", + b"1", + b"d:", + b"SCHEMA", + b"v", + b"VECTOR", + ] + .iter() + .map(|s| bulk(s)) + .collect() +} + +/// Decode `data` into an argv: one byte per argv element. +/// +/// A byte selects a `VOCAB` entry; two reserved residues emit an `Integer` and +/// a `Null` instead, because a non-string where a keyword belongs is a +/// malformed invocation the parser still has to survive, and `extract_bulk` +/// returning `None` drives cursor arithmetic the all-strings shape never +/// reaches. +/// +/// One input in four is argv with no skeleton at all, so the preamble parser +/// -- everything before `VECTOR` -- is not left uncovered by the specialisation. +fn decode(data: &[u8]) -> Vec { + if data.is_empty() { + return Vec::new(); + } + let tag = data[0]; + let tail: Vec = data[1..] + .iter() + .take(MAX_ARGS) + .enumerate() + .map(|(i, &b)| { + let slot = b as usize % (VOCAB.len() + 2); + match slot.checked_sub(VOCAB.len()) { + Some(0) => Frame::Integer(i as i64), + Some(_) => Frame::Null, + None => bulk(VOCAB[slot]), + } + }) + .collect(); + + if tag % 4 == 0 { + return tail; + } + let mut args = skeleton(); + args.extend(tail); + args +} + +fuzz_target!(|data: &[u8]| { + let args = decode(data); + // No length guard here on purpose: `take(MAX_ARGS)` already bounds the + // tail, and an early `return` would be a silent skip -- inputs the target + // reports as covered while never running them. + let mut store = VectorStore::new(); + let mut text = TextStore::new(); + // The reply is not asserted on: FT.CREATE legitimately answers +OK or any + // of a dozen errors depending on argv. Surviving the call IS the property. + let _ = ft_create(&mut store, &mut text, &args, 0); +}); diff --git a/src/command/vector_search/ft_create.rs b/src/command/vector_search/ft_create.rs index 03d183553..95a5a6ba1 100644 --- a/src/command/vector_search/ft_create.rs +++ b/src/command/vector_search/ft_create.rs @@ -547,6 +547,21 @@ fn parse_vector_field_params(args: &[Frame], pos: &mut usize) -> Result= args.len() { + return Err(Frame::Error(Bytes::from_static(b"ERR invalid param count"))); + } + let num_params = match parse_u32(&args[*pos]) { Some(n) => n as usize, None => { @@ -781,3 +796,96 @@ fn parse_vector_field_params(args: &[Frame], pos: &mut usize) -> Result Frame { + Frame::BulkString(Bytes::copy_from_slice(s)) + } + + fn err_text(f: &Frame) -> String { + match f { + Frame::Error(b) => String::from_utf8_lossy(b).into_owned(), + other => panic!("expected an error frame, got {other:?}"), + } + } + + /// moon#681: `FT.CREATE idx ... SCHEMA v VECTOR HNSW` with nothing after + /// the algorithm keyword used to index one past the end and panic. The + /// panic was on a shard thread, and moon escalates a shard panic to a + /// process abort -- so a single short line from any client took the whole + /// server down, every database and every other connection with it. + /// + /// Before the fix this test does not fail an assertion, it *panics*, which + /// is the point: the parser must return an error frame for a truncated + /// argument list, never index past the end. + #[test] + fn truncated_after_the_algorithm_keyword_errors_instead_of_panicking() { + let args = vec![bulk(b"HNSW")]; + let mut pos = 0usize; + let Err(err) = parse_vector_field_params(&args, &mut pos) else { + panic!("a truncated VECTOR clause must not parse"); + }; + assert_eq!(err_text(&err), "ERR invalid param count"); + } + + /// The neighbouring truncations were already safe -- the parameter loop + /// guards `*pos + 1 < args.len()` before every value read -- and this + /// pins that, so a future edit to the loop condition cannot quietly + /// reopen the same hole one keyword further in. + #[test] + fn truncations_inside_the_param_list_are_already_bounded() { + for tail in [ + vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"TYPE")], + vec![ + bulk(b"HNSW"), + bulk(b"6"), + bulk(b"TYPE"), + bulk(b"FLOAT32"), + bulk(b"DIM"), + ], + vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"DISTANCE_METRIC")], + vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"M")], + vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"EF_CONSTRUCTION")], + vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"EF_RUNTIME")], + vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"COMPACT_THRESHOLD")], + ] { + let mut pos = 0usize; + // Either outcome is fine; not panicking is the assertion. + let _ = parse_vector_field_params(&tail, &mut pos); + } + } + + /// A non-numeric count still reports the count, not something further on. + #[test] + fn a_non_numeric_param_count_is_reported_as_such() { + let args = vec![bulk(b"HNSW"), bulk(b"notanint")]; + let mut pos = 0usize; + let Err(err) = parse_vector_field_params(&args, &mut pos) else { + panic!("a non-numeric param count must not parse"); + }; + assert_eq!(err_text(&err), "ERR invalid param count"); + } + + /// The pre-existing guard above the fix: a missing/incorrect algorithm + /// keyword keeps its own distinct message, so the two failures stay + /// distinguishable to a client. + #[test] + fn a_wrong_algorithm_keyword_keeps_its_own_message() { + let args = vec![bulk(b"FLAT"), bulk(b"6")]; + let mut pos = 0usize; + let Err(err) = parse_vector_field_params(&args, &mut pos) else { + panic!("FLAT is not implemented"); + }; + assert_eq!(err_text(&err), "ERR expected HNSW algorithm"); + + let empty: Vec = vec![]; + let mut pos = 0usize; + let Err(err) = parse_vector_field_params(&empty, &mut pos) else { + panic!("an empty argument list must not parse"); + }; + assert_eq!(err_text(&err), "ERR expected HNSW algorithm"); + } +}