Security: fix critical vulnerabilities (v0.11.5) - #37
Merged
Conversation
Closes #31 #35 #36. #31 CONFIG SET - Stop returning OK without applying changes. Return ERR explaining the parameter must be set via the matching CLI flag at startup. Silent no-op was actively misleading. #35 CI hygiene - Trim tokio features from "full" to the eight actually used (rt-multi-thread, net, io-util, sync, time, macros, signal, fs) - Trim hyper to ["server", "http1"]; hyper-util to ["tokio"] - Tag legitimate-but-unused API surface with #[allow(dead_code)] so #![warn(dead_code)] at the crate root catches accidental dead code from now on - Add .github/workflows/ci.yml running cargo check + clippy (-D warnings) + test + fmt on every PR and push to main #36 Low-impact perf cleanup - maxmemory_policy: borrow instead of cloning the String per write - ZRANGEBYSCORE: fold take_while + filter into a single filter_map - GETRANGE: zero-copy via Bytes::slice instead of slice.to_vec() - MGET: iterate args by index instead of building an intermediate Vec Bump version 0.11.0 -> 0.11.1.
Closes #30 #32 #33. #30 AUTH brute-force protection - Per-connection consecutive-failure counter, reset on success - After 5 failures, exponential backoff (100ms doubling, capped 5s) applied before returning WRONGPASS - handle_auth / handle_auth_hello are now async and consult ACL via proper .await locking instead of try_lock (the old try_lock could spuriously fail under contention and report WRONGPASS) - Extract AclMatch enum to clean up the auth flow #32 Information-disclosure cleanup - Skip recording latency stats and command-name entries when the response is NOAUTH or WRONGPASS; unauthenticated probes can no longer populate the histograms visible via INFO commandstats / LATENCY HISTORY #33 Pub/Sub dead-sender cleanup - PubSubManager::publish now takes &mut self and prunes any client whose Sender returns an error from send() (channel closed). This removes the slow leak on abrupt-disconnect paths where the normal cleanup_pubsub didn't run Bump version 0.11.1 -> 0.11.2.
Closes #27 (with follow-up note below). Add src/command/parse.rs with strict numeric helpers (int, u64_, usize_, float) that use std::str::from_utf8 instead of from_utf8_lossy. The old chain allocated a Cow + an owned String per call and silently accepted malformed UTF-8 in numeric arguments. 96 sites converted across: - sorted_set.rs (12), list.rs (14), geo.rs (14) - strings.rs (13), bitmap.rs (8) - stream.rs (9), keys.rs (5), set.rs (3) - server_cmds.rs (3), hash.rs (3), cluster.rs (3) - scripting.rs (3), scan.rs (4), advanced.rs (1), json.rs (1) Sites left in place (~20): - `.parse().unwrap_or(...)` defaulting calls (no error path to migrate) - `parse::<u8>()` in bitmap.rs (no u8 helper yet) - Non-template variants inside nested closures and Result-returning helpers — these need per-site review and were out of scope here Bump version 0.11.2 -> 0.11.3.
Closes #29. SortedSetData.scores was BTreeMap<ScoreKey, Bytes> where the value was always Bytes::new() — a per-insert no-op allocation kept around so the type could be a Map. Migrate to BTreeSet<ScoreKey>, which expresses what the code actually wanted (an ordered set of keys) and removes: - one Bytes::new() per insert - the (_, _) tuple destructuring in iter/range/skip paths Iter patterns shift from |(k, _)| -> |k|. .keys() becomes .iter(). External callers (http.rs serializer) updated accordingly. Bump version 0.11.3 -> 0.11.4.
Unify ACL enforcement, gate HTTP command execution, sandbox scripting, and bound attacker-controlled allocations. - ACL: make the acl.rs registry the single source of truth so runtime ACL SETUSER changes are actually enforced; drop the disconnected SharedState.acl_users snapshot and dead AclUser/AclMatch types. - HTTP: resolve the bearer token to a user and enforce per-user command and key ACLs on /api/v1/command and the key endpoints (403 on denial). - Scripting: add a 256MB memory limit and 5s wall-clock timeout, block nested EVAL/FCALL/SCRIPT/FUNCTION (recursion crash), and make the _RO variants reject write commands. - Allocation caps: bound SETBIT/BITFIELD bit offsets and SETRANGE length (512MB) with checked arithmetic; cap CMS/TopK width*depth*8 at 256MB. - Connection: cap the per-connection query buffer at 1GB. - AOF: bound multibulk count and bulk length before allocation so a tampered appendonly.aof cannot OOM/panic on startup.
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
1 participant
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.
Fixes the six Critical findings from the security audit. Version bumped to 0.11.5.
cargo checkis clean (no warnings); all unit + integration tests pass except two pubsub integration tests in the untracked WIPtests/dir that fail identically onmain(pre-existing, not touched here).Findings addressed
1. ACL enforcement was non-functional. Runtime
ACL SETUSERwrote to a static registry while the auth/enforcement path read a separate startup-only snapshot, so no runtime rule was ever enforced and ACL-created users could never authenticate. Theacl.rsregistry is now the single source of truth; the connection and HTTP layers read/write it via new public functions. Removed the disconnectedSharedState.acl_usersmap and the deadAclUser/AclMatchtypes.2. HTTP
/api/v1/commandran arbitrary commands with no authorization.check_authnow resolves the bearer token to a username, and the command + key endpoints enforce the same per-user command and key-pattern ACL as the RESP path (403on denial).3. Scripting engine could DoS the server and bypass read-only intent. Added a 256 MB Lua memory limit and a 5 s wall-clock timeout (instruction hook), blocked nested
EVAL/EVALSHA/FCALL/FUNCTION/SCRIPT(unbounded recursion → stack-overflow abort), and madeEVAL_RO/EVALSHA_RO/FCALL_ROgenuinely reject write commands.4. Attacker-controlled allocations (multi-GB/TB OOM).
SETBIT/BITFIELDreject bit offsets past 512 MB (checked#-form multiply);SETRANGEcaps result length withchecked_add;CMS.INITBYDIM/INITBYPROBandTOPK.RESERVEvalidatewidth × depth × 8 ≤ 256 MBwith checked arithmetic.5. Unbounded per-connection query buffer. Capped at 1 GB; oversized/incomplete frames get an error + disconnect.
6. AOF replay could OOM/panic on a tampered file. Multibulk count (≤ 1,048,576) and bulk length (≤ 512 MB) are validated before allocation.
Known residual
Per-user ACL attribution of individual
redis.call()invocations inside a script is still not wired (CommandContextcarries no user identity — a broader refactor). Mitigated: runningEVALis ACL-gated at the connection/HTTP layer and the_ROvariants are now truly read-only. Suggested as a follow-up.High/Medium/Low findings from the audit are intentionally out of scope for this PR.