daemon: no cap on concurrent connections + unbounded outer read wait (local DoS) #11

New issue

Closed

opened 2026-07-07 13:19:31 +02:00 by buildagent · 1 comment

buildagent commented

2026-07-07 13:19:31 +02:00

Member

Severity: High · Category: DoS

accept_loop (crates/daemon/src/server.rs:153-166) spawns one unbounded tokio task per accepted socket. active_connections is fetch_add/fetch_sub-bracketed but never read to reject — it is a dead metric (only last_activity drives idle-shutdown). A local peer can open thousands of sockets; each can sit in read_frame's unbounded outer wait for the next length prefix (protocol.rs:104-105 documents this as deliberate — only the body read is bounded by FRAME_BODY_TIMEOUT), holding a task + fd + a potential 16 MiB buffer (see sibling frame-before-auth issue).

Fix

Acquire a tokio::sync::Semaphore permit (e.g. 8–16) before serve_connection; drop over-cap connections.
Add an idle-connection read timeout on the outer length-prefix read (a per-connection "no frame within T → close"), not just the body read.
Consider an RAII guard for active_connections so a handler panic can't skew the gauge (server.rs:154/165).

Impact / scope

Loopback-only. Compounds with the frame-before-auth issue into a cheap unauthenticated exhaust.

Filed from a deep multi-agent code review of the workspace (v0.5.7). Severity/category per the review; file:line refs were accurate at review time — verify against current master.

**Severity:** High · **Category:** DoS `accept_loop` (`crates/daemon/src/server.rs:153-166`) spawns one unbounded tokio task per accepted socket. `active_connections` is `fetch_add`/`fetch_sub`-bracketed but **never read to reject** — it is a dead metric (only `last_activity` drives idle-shutdown). A local peer can open thousands of sockets; each can sit in `read_frame`'s **unbounded** outer wait for the next length prefix (`protocol.rs:104-105` documents this as deliberate — only the *body* read is bounded by `FRAME_BODY_TIMEOUT`), holding a task + fd + a potential 16 MiB buffer (see sibling frame-before-auth issue). ### Fix - Acquire a `tokio::sync::Semaphore` permit (e.g. 8–16) before `serve_connection`; drop over-cap connections. - Add an idle-connection read timeout on the **outer** length-prefix read (a per-connection "no frame within T → close"), not just the body read. - Consider an RAII guard for `active_connections` so a handler panic can't skew the gauge (`server.rs:154/165`). ### Impact / scope - Loopback-only. Compounds with the frame-before-auth issue into a cheap unauthenticated exhaust. --- _Filed from a deep multi-agent code review of the workspace (v0.5.7). Severity/category per the review; file:line refs were accurate at review time — verify against current `master`._

buildagent added the

severity/high

dos

code-review

labels

2026-07-07 13:19:31 +02:00

buildagent referenced this issue

2026-07-07 14:22:38 +02:00

daemon: 16 MiB RPC frame allocated + JSON-parsed BEFORE auth check (unauthenticated local DoS) #10

buildagent commented

2026-07-07 14:22:45 +02:00

Author

Member

Fixed in 0f4bfe7 (branch fix/ultradeep-review-findings).

Reassessed on current master — all three claims held: unbounded task-per-socket, active_connections a dead metric never read to reject, unbounded outer length-prefix wait. Fix:

Connection cap: a Semaphore(64) in accept_loop; over-cap sockets are closed immediately (try_acquire_owned → drop) rather than queued. One legit client uses a handful of connections, so normal traffic never reaches the cap.
Outer idle-read timeout: 300s CONNECTION_IDLE_TIMEOUT wraps only the between-requests wait (never an in-flight request); a quiet connection frees its slot and the client's RpcIndex reconnects transparently (covered by reconnect_e2e). Reaps idle squatters well before the 30-min daemon idle-shutdown.
RAII gauge: ConnGauge replaces the manual fetch_add/fetch_sub so the active_connections count can't leak on a handler panic (also closes the #18 sub-item for this gauge).

Combined with #10's 1 MiB request cap, worst-case unauthenticated footprint is bounded to 64 × 1 MiB. Full daemon suite green (193 tests).

Fixed in `0f4bfe7` (branch `fix/ultradeep-review-findings`). Reassessed on current master — all three claims held: unbounded task-per-socket, `active_connections` a dead metric never read to reject, unbounded outer length-prefix wait. Fix: - **Connection cap**: a `Semaphore(64)` in `accept_loop`; over-cap sockets are closed immediately (`try_acquire_owned` → drop) rather than queued. One legit client uses a handful of connections, so normal traffic never reaches the cap. - **Outer idle-read timeout**: 300s `CONNECTION_IDLE_TIMEOUT` wraps only the between-requests wait (never an in-flight request); a quiet connection frees its slot and the client's `RpcIndex` reconnects transparently (covered by `reconnect_e2e`). Reaps idle squatters well before the 30-min daemon idle-shutdown. - **RAII gauge**: `ConnGauge` replaces the manual `fetch_add`/`fetch_sub` so the `active_connections` count can't leak on a handler panic (also closes the #18 sub-item for this gauge). Combined with #10's 1 MiB request cap, worst-case unauthenticated footprint is bounded to 64 × 1 MiB. Full daemon suite green (193 tests).

buildagent closed this issue

2026-07-07 14:22:49 +02:00

buildagent referenced this issue

2026-07-07 14:55:04 +02:00

Polish backlog: low-severity robustness/coverage items from the code review #18

buildagent referenced this issue

2026-07-07 15:09:53 +02:00

Polish backlog: low-severity robustness/coverage items from the code review #18