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

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 / auth ordering

read_frame (crates/daemon/src/protocol.rs:126) allocates the full body buffer (vec![0u8; len as usize], up to MAX_FRAME_BYTES = 16 MiB) and runs serde_json over the entire body, and only afterwards does serve_connection check the capability token (crates/daemon/src/server.rs:180, token check at :192).

The per-frame cap is enforced correctly — protocol.rs:121 rejects len > MAX_FRAME_BYTES before the vec!, so a claimed-4 GiB frame does not OOM. The residual hole is that an unauthenticated local peer can still force a 16 MiB allocation + full JSON parse per connection, with zero credentials. Combined with the lack of a connection cap (see the sibling issue), this is a cheap local memory/CPU exhaustion vector.

Fix

Gate expensive work behind the token check: read the frame header, verify the token before parsing/allocating the full body, or keep the auth handshake to small frames.
Should be fixed together with the concurrent-connection cap issue.

Impact / scope

Loopback-only (127.0.0.1). Matters on shared / multi-user hosts. Not memory-unsafety, not write-corruption.

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 / auth ordering `read_frame` (`crates/daemon/src/protocol.rs:126`) allocates the full body buffer (`vec![0u8; len as usize]`, up to `MAX_FRAME_BYTES` = 16 MiB) and runs `serde_json` over the entire body, and only *afterwards* does `serve_connection` check the capability token (`crates/daemon/src/server.rs:180`, token check at `:192`). The per-frame cap is enforced correctly — `protocol.rs:121` rejects `len > MAX_FRAME_BYTES` *before* the `vec!`, so a claimed-4 GiB frame does **not** OOM. The residual hole is that an **unauthenticated** local peer can still force a 16 MiB allocation + full JSON parse per connection, with zero credentials. Combined with the lack of a connection cap (see the sibling issue), this is a cheap local memory/CPU exhaustion vector. ### Fix - Gate expensive work behind the token check: read the frame header, verify the token before parsing/allocating the full body, or keep the auth handshake to small frames. - Should be fixed together with the concurrent-connection cap issue. ### Impact / scope - Loopback-only (`127.0.0.1`). Matters on shared / multi-user hosts. Not memory-unsafety, not write-corruption. --- _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

labels

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

buildagent commented

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

Author

Member

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

Reassessed on current master: the ordering was still present (token rides inside the request body, so it genuinely can't be checked before parse), but severity had already dropped because FRAME_BODY_TIMEOUT (30s) bounds the hold. Rather than a protocol change, the fix removes the amplification: requests are now read under MAX_REQUEST_FRAME_BYTES = 1 MiB (protocol.rs), so the buffer an unauthenticated peer can force the daemon to eagerly allocate before the token is seen drops from 16 MiB → 1 MiB (16×). read_frame_capped rejects an oversized declared length on the 4-byte prefix alone, before any allocation. Sibling connection-cap landed in the same commit (#11).

Tests: protocol::read_frame_capped_rejects_len_over_cap_before_body + auth_e2e::oversized_request_frame_is_rejected_before_body (raw socket declares 4 MiB, sends no body, asserts the server closes within 5s rather than waiting the 30s body timeout).

Fixed in `0f4bfe7` (branch `fix/ultradeep-review-findings`). Reassessed on current master: the ordering was still present (token rides inside the request body, so it genuinely can't be checked before parse), but severity had already dropped because `FRAME_BODY_TIMEOUT` (30s) bounds the hold. Rather than a protocol change, the fix removes the **amplification**: requests are now read under `MAX_REQUEST_FRAME_BYTES` = 1 MiB (`protocol.rs`), so the buffer an unauthenticated peer can force the daemon to eagerly allocate before the token is seen drops from 16 MiB → 1 MiB (16×). `read_frame_capped` rejects an oversized declared length on the 4-byte prefix alone, before any allocation. Sibling connection-cap landed in the same commit (#11). **Tests:** `protocol::read_frame_capped_rejects_len_over_cap_before_body` + `auth_e2e::oversized_request_frame_is_rejected_before_body` (raw socket declares 4 MiB, sends no body, asserts the server closes within 5s rather than waiting the 30s body timeout).

buildagent referenced this issue

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

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

buildagent closed this issue

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