diff --git a/tasks/done/security-audit-1.md b/tasks/done/security-audit-1.md new file mode 100644 index 0000000..064280f --- /dev/null +++ b/tasks/done/security-audit-1.md @@ -0,0 +1,4 @@ +# security audit + +as a rust expert do a security audit. +create tickets in tasks/todo for any security issue found \ No newline at end of file diff --git a/tasks/todo/SEC-001-resp-idle-timeout.md b/tasks/todo/SEC-001-resp-idle-timeout.md new file mode 100644 index 0000000..52391b1 --- /dev/null +++ b/tasks/todo/SEC-001-resp-idle-timeout.md @@ -0,0 +1,39 @@ +# SEC-001 — RESP server has no read/idle timeout, enabling connection exhaustion DoS + +**Severity:** High + +## Location + +- `crates/solikv-server/src/resp_server.rs:67-110` (`handle_connection`) +- `crates/solikv-server/src/resp_server.rs:19` (`MAX_CONNECTIONS = 10_000`) + +## Issue + +`handle_connection` calls `socket.read_buf(&mut read_buf).await` (line 105) with no +read timeout. A connection that sends nothing (or a partial RESP frame) sits forever +in `read_buf` and holds one of the 10,000 semaphore permits. + +A single attacker opening 10,000 idle TCP connections (no bytes ever sent) drains +the connection pool and locks every legitimate client out. The pre-pubsub-mode read +loop, the pubsub-mode `socket.read_buf` (line 611), and the `auth_middleware` +fast-fail path all share this property: no per-stage deadline. + +There is also no `tcp_keepalive` configuration, so half-open TCP connections are +never reaped. + +## Fix + +1. Wrap the per-iteration `socket.read_buf(...)` calls in + `tokio::time::timeout(...)` with a configurable idle deadline (suggested + default: 60 s, matching Redis `timeout 0` override). +2. On timeout, write a RESP error and close the connection. +3. Apply the same timeout in `handle_pubsub_mode` (`resp_server.rs:611`). +4. Optionally enable TCP keepalive on the accepted socket via + `socket2::SockRef::from(&stream).set_tcp_keepalive(...)`. + +## Verification + +- Unit: open a TCP connection, send 0 bytes, assert it is closed after the + configured timeout. +- Manual: `nc -q -1 127.0.0.1 6379 &` ×10 000, then verify a fresh `redis-cli` + client can still connect within `timeout * 2`. diff --git a/tasks/todo/SEC-002-resp-array-stack-overflow.md b/tasks/todo/SEC-002-resp-array-stack-overflow.md new file mode 100644 index 0000000..2403b2a --- /dev/null +++ b/tasks/todo/SEC-002-resp-array-stack-overflow.md @@ -0,0 +1,39 @@ +# SEC-002 — RESP parser recurses without depth limit, allowing single-packet stack overflow / crash + +**Severity:** High + +## Location + +- `crates/solikv-resp/src/codec.rs:227-269` (`decode_array`, recursive call to `decode_frame`) +- `crates/solikv-resp/src/codec.rs:131` (`MAX_ARRAY_LEN = 1_048_576`) + +## Issue + +`decode_array` calls `decode_frame(&src[offset..])` for every element, and +`decode_frame` dispatches back to `decode_array` for nested arrays +(`*` prefix). There is no recursion-depth limit. An attacker connected to the +RESP port can send a single buffer containing `*1\r\n*1\r\n*1\r\n…` repeated a +few thousand times. Each level adds a stack frame, and the recursion eventually +overflows the (default 2 MB) thread stack — the Tokio worker dies with SIGSEGV +and the server's connection accept loop on that worker is gone. + +This is *pre-auth* (the parser runs before AUTH is checked) and unauthenticated +remote callers can crash the server with a single TCP packet. `MAX_ARRAY_LEN` +limits each level's element count but does not bound nesting depth. + +## Fix + +1. Add a `decode_frame_with_depth(src, depth)` helper. Bump `depth` on each + recursive call from `decode_array`. Reject (return `Err`) when `depth` + exceeds a hard cap (suggested: 32, matching Redis client/server typical + nesting use). +2. Change the public `decode_frame(src)` to call + `decode_frame_with_depth(src, 0)`. +3. Add a unit test that constructs an N-deep nested array (e.g. 10 000) and + asserts the parser returns an error rather than panicking / overflowing. + +## Verification + +- Unit: deep-nested input returns `Err`, not a stack overflow. +- Existing parser tests still pass. +- `cargo clippy --quiet -- -D warnings` clean. diff --git a/tasks/todo/SEC-003-lua-sandbox-state-leak.md b/tasks/todo/SEC-003-lua-sandbox-state-leak.md new file mode 100644 index 0000000..d9cd40b --- /dev/null +++ b/tasks/todo/SEC-003-lua-sandbox-state-leak.md @@ -0,0 +1,67 @@ +# SEC-003 — Lua VM is pooled and reused without resetting global state, allowing sandbox poisoning across scripts + +**Severity:** High + +## Location + +- `crates/solikv-engine/src/lua.rs:103-138` (`LUA_POOL`, `take_lua`, `return_lua`) +- `crates/solikv-engine/src/lua.rs:147-186` (`execute_script`) +- `crates/solikv-engine/src/lua.rs:192-209` (`sandbox_lua` runs once on cold path only) + +## Issue + +`take_lua` reuses a previously-sandboxed `Lua` instance from a thread-local +`LUA_POOL`. The sandbox (`sandbox_lua`) is applied once on creation, and only +`KEYS` / `ARGV` are reset between invocations. Globals other than `KEYS` / +`ARGV` — including the standard-library tables `string`, `table`, `math` — +are *not* restored between scripts. + +A malicious script can therefore plant traps that affect future scripts running +on the same worker thread: + +```lua +-- attacker: EVAL "string.byte = function() return 999 end ..." +-- victim: EVAL "return string.byte('x')" → 999, not 120 +``` + +Or worse, a script can monkey-patch `redis.call` itself: + +```lua +local orig = redis.call +redis.call = function(...) ... return "" end -- silent data corruption +``` + +Because the VM is pooled per-thread and reused, this contaminates other tenants +sharing the server. `redis` itself is set in `setup_redis_module` only on cold +path (line 124), so a poisoned `redis.call` survives every subsequent +`execute_script` on that thread until the VM is dropped (pool is full). + +## Fix + +Either (preferred): + +1. Re-snapshot the global table after `sandbox_lua` + `setup_redis_module` + completes (e.g., serialize a registry-stashed clean copy), and restore it on + each `take_lua`. mlua exposes `Lua::globals()` and `Table::clear()` / + pairs traversal to rebuild it. + +Or, simpler: + +2. Drop the pool entirely — create a fresh `Lua` per script. The cost is one + sandbox setup per EVAL, which dominates only for trivially small scripts. + Benchmark first. + +Or: + +3. Wrap user scripts in a fresh Lua function environment (`setfenv` / + `_ENV` swap in 5.2+) so each script gets its own `_ENV` with read-only + parent. Verify mlua exposes the necessary primitives. + +Option 1 or 3 preserves the perf benefit. Option 2 is simplest and safest. + +## Verification + +- New test: run two scripts back-to-back on the same engine; the first mutates + `string.byte`, the second calls `string.byte('x')` and must observe the + original value. +- Existing Lua tests still pass. diff --git a/tasks/todo/SEC-004-script-cache-unbounded.md b/tasks/todo/SEC-004-script-cache-unbounded.md new file mode 100644 index 0000000..2a2e847 --- /dev/null +++ b/tasks/todo/SEC-004-script-cache-unbounded.md @@ -0,0 +1,43 @@ +# SEC-004 — Script cache (`SCRIPT LOAD` / `EVAL`) has no size cap, enabling memory-exhaustion DoS + +**Severity:** High + +## Location + +- `crates/solikv-engine/src/lua.rs:20-58` (`ScriptCache`) +- `crates/solikv-engine/src/dispatch.rs:3236` (`EVAL` calls `script_cache.load(script)` on every invocation) +- `crates/solikv-engine/src/dispatch.rs:3286-3303` (`SCRIPT LOAD`) + +## Issue + +`ScriptCache` is a `DashMap` with no eviction, no maximum +entry count, and no maximum source size. Any authenticated client can call: + +``` +SCRIPT LOAD "" +``` + +repeatedly, and each insert is permanent. With ~1 KiB scripts the server holds +~10 MB after 10 000 calls; with 1 MiB scripts, 10 GB after 10 000 calls. There +is no command to flush only stale entries, and even `SCRIPT FLUSH` requires the +attacker's cooperation. + +`EVAL` *also* re-inserts the script source on every execution (line 3236), so +the cache grows on the regular EVAL hot path too — not just on `SCRIPT LOAD`. + +## Fix + +1. Cap script source size at a sane limit (suggested: 64 KiB, configurable). + Reject larger scripts with `ERR script too long`. +2. Cap total script count (suggested: 8 192 entries) and switch the storage + from `DashMap` to a small LRU (e.g. `lru::LruCache` behind a `Mutex`, + contention is fine — script load is rare). +3. In `EVAL`, only insert into the cache if the SHA is not already present + (cheap `contains_key` check), to avoid the rewrite-on-every-execution + churn. + +## Verification + +- Unit: load N scripts where N > cache cap, assert oldest is evicted. +- Unit: `EVAL` of an oversize script returns `ERR script too long`. +- Existing scripting tests still pass. diff --git a/tasks/todo/SEC-005-secrets-on-cli.md b/tasks/todo/SEC-005-secrets-on-cli.md new file mode 100644 index 0000000..721df9e --- /dev/null +++ b/tasks/todo/SEC-005-secrets-on-cli.md @@ -0,0 +1,45 @@ +# SEC-005 — Passwords accepted only via CLI flags, exposing them in `ps` / `/proc//cmdline` / shell history + +**Severity:** High + +## Location + +- `crates/solikv-server/src/main.rs:65-67` (`--requirepass`) +- `crates/solikv-server/src/main.rs:97-99` (`--cluster-password`) + +## Issue + +The auth password and cluster password are accepted only as `clap` arguments +with `value_name = "PASSWORD"`. On Linux: + +- `ps auxe`, `/proc//cmdline`, and any monitoring tool that reads cmdlines + expose the password to other local users. +- The launching shell's history (`.bash_history`, `.zsh_history`) records it. +- The daemon code (`main.rs:140-149`) re-`spawn`s itself with + `std::env::args().skip(1)`, so the child process *also* has the password in + its cmdline. + +Production deployments commonly leak Redis passwords this way; SoliKV inherits +the same risk and has no alternative input mechanism. + +## Fix + +Add support for password sources, in priority order: + +1. `--requirepass-file ` / `--cluster-password-file `: read a file + (newline-trimmed). Recommended for production. +2. `SOLIKV_PASSWORD` / `SOLIKV_CLUSTER_PASSWORD` environment variables (clap's + `env = "..."` attribute). +3. Keep `--requirepass ` for dev convenience but **emit a warning to + stderr** when used so operators see the leak risk. + +Refuse to start if both flag and file/env are set, to avoid silent precedence +confusion. + +When the file path is used, verify mode 0600 (warn loudly if world-readable). + +## Verification + +- Unit: parse args from a file successfully; warn when flag is used. +- Manual: `ps -ef | grep solikv` after start with `--requirepass-file` shows + no password in cmdline. diff --git a/tasks/todo/SEC-006-pidfile-tmp-symlink.md b/tasks/todo/SEC-006-pidfile-tmp-symlink.md new file mode 100644 index 0000000..83b4307 --- /dev/null +++ b/tasks/todo/SEC-006-pidfile-tmp-symlink.md @@ -0,0 +1,53 @@ +# SEC-006 — Daemon mode uses world-writable `/tmp/solikv.pid` + `/tmp/solikv.log`, with weak process verification + +**Severity:** High + +## Location + +- `crates/solikv-server/src/main.rs:106` (`pidfile = PathBuf::from("/tmp/solikv.pid")`) +- `crates/solikv-server/src/main.rs:108-130` (kill-old logic) +- `crates/solikv-server/src/main.rs:132-138` (`/tmp/solikv.log` opened with `create + append`) + +## Issue + +Three problems compound: + +1. **Symlink attack on the log file.** `OpenOptions::new().create(true).append(true) + .open("/tmp/solikv.log")` follows existing symlinks. On a multi-user host an + attacker who controls `/tmp/solikv.log` (it is in a sticky-bit directory but + they can win the race or pre-place a symlink before solikv first runs) can + point it at any path the operator can write to (e.g. `/etc/something`, + `~/.bashrc`). solikv then appends its stdout/stderr there. + +2. **PID file race + weak verification.** Lines 113-122 read `/proc//cmdline` + and only check `.contains("solikv")`. Any cmdline containing the substring + "solikv" passes — including a user's `vim /tmp/solikv-notes`, `grep solikv + …`, etc. Combined with the predictable `/tmp/solikv.pid` path (which a local + attacker can pre-write), this lets the attacker get solikv to send `kill` to + an arbitrary PID belonging to the operator (so long as that process's + cmdline mentions "solikv"). + +3. **Use of `kill(1)` subprocess** for what is a built-in syscall — extra fork, + extra attack surface, and silently swallows errors with `let _ = ... .output()`. + +## Fix + +1. Move pidfile and log to `args.dir` (the operator-supplied data directory), + defaulting to `./data`. Refuse to operate on `/tmp` paths. +2. Open the log with `OpenOptions::new().custom_flags(libc::O_NOFOLLOW)` (Unix) + to refuse symlinks; or open with `O_CREAT | O_EXCL` first and fall back to + append-after-stat-check. +3. Strengthen the PID check: parse `/proc//exe` (a symlink to the binary) + and compare to `std::env::current_exe()`. `cmdline.contains("solikv")` is + not a security check. +4. Replace the `Command::new("kill")` subprocess with `nix::sys::signal::kill` + or `libc::kill` directly. +5. After unlinking the stale pidfile, write the new one with `O_EXCL` so two + concurrent daemon starts cannot both succeed. + +## Verification + +- Unit (where feasible): given a PID whose `/proc//exe` does not equal our + current exe, the kill path is a no-op. +- Manual: `ln -s /tmp/victim /tmp/solikv.log`, run `solikv -d`, confirm solikv + refuses (or creates a new file separate from the symlink target). diff --git a/tasks/todo/SEC-007-default-bind-no-auth.md b/tasks/todo/SEC-007-default-bind-no-auth.md new file mode 100644 index 0000000..8dd1ba3 --- /dev/null +++ b/tasks/todo/SEC-007-default-bind-no-auth.md @@ -0,0 +1,51 @@ +# SEC-007 — Default bind is `0.0.0.0` with no password required, exposing an unauthenticated DB to the network + +**Severity:** High + +## Location + +- `crates/solikv-server/src/main.rs:38-39` (`#[arg(long, default_value = "0.0.0.0")] bind`) +- `crates/solikv-server/src/main.rs:65-67` (`requirepass: Option` — no default) +- `crates/solikv-server/src/resp_server.rs:80` (`requires_auth = password.is_some()`) + +## Issue + +A user who runs `solikv` with no flags gets: + +- RESP server listening on `0.0.0.0:6379` +- REST API on `0.0.0.0:5020` +- No authentication required (`password = None` ⇒ `requires_auth = false`) + +This exposes the entire database to anyone on the same network. The same +default tripped Redis for years and led to a documented mass-exploitation +campaign ("Redis WCRY"). The README's quickstart and Docker example both run +this way. + +The cluster-bus port at least binds to `127.0.0.1` only (`gossip.rs:336`), but +the public RESP/REST surface does not. + +## Fix + +Pick one of these (in order of preference): + +1. **Refuse to start unauthenticated when bind is non-loopback.** If + `args.bind != "127.0.0.1"` and `args.requirepass.is_none()`, exit with + a clear error pointing at `--requirepass` and `--bind 127.0.0.1`. Provide + `--protected-mode no` opt-out for users who really mean it. +2. Change the default `bind` to `127.0.0.1`. Document `--bind 0.0.0.0` for + network exposure. + +Approach 1 is closer to upstream Redis "protected mode" and is what most +Redis-compatible projects do. Approach 2 is simpler but slightly less +forgiving. + +Also update the README quickstart and Docker examples to set `--requirepass` +and/or `--bind 127.0.0.1`, and note the new behavior in +`www/app/views/docs/getting_started.html.slv`. + +## Verification + +- Unit: `Args::parse_from(["solikv", "--bind", "0.0.0.0"])` plus the + protected-mode check returns an error. +- Manual: `cargo run` with no flags refuses to bind public; `cargo run -- + --bind 0.0.0.0 --requirepass foo` works. diff --git a/tasks/todo/SEC-008-no-tls-support.md b/tasks/todo/SEC-008-no-tls-support.md new file mode 100644 index 0000000..79fdab2 --- /dev/null +++ b/tasks/todo/SEC-008-no-tls-support.md @@ -0,0 +1,52 @@ +# SEC-008 — No TLS support; passwords and data travel in cleartext on both RESP and REST ports + +**Severity:** Medium + +## Location + +- `crates/solikv-server/src/resp_server.rs:28` (`TcpListener::bind`) +- `crates/solikv-server/src/rest_server.rs:84-86` (`tokio::net::TcpListener` + `axum::serve`) +- `Cargo.toml` workspace deps — no `rustls`, `tokio-rustls`, `axum-server` w/ TLS. + +## Issue + +SoliKV speaks plaintext RESP on the wire and plaintext HTTP on the REST port. +There is no TLS support and no documented "front it with a proxy" guidance. + +Concrete consequences: + +- The RESP `AUTH` password is sent verbatim over the wire — passive sniffing on + any network segment yields the credential. +- REST API uses HTTP Bearer tokens (`rest_server.rs:106-115`); same story. +- Every key and value transits in cleartext, including data placed via SET + from clients in another datacenter or VPC peer. + +For a Redis-compatible KV intended to be reachable across hosts (the README +documents Docker run patterns), no-TLS is a noticeable gap. Redis itself +supports TLS since 6.0. + +## Fix + +Add optional TLS termination for both servers using `tokio-rustls`: + +1. New CLI flags: `--tls-cert `, `--tls-key `, optional + `--tls-ca-file ` (for client cert verification), `--tls-port ` + to bind a TLS-only listener (so plaintext can stay for local clients). +2. Mirror flags for REST: `--rest-tls-cert`, `--rest-tls-key`, etc., or share + the same cert if both ports want TLS. +3. Use `axum-server` with rustls for the REST side; thread a + `tokio_rustls::TlsAcceptor` into the RESP accept loop in `resp_server.rs` + for the RESP side. +4. Document `tlsv1.2`+ minimum, support PEM cert reload via SIGHUP if cheap to + wire up. + +Add an integration test that performs a TLS handshake and a `PING`. + +This is a non-trivial feature; consider scoping it as a separate PR after the +higher-severity tickets land. + +## Verification + +- New integration test using a self-signed cert. +- `cargo clippy --quiet -- -D warnings` clean with new feature. +- README and `www/app/views/docs/getting_started.html.slv` updated. diff --git a/tasks/todo/SEC-009-cluster-gossip-unauthenticated.md b/tasks/todo/SEC-009-cluster-gossip-unauthenticated.md new file mode 100644 index 0000000..cdb8879 --- /dev/null +++ b/tasks/todo/SEC-009-cluster-gossip-unauthenticated.md @@ -0,0 +1,51 @@ +# SEC-009 — Cluster gossip messages are unauthenticated, allowing topology spoofing on any reachable bus port + +**Severity:** Medium + +## Location + +- `crates/solikv-cluster/src/gossip.rs:102-178` (`GossipMessage::encode` / `decode` — plain space-separated text) +- `crates/solikv-cluster/src/gossip.rs:300-328` (`handle_meet`, `handle_update` — overwrite node info with no verification) +- `crates/solikv-cluster/src/gossip.rs:331-389` (`start_gossip_server` — no shared secret, no signature) + +## Issue + +Gossip messages have zero integrity protection: any peer that connects to the +cluster bus port can send `MEET ` and `UPDATE + ` packets, and the receiver applies them blindly via +`handle_update`. Specifically `handle_update` (line 304) overwrites the +`ip` / `port` fields of an existing node — meaning a malicious peer can +redirect cluster traffic for any node ID it can guess or learn. + +Mitigations *currently in place*: + +- Listener binds to `127.0.0.1` only (`gossip.rs:336`). +- Non-loopback peers are rejected unless their IP is already in the known-nodes + list (`gossip.rs:352-364`). + +These help when the cluster runs on a single host, but the moment the operator +exposes the bus port (multi-host cluster, container with mapped port, +mis-configured firewall), there is no second line of defense — no shared +cluster secret, no HMAC, no TLS. Compare with Redis cluster's `cluster-auth` +shared key. + +## Fix + +1. Add `--cluster-secret ` (also via env / file per SEC-005). When + set, every gossip message is wrapped with an HMAC-SHA256 over the message + bytes; receivers reject messages whose MAC does not verify or that lack a + MAC. +2. While in there, switch the wire format to a length-prefixed framed format + so the 1024-byte read buffer (`gossip.rs:346`) does not silently truncate + long UPDATE messages — see SEC-014. +3. Document that `--cluster-secret` is **required** when running across more + than one host. Refuse to start with `cluster_enabled` + non-loopback bind + + no secret (mirroring SEC-007). +4. Validate `ip` and `port` in `handle_update` / `handle_meet`: parse as a + real `SocketAddr`, refuse loopback↔public swaps mid-session, refuse port + 0 / >65535. + +## Verification + +- New unit: signed message accepted; unsigned/tampered rejected. +- New unit: `handle_update` with malformed IP refuses to overwrite. diff --git a/tasks/todo/SEC-010-resp-allocation-amplification.md b/tasks/todo/SEC-010-resp-allocation-amplification.md new file mode 100644 index 0000000..9e36c32 --- /dev/null +++ b/tasks/todo/SEC-010-resp-allocation-amplification.md @@ -0,0 +1,51 @@ +# SEC-010 — RESP decoder pre-allocates from untrusted lengths, enabling per-connection memory amplification + +**Severity:** Medium + +## Location + +- `crates/solikv-resp/src/codec.rs:227-269` (`decode_array`, `Vec::with_capacity(count)` on line 254) +- `crates/solikv-resp/src/codec.rs:188-225` (`decode_bulk_string`, `Bytes::copy_from_slice` of length-prefixed buffer) +- `crates/solikv-resp/src/codec.rs:131` (`MAX_ARRAY_LEN = 1_048_576`) + +## Issue + +Two amplification vectors: + +1. **Array preallocation.** `decode_array` accepts any `count ≤ MAX_ARRAY_LEN` + (1 048 576) and immediately calls `Vec::with_capacity(count)`. Each + `RespFrame` is at least 24 B on 64-bit (enum tag + `Bytes` overhead). At + the cap, a single `*1048576\r\n` line followed by no further data forces + the server to reserve ≥ 24 MiB before any data has even arrived. + Multiplied by `MAX_CONNECTIONS = 10 000` this is 240 GiB of reservation + from idle attackers. + +2. **Bulk-string copy on speculative length.** `decode_bulk_string` accepts + any length up to `MAX_BULK_STRING_LEN = 512 MiB`. The framed read buffer is + capped at 256 MiB (`resp_server.rs:76`), so 512 MiB inputs cannot complete + — but `Bytes::copy_from_slice(&src[..len])` still runs once `total_needed` + is satisfied, which means up to a 256 MiB heap allocation per legitimate + in-flight request. + +In aggregate, an authenticated attacker can drive memory pressure several +orders of magnitude above the actual data bandwidth they consume. + +## Fix + +1. Replace `Vec::with_capacity(count)` in `decode_array` with `Vec::new()` and + let it grow on `push`. The amortized cost is the same; the worst-case + allocation is bounded by what the attacker actually sends. +2. Lower the per-frame caps to something realistic. Suggested: + - `MAX_BULK_STRING_LEN = 64 MiB` (128 KiB is more typical for K/V workloads + but 64 MiB matches Redis client buffer limits). + - `MAX_ARRAY_LEN = 1 048 576` is fine to keep as an upper bound, but + additionally cap *aggregate* allocation across all frames in a single + `BytesMut` — i.e. add a per-connection memory budget. +3. Optionally make the limits configurable via CLI (`--proto-max-bulk-len`, + `--client-output-buffer-limit`). + +## Verification + +- Unit: feeding `*1048576\r\n` plus a minimal payload no longer triggers a + multi-MB allocation immediately. +- Existing parser tests still pass. diff --git a/tasks/todo/SEC-011-rdb-import-unbounded-alloc.md b/tasks/todo/SEC-011-rdb-import-unbounded-alloc.md new file mode 100644 index 0000000..fe25d04 --- /dev/null +++ b/tasks/todo/SEC-011-rdb-import-unbounded-alloc.md @@ -0,0 +1,51 @@ +# SEC-011 — Redis RDB importer allocates `Vec` of attacker-controlled length, allowing OOM via crafted dump file + +**Severity:** Medium + +## Location + +- `crates/solikv-persist/src/redis_rdb/parser.rs:152-184` (`read_string`) +- `crates/solikv-persist/src/redis_rdb/parser.rs:187-218` (`read_string_raw`) +- `crates/solikv-persist/src/redis_rdb/parser.rs:108-115` (64-bit length encoding read directly into `usize`) + +## Issue + +The RDB parser reads length-prefix fields and immediately allocates: + +```rust +let mut buf = vec![0u8; len]; // line 155, 178, 190, 213 +self.read_exact(&mut buf)?; +``` + +`len` for the 64-bit case (lines 110-115) is `u64::from_be_bytes(...) as usize`, +i.e. the raw value from the file with no upper bound and a silently-truncating +cast on 32-bit targets. A malicious `dump.rdb` with `len = 0x7FFF_FFFF_FFFF_FFFF` +triggers an immediate `vec![0u8; 9_223_372_036_854_775_807]`, which either +panics on allocation failure or pages the box into the abyss. + +Trust model: the import is enabled via the operator-supplied +`--import-redis-rdb ` flag (`main.rs:308`), so the attacker must convince +the operator to import a hostile file. That is exactly the migration scenario +the README advertises ("Generate a dump from your Redis instance and load it at +startup") — operators *do* import files received from third parties. + +The same pattern shows up in `read_string`'s LZF branch (`compressed_len` on +line 178) and again in `skip` (line 252) where `n` is a parameter to the +function but originates from file lengths. + +## Fix + +1. Define a sane maximum-string length constant (suggested 1 GiB, well beyond + real Redis values) and reject larger values with an `InvalidData` error + *before* allocating. +2. Same for `compressed_len` and `uncompressed_len` (LZF path). +3. Use `Vec::try_reserve_exact` followed by manual `set_len` after `read_exact`, + so allocation failure returns `Err` instead of aborting. +4. For the 64-bit length encoding, reject values that exceed the cap *or* that + don't fit in `usize` on the current target. + +## Verification + +- New test: feed a synthesized RDB header with an oversized `0b10_000001` + length and assert the importer returns `Err`, not panic. +- Existing import tests still pass. diff --git a/tasks/todo/SEC-012-cluster-meet-ssrf.md b/tasks/todo/SEC-012-cluster-meet-ssrf.md new file mode 100644 index 0000000..998c8d5 --- /dev/null +++ b/tasks/todo/SEC-012-cluster-meet-ssrf.md @@ -0,0 +1,62 @@ +# SEC-012 — `CLUSTER MEET` lets any authenticated client direct the server to connect to arbitrary host:port (SSRF) + +**Severity:** Medium + +## Location + +- `crates/solikv-server/src/resp_server.rs:392-411` (`CLUSTER MEET` subcommand handler) +- `crates/solikv-cluster/src/cluster.rs:90-99` (`ClusterManager::meet`) +- `crates/solikv-cluster/src/gossip.rs:300-302` (`GossipState::handle_meet`) + +## Issue + +When cluster mode is enabled, any authenticated RESP client can send + +``` +CLUSTER MEET 169.254.169.254 80 +CLUSTER MEET internal-redis.prod.svc.cluster.local 6379 +CLUSTER MEET 127.0.0.1 22 +``` + +and the server will register that endpoint as a cluster peer, which (once the +gossip ping/pong loop kicks in) causes it to open outbound TCP connections to +the supplied address. Concretely this gives the attacker: + +- **SSRF probing** of the server's internal network — useful against IMDS + endpoints (`169.254.169.254`), private services, Kubernetes service IPs, + cluster-internal DNS. +- **Topology poisoning** — the new peer can reply with arbitrary `UPDATE` + messages and reshape slot ownership (see SEC-009 for the integrity gap). +- **Connection amplification** to internal services (a side-channel for port + scanning the server's network). + +Today the only restrictions are: + +- The IP arrives via `arg_str` and is not validated (`resp_server.rs:401`). +- `port` parses to `u16`, defaulting to 7000 on failure. + +There is no allow-list, no DNS-rebinding protection, no refusal of loopback +or link-local targets. + +## Fix + +1. Validate the `ip` argument: + - Parse as `IpAddr` (reject hostnames, or only allow them when an explicit + opt-in flag is set). + - Reject loopback (unless we ourselves bind loopback), link-local + (`169.254.0.0/16`), multicast, broadcast, unspecified (`0.0.0.0`). + - Reject the metadata-service ranges by default (`169.254.169.254`, + `fd00:ec2::254`). +2. Optionally introduce an operator-provided allow-list: + `--cluster-meet-allow [,...]`. +3. When cluster auth is enabled (per SEC-009), require the new peer to + complete the gossip-secret handshake before any outbound traffic to it is + considered "real" cluster traffic. +4. Audit the engine path and disallow `CLUSTER MEET` for non-admin clients + once a future ACL system exists (out of scope here, file follow-up). + +## Verification + +- Unit: `CLUSTER MEET 169.254.169.254 80` returns + `ERR cluster meet target rejected (link-local)`. +- Unit: `CLUSTER MEET 10.0.0.5 6379` succeeds (RFC1918 is allowed by default). diff --git a/tasks/todo/SEC-013-no-auth-rate-limit.md b/tasks/todo/SEC-013-no-auth-rate-limit.md new file mode 100644 index 0000000..0a355c4 --- /dev/null +++ b/tasks/todo/SEC-013-no-auth-rate-limit.md @@ -0,0 +1,41 @@ +# SEC-013 — No rate limiting on RESP `AUTH` or REST Bearer attempts; brute-forceable passwords + +**Severity:** Medium + +## Location + +- `crates/solikv-server/src/resp_server.rs:163-191` (RESP `AUTH` handler — no failure counter, no delay) +- `crates/solikv-server/src/rest_server.rs:90-122` (REST `auth_middleware` — no failure counter, no delay) +- `crates/solikv-server/src/resp_server.rs:19` (`MAX_CONNECTIONS = 10_000`) + +## Issue + +Both authentication paths use a constant-time string comparison (good) but +neither imposes a delay on failure, locks out a misbehaving peer IP, nor caps +the number of attempts per connection. A client can pipeline thousands of +`AUTH ` per TCP connection (RESP processes commands in batch) and open +up to 10 000 simultaneous connections. + +With a typical 8-character ASCII-printable password (~52 bits of entropy) +this is still computationally infeasible offline, but with a *weak* password +(dictionary word, defaulted secret, leaked from another system) it is brute- +forceable in minutes from a LAN attacker. + +The REST side is even worse: each request goes through `auth_middleware` +independently with no per-IP throttling. + +## Fix + +1. After N (suggested: 5) failed `AUTH` attempts on a single connection, + close it and emit a structured warning log. +2. Add a global token-bucket per peer-IP for failed AUTH (suggested: 30 + failures/minute, then 1-second delay before the response). Use a + `dashmap::DashMap` cleaned up periodically to bound + memory. +3. Apply the same logic to the REST middleware: after the per-IP failure + threshold, return 429 Too Many Requests with `Retry-After`. + +## Verification + +- Unit: 6 wrong AUTHs on the same connection ⇒ connection closed after 5th. +- Unit: 31 wrong AUTHs from one IP ⇒ subsequent requests delayed/rejected. diff --git a/tasks/todo/SEC-014-gossip-fixed-1k-buffer.md b/tasks/todo/SEC-014-gossip-fixed-1k-buffer.md new file mode 100644 index 0000000..a3fc55b --- /dev/null +++ b/tasks/todo/SEC-014-gossip-fixed-1k-buffer.md @@ -0,0 +1,44 @@ +# SEC-014 — Gossip server uses a fixed 1024-byte read buffer with no framing, silently truncating long messages + +**Severity:** Low + +## Location + +- `crates/solikv-cluster/src/gossip.rs:346` (`let mut buf = [0u8; 1024];`) +- `crates/solikv-cluster/src/gossip.rs:368-379` (`stream.read(&mut buf)` then `decode(&buf[..n])`) + +## Issue + +The gossip listener reads a single TCP `read()` worth of bytes (≤ 1024) and +hands them to `GossipMessage::decode`. This has two consequences: + +1. **Truncation when packets are larger than 1024 B.** A future + `UPDATE ` with a long + flags list, or a node ID longer than expected, simply gets cut. `decode` + then returns `None` and the message is silently dropped — there is no + reassembly across reads. + +2. **Boundary fragility.** Two messages arriving back-to-back may be coalesced + by TCP into a single read; `decode` parses only the first line and + discards the second. + +With a reasonable cluster, the impact today is "occasional missed gossip, +re-sent next tick". But once `GossipMessage` carries authentication tags +(see SEC-009), TLS, or cluster-state diffs, this fragility becomes a real +problem. + +## Fix + +1. Switch to a length-prefixed framing (e.g. 2-byte BE length + payload) and + keep a per-connection accumulator buffer. Use `tokio_util::codec::Framed` + with a small custom codec, or hand-roll it. +2. Cap maximum frame size at e.g. 16 KiB to bound memory. +3. Make the per-connection buffer grow to that cap rather than being a + stack-allocated 1024-byte array shared across the loop iteration. + +## Verification + +- Unit: a synthesized 2 KB UPDATE message round-trips (encode → split into + two TCP reads → decode) successfully. +- Manual: introduce a long node-id and watch tracing logs to confirm no + truncation warnings. diff --git a/tasks/todo/SEC-015-cluster-dump-unwrap-panics.md b/tasks/todo/SEC-015-cluster-dump-unwrap-panics.md new file mode 100644 index 0000000..5492c27 --- /dev/null +++ b/tasks/todo/SEC-015-cluster-dump-unwrap-panics.md @@ -0,0 +1,51 @@ +# SEC-015 — `cluster_dump` panics on malformed CLI input via `addr.parse().unwrap()` and similar + +**Severity:** Low + +## Location + +- `crates/solikv-server/src/cluster_dump.rs:163` — `TcpStream::connect_timeout(&addr.parse().unwrap(), ...)` +- `crates/solikv-server/src/cluster_dump.rs:267-271` — `a[0].parse::().unwrap().cmp(...)` +- `crates/solikv-cluster/src/gossip.rs:248-251, 287-290` — `SystemTime::...duration_since(UNIX_EPOCH).unwrap()` +- `crates/solikv-cluster/src/cluster.rs:268-270` — same `.unwrap()` on slot parse during `cluster_slots()` sort + +## Issue + +Several call sites unwrap parser results that originate from operator- or +peer-supplied input. They will not corrupt data, but they crash the server (or +the standalone dump/restore tool) on malformed input: + +- `cluster_dump --cluster-connect "not a host"` ⇒ panic in + `addr.parse().unwrap()`. +- A `CLUSTER NODES` response with a non-numeric slot field ⇒ panic in the sort + closure. +- A system clock that goes backwards before UNIX epoch (very unlikely on + servers, but possible on misconfigured VMs / containers booting with bad + RTC) ⇒ every gossip pong-handler tick panics. + +This pattern is also a future-availability risk: any new code path that +forwards untrusted strings into one of these unwrap sites becomes a remote +DoS. + +## Fix + +Replace `.unwrap()` on parser results with explicit error handling: + +```rust +let socket: SocketAddr = addr.parse() + .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, + format!("bad address {addr:?}: {e}")))?; +``` + +For the slot sort, use `unwrap_or(0)` (or filter out unparseable slots before +sorting). For SystemTime, use `unwrap_or_default()` — a zero offset is a +sane fallback for a clock that briefly reports pre-epoch. + +Add `#[deny(clippy::unwrap_used)]` at crate level on +`solikv-server` and `solikv-cluster` to keep this from regressing +(allow on tests only). + +## Verification + +- Unit: invalid `--cluster-connect` returns an `Err`, not a panic. +- `cargo clippy --quiet -- -D warnings` clean (with the new deny).