Skip to content

harden round 3 — overflow, DoS, correctness fixes - #93

Merged
kacy merged 12 commits into
mainfrom
harden-round3
Feb 12, 2026
Merged

harden round 3 — overflow, DoS, correctness fixes#93
kacy merged 12 commits into
mainfrom
harden-round3

Conversation

@kacy

@kacy kacy commented Feb 12, 2026

Copy link
Copy Markdown
Owner

summary

third round of hardening from a full codebase audit. addresses 11 issues across 5 crates:

high severity:

  • saturating arithmetic for EXPIRE/PEXPIRE timestamps (prevents u64 wrap on large TTLs)
  • checked negation for DECRBY i64::MIN (prevents panic on overflow)
  • safe read helpers for gossip message decoding (prevents panic on truncated messages)
  • element count + bulk string length limits in RESP3 parser (prevents allocation bombs)
  • buffer limit and idle timeout for subscriber mode (prevents memory exhaustion)
  • decrement expiry_count when hdel/srem auto-delete empty containers

medium severity:

  • 10s timeout on TLS handshake (prevents slowloris-style connection exhaustion)
  • reject RENAME when keys hash to different shards (prevents silent data loss)
  • saturating_add for gossip incarnation counter (prevents u64 wrap)
  • capped Vec::with_capacity in AOF and snapshot parsing (prevents OOM from corrupt count fields)
  • fsync parent directory after snapshot atomic rename (ensures rename durability)

what was tested

  • full test suite: cargo test -p emberkv-core -p ember-persistence -p ember-server -p ember-cluster -p ember-protocol — all pass
  • cargo clippy --workspace -- -D warnings — clean
  • cargo fmt --all -- --check — clean

design considerations

  • capped_capacity uses 65,536 as the pre-allocation ceiling — large enough for any legitimate single record, small enough to prevent OOM from corrupt u32 values. the loop still iterates the full count, so valid large collections work correctly (just with a few extra reallocations).
  • RENAME cross-shard check uses a new Engine::same_shard() method rather than exposing the internal shard_index function. concurrent mode (DashMap) doesn't need this check since it's thread-safe.
  • directory fsync after snapshot rename uses best-effort (let _ =) — a failure here shouldn't abort the snapshot since the data is already on disk.

kacy added 12 commits February 11, 2026 19:35
EXPIRE seconds * 1000 and PEXPIRE now_ms() + millis could overflow
u64 from user-supplied values. expiry_from_duration also truncated
u128 to u64 without clamping. all three sites now use saturating_mul
and saturating_add.
-i64::MIN overflows in both debug (panic) and release (wraps).
shard.rs now uses checked_neg() and returns an overflow error.
recovery.rs uses saturating_neg() for safe AOF replay.
Buf::get_* methods panic on insufficient data. replaced all bare
get_u8/get_u16_le/get_u64_le calls in decode paths with safe_get_*
helpers that return io::Error. also caps member/update/slot counts
at 1024 to prevent allocation bombs from crafted packets.
a flat array with millions of tiny null elements could amplify ~3
bytes of wire data into ~32 bytes of heap per element. adds
MAX_ARRAY_ELEMENTS (1M) for arrays/maps and MAX_BULK_LEN (512MB,
matching redis) for bulk strings.
the subscriber loop was missing both MAX_BUF_SIZE and IDLE_TIMEOUT
guards that the main connection loop has. a client in subscriber
mode could grow the buffer without bound or hold a connection slot
forever.
when all fields are removed from a hash or all members from a set,
the key is auto-deleted but expiry_count was not decremented if the
key had a TTL. matches the existing correct behavior in zrem and
list_pop.
acceptor.accept() blocked indefinitely, allowing a slowloris attack
on the TLS port to exhaust connection semaphore permits. both
sharded and concurrent mode TLS paths now timeout after 10 seconds.
@kacy
kacy merged commit f7e6ce8 into main Feb 12, 2026
5 of 7 checks passed
@kacy
kacy deleted the harden-round3 branch February 12, 2026 03:27
kacy added a commit that referenced this pull request Feb 19, 2026
* fix: use saturating arithmetic for expire/pexpire timestamps

EXPIRE seconds * 1000 and PEXPIRE now_ms() + millis could overflow
u64 from user-supplied values. expiry_from_duration also truncated
u128 to u64 without clamping. all three sites now use saturating_mul
and saturating_add.

* fix: prevent decrby panic on i64::MIN negation

-i64::MIN overflows in both debug (panic) and release (wraps).
shard.rs now uses checked_neg() and returns an overflow error.
recovery.rs uses saturating_neg() for safe AOF replay.

* fix: prevent panics from truncated gossip messages

Buf::get_* methods panic on insufficient data. replaced all bare
get_u8/get_u16_le/get_u64_le calls in decode paths with safe_get_*
helpers that return io::Error. also caps member/update/slot counts
at 1024 to prevent allocation bombs from crafted packets.

* fix: add element count and bulk string length limits to parser

a flat array with millions of tiny null elements could amplify ~3
bytes of wire data into ~32 bytes of heap per element. adds
MAX_ARRAY_ELEMENTS (1M) for arrays/maps and MAX_BULK_LEN (512MB,
matching redis) for bulk strings.

* fix: add buffer limit and idle timeout to subscriber mode

the subscriber loop was missing both MAX_BUF_SIZE and IDLE_TIMEOUT
guards that the main connection loop has. a client in subscriber
mode could grow the buffer without bound or hold a connection slot
forever.

* fix: decrement expiry_count when hdel/srem auto-deletes empty key

when all fields are removed from a hash or all members from a set,
the key is auto-deleted but expiry_count was not decremented if the
key had a TTL. matches the existing correct behavior in zrem and
list_pop.

* fix: add 10s timeout to TLS handshake

acceptor.accept() blocked indefinitely, allowing a slowloris attack
on the TLS port to exhaust connection semaphore permits. both
sharded and concurrent mode TLS paths now timeout after 10 seconds.

* fix: reject RENAME when keys hash to different shards

* fix: use saturating_add for gossip incarnation counter

* fix: cap pre-allocation from untrusted count fields in aof and snapshot parsing

* fix: fsync parent directory after snapshot atomic rename

* chore: cargo fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant