Security: accept errors retried in tight loop — CPU/log-flood DoS via descriptor exhaustion

[coderabbitai]

Summary

Severity: High
Affected commit: cbf5791
Raised via: PR #323, requested by @leynos

Accept errors in the main TCP server loop are now swallowed and retried immediately. For persistent errors such as EMFILE/ENFILE caused by file-descriptor exhaustion, listener.accept() can return an error immediately and repeatedly. Because there is no connection limit, no backoff, no reserve-fd recovery path, and no shutdown/break on repeated fatal accept errors, a remote unauthenticated attacker can open and hold enough TCP connections to exhaust process file descriptors, after which the daemon spins, burns CPU, and floods logs.

The prior code propagated the accept error from main rather than entering a retry loop, so the tight retry/logging behaviour is introduced by this commit.

---

Evidence

src/main.rs (lines 120–134)

res = listener.accept() => {
    match res {
        Ok((socket, peer)) => {
            let pool = pool.clone();
            let mut rx = shutdown_rx.clone();
            join_set.spawn(async move {
                if let Err(e) = handle_client(socket, peer, pool, &mut rx).await {
                    eprintln!("connection error from {}: {}", peer, e);
                }
            });
        }
        Err(e) => {
            eprintln!("accept error: {}", e);
        }
    }

• The Err branch logs and immediately returns to the accept loop with no backoff, fatal-error classification, or rate limiting.
• Each accepted socket spawns a task without a connection cap, making descriptor exhaustion by many unauthenticated idle connections practical.

---

Attack Path

1. Unauthenticated TCP client opens and holds many TCP connections to mxd on 0.0.0.0:5500 (default).
2. listener.accept() succeeds and spawns one unbounded JoinSet task per socket.
3. Idle clients keep their descriptors open; handle_client writes a banner then waits for input or shutdown signal.
4. Process file descriptors are exhausted; subsequent accept() calls return EMFILE/ENFILE.
5. The error branch logs and immediately retries — no delay, no recovery, no shutdown.
6. CPU burns and logs flood; legitimate clients cannot connect.

---

Impact

Dimension	Assessment
Likelihood	High — unauthenticated, reachable over the primary TCP service, no application-level connection cap or idle timeout.
Impact	High — availability-only: CPU/log flood and service unavailability. No RCE, authentication bypass, or data disclosure.
Severity	High

---

Existing Controls

• No authentication is required to consume a connection slot/file descriptor.
• No global connection limit is present in repository code.
• No per-IP rate limiting is present in repository code.
• No unauthenticated idle timeout is present in repository code.
• No accept-error backoff or log rate-limiting is present in repository code.
• Shutdown signalling exists but does not mitigate attacker-driven descriptor exhaustion.

---

Suggested Remediation Direction

• Introduce an accept-error backoff (e.g. exponential with a ceiling) for persistent/resource errors.
• Classify errors: treat EMFILE/ENFILE as fatal or apply a sleep before retrying.
• Add a global connection semaphore or cap to prevent unbounded task spawning.
• Add an unauthenticated-idle timeout so clients that do not progress through authentication release descriptors promptly.
• Optionally, rate-limit accept-error log emission to prevent log flooding.

---

Assumptions / Blindspots

• No repository IaC, Kubernetes, systemd, or load-balancer manifests were present to confirm real deployment exposure or external connection limits.
• Static review cannot confirm production ulimit values or host-level mitigations.
• The actual mxd binary could not be built in the validation environment due to dependency/network restrictions; however, the source path and a dependency-free harness strongly support reachability.

[leynos]

@CodeRabbit plan

[coderabbitai]

Coding Plan

Summary

• Implement exponential backoff with ceiling for accept errors, classifying EMFILE/ENFILE via raw_os_error() on Unix to trigger delays and prevent CPU spin.
• Introduce a global Semaphore in ServerResources to cap concurrent connections, with permits held by spawned tasks and released on exit.
• Add an unauthenticated idle timeout using a deadline check in the handle_client select loop, disconnecting clients that fail to authenticate promptly.
• Rate-limit error logging to prevent log flooding during sustained resource exhaustion.

Design Choices

Design Choice 1: Scope limited to legacy server

Options Considered:

1. Implement mitigations in both legacy and wireframe servers.
2. Implement mitigations only in the legacy server, noting wireframe as out of scope.

Chosen Option: 2

Rationale: The wireframe server's accept logic is external; changes there require upstream modifications or feature requests to the wireframe crate.

Design Choice 2: Error classification strategy for EMFILE/ENFILE

Options Considered:

1. Classify via raw_os_error() matching specific errno values (platform-specific).
2. Treat all accept errors as potentially resource-related and apply backoff uniformly.
3. Classify by ErrorKind only, applying backoff to Other and WouldBlock.

Chosen Option: 1

Rationale: Match raw_os_error() against libc::EMFILE and libc::ENFILE on Unix platforms for precise classification; fall back to generic backoff for unrecognised errors.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Accept-Error Backoff with Error Classification

Introduce exponential backoff for resource-exhaustion errors in the accept loop, preventing CPU spin and log flooding when file descriptors are exhausted.

Task 1: Define Connection-Management Constants

Add constants governing backoff behaviour to the legacy server module.

• Add ACCEPT_BACKOFF_INITIAL: Duration (e.g., 5 ms) and ACCEPT_BACKOFF_CEILING: Duration (e.g., 1 second) in src/server/legacy/mod.rs.
• Place these alongside existing module-level definitions to keep connection-management concerns co-located.

Task 2: Implement Error Classification Helper

Create a function that identifies resource-exhaustion errors requiring backoff.

• Add a private function is_resource_exhaustion_error(e: &io::Error) -> bool in src/server/legacy/mod.rs.
• On Unix, check e.raw_os_error() against libc::EMFILE and libc::ENFILE.
• Return true for these errors; return false otherwise.
• Use conditional compilation (#[cfg(unix)]) to handle platform differences.

Task 3: Integrate Backoff into Accept Loop

Modify accept_connections to apply exponential backoff on resource errors.

• Introduce mutable backoff_duration state initialised to ACCEPT_BACKOFF_INITIAL.
• On accept error classified as resource-exhaustion, sleep for backoff_duration before continuing; double backoff_duration up to ACCEPT_BACKOFF_CEILING.
• On successful accept, reset backoff_duration to ACCEPT_BACKOFF_INITIAL.
• For non-resource errors, log and continue immediately (existing behaviour).

Task 4: Rate-Limit Error Logging

Prevent log flooding by throttling repeated error messages.

• Track the last log timestamp for accept errors.
• Suppress duplicate log entries if the previous log occurred within a short window (e.g., 1 second).
• Alternatively, log a count of suppressed errors when the window expires.

🤖 Prompt for AI agents

Implement exponential backoff with error classification in the legacy server's
accept loop (`src/server/legacy/mod.rs`). The goal is to prevent CPU spin and
log flooding when file descriptors are exhausted.

**Define constants** at module level alongside existing definitions:
- `ACCEPT_BACKOFF_INITIAL: Duration` (e.g., 5 ms)
- `ACCEPT_BACKOFF_CEILING: Duration` (e.g., 1 second)

**Add a private helper function** `is_resource_exhaustion_error(e: &io::Error)
-> bool`:
- Use `#[cfg(unix)]` conditional compilation
- On Unix, inspect `e.raw_os_error()` and match against `libc::EMFILE` and
`libc::ENFILE`, returning `true` for those values
- Return `false` for all other errors

**Modify `accept_connections`** to integrate backoff:
- Introduce a mutable `backoff_duration` variable initialised to
`ACCEPT_BACKOFF_INITIAL`
- On accept error: if `is_resource_exhaustion_error` returns `true`, sleep for
`backoff_duration`, then double `backoff_duration` up to
`ACCEPT_BACKOFF_CEILING`, then continue the loop
- On accept error: if not resource-exhaustion, log and continue immediately
(existing behaviour)
- On successful accept, reset `backoff_duration` to `ACCEPT_BACKOFF_INITIAL`

**Rate-limit error logging**:
- Track a `last_log_timestamp` for accept errors
- Suppress duplicate log entries if the previous log occurred within a short
window (e.g., 1 second)
- Log a count of suppressed errors when the window expires

Phase 2: Connection Semaphore

Introduce a global connection limit to prevent unbounded task spawning and file-descriptor exhaustion from many concurrent connections.

Task 1: Add Semaphore to Server Resources

Extend ServerResources with a connection-limiting semaphore.

• Define MAX_CONNECTIONS: usize constant (e.g., 512) in src/server/legacy/mod.rs.
• Add connection_semaphore: Arc<Semaphore> field to ServerResources.
• Initialise the semaphore with MAX_CONNECTIONS permits in accept_connections.

Task 2: Acquire Permit Before Spawning Handler

Gate task spawning on semaphore permit acquisition.

• In accept_connections, after successful listener.accept(), call connection_semaphore.clone().acquire_owned().await.
• Pass the OwnedSemaphorePermit into spawn_client_handler (update signature accordingly).
• The permit is held by the spawned task and released automatically when the task exits.

Task 3: Update spawn_client_handler Signature

Modify the function to accept and hold the permit.

• Add permit: OwnedSemaphorePermit parameter to spawn_client_handler.
• Move the permit into the spawned async block so it is dropped when the handler completes.
• No explicit drop required; Rust's ownership ensures release.

Task 4: Expose Semaphore for Test Support

Ensure white-box tests can access or inject the semaphore.

• Under #[cfg(feature = "test-support")], expose the connection_semaphore field publicly on ServerResources.
• Update AcceptContext fixture in tests/integration/server_legacy.rs if necessary.

🤖 Prompt for AI agents

Introduce a connection-limiting semaphore into `src/server/legacy/mod.rs` to cap
concurrent connections and prevent unbounded task spawning. The goal is to bound
file-descriptor usage under load.

**Add a constant** at module level:
- `MAX_CONNECTIONS: usize` (e.g., 512)

**Extend `ServerResources`**:
- Add field `connection_semaphore: Arc<Semaphore>`
- Initialise it with `MAX_CONNECTIONS` permits in `accept_connections`

**Modify `accept_connections`**:
- After a successful `listener.accept()`, call
`connection_semaphore.clone().acquire_owned().await` to obtain an
`OwnedSemaphorePermit`
- Pass the acquired permit into `spawn_client_handler` (update the call site
accordingly)

**Update `spawn_client_handler`**:
- Add `permit: OwnedSemaphorePermit` as a parameter
- Move the permit into the spawned async block; it will be dropped automatically
when the handler completes, releasing the semaphore slot
- No explicit `drop` call is required

**Expose for test support**:
- Under `#[cfg(feature = "test-support")]`, make the `connection_semaphore`
field on `ServerResources` `pub`
- Update the `AcceptContext` fixture in `tests/integration/server_legacy.rs` as
necessary

Phase 3: Unauthenticated Idle Timeout

Disconnect clients that fail to authenticate within a deadline, releasing file descriptors promptly.

Task 1: Define Unauthenticated Timeout Constant

Add a constant for the maximum time an unauthenticated client may remain connected.

• Add UNAUTHENTICATED_TIMEOUT: Duration (e.g., 30 seconds) in src/server/legacy/mod.rs.

Task 2: Track Authentication Deadline in handle_client

Implement deadline-based disconnection for unauthenticated sessions.

• At the start of handle_client, compute auth_deadline = Instant::now() + UNAUTHENTICATED_TIMEOUT.
• In the main tokio::select! loop, add a branch that fires when session.user_id.is_none() and Instant::now() >= auth_deadline.
• When this branch fires, log a warning and break out of the loop, closing the connection.

Task 3: Reset or Disable Timeout After Authentication

Once authenticated, remove the deadline constraint.

• After processing a command that sets session.user_id to Some(...), the deadline check naturally becomes inactive because the guard condition session.user_id.is_none() is no longer true.
• Ensure the select! branch uses an if guard so it is skipped for authenticated sessions.

Task 4: Update Existing Tests

Ensure tests that exercise connection handling account for the new timeout.

• Review src/server/legacy/unit_tests.rs and tests/integration/server_legacy.rs.
• For tests that do not authenticate, ensure they complete or disconnect before the unauthenticated timeout.
• Add a test case verifying that unauthenticated connections are closed after UNAUTHENTICATED_TIMEOUT.

🤖 Prompt for AI agents

Add an unauthenticated idle timeout to `src/server/legacy/mod.rs` to disconnect
clients that fail to authenticate within a deadline. The goal is to release file
descriptors promptly from stalled connections.

**Add a constant** at module level:
- `UNAUTHENTICATED_TIMEOUT: Duration` (e.g., 30 seconds)

**Modify `handle_client`**:
- At the start of the function, compute `auth_deadline = Instant::now() +
UNAUTHENTICATED_TIMEOUT`
- In the main `tokio::select!` loop, add a branch that fires on
`tokio::time::sleep_until(auth_deadline.into())` (or equivalent), guarded by `if
session.user_id.is_none()`
- When this branch fires, log a warning and `break` out of the loop to close the
connection
- The guard condition `session.user_id.is_none()` ensures the branch is skipped
automatically once authentication succeeds — no explicit reset is required

**Update tests**:
- Review `src/server/legacy/unit_tests.rs` and
`tests/integration/server_legacy.rs`
- Ensure tests that do not authenticate complete or disconnect before
`UNAUTHENTICATED_TIMEOUT` elapses
- Add a test case that verifies unauthenticated connections are closed after
`UNAUTHENTICATED_TIMEOUT`

Research

The server entry point (src/main.rs) delegates to mxd::server::run(), which routes to either the legacy or wireframe runtime based on compile-time features. The legacy accept loop lives in src/server/legacy/mod.rs, using tokio::select! over listener.accept() with a JoinSet for task tracking and a watch channel for shutdown signalling. ServerResources holds shared state (DbPool, Arc<Argon2>, Arc<PresenceRegistry>). The TransactionReader already supports configurable per-frame timeouts via .with_timeout(). Constants such as HANDSHAKE_TIMEOUT reside in src/protocol.rs, though connection-management constants are absent.

The TCP accept loop in src/server/legacy/mod.rs retries errors immediately without backoff or classification, and spawns unbounded tasks without connection limits or unauthenticated idle timeouts. This enables a denial-of-service attack via file-descriptor exhaustion, causing CPU spin and log flooding.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Implement exponential backoff with error classification in the legacy server's
accept loop (`src/server/legacy/mod.rs`). The goal is to prevent CPU spin and
log flooding when file descriptors are exhausted.

**Define constants** at module level alongside existing definitions:
- `ACCEPT_BACKOFF_INITIAL: Duration` (e.g., 5 ms)
- `ACCEPT_BACKOFF_CEILING: Duration` (e.g., 1 second)

**Add a private helper function** `is_resource_exhaustion_error(e: &io::Error)
-> bool`:
- Use `#[cfg(unix)]` conditional compilation
- On Unix, inspect `e.raw_os_error()` and match against `libc::EMFILE` and
`libc::ENFILE`, returning `true` for those values
- Return `false` for all other errors

**Modify `accept_connections`** to integrate backoff:
- Introduce a mutable `backoff_duration` variable initialised to
`ACCEPT_BACKOFF_INITIAL`
- On accept error: if `is_resource_exhaustion_error` returns `true`, sleep for
`backoff_duration`, then double `backoff_duration` up to
`ACCEPT_BACKOFF_CEILING`, then continue the loop
- On accept error: if not resource-exhaustion, log and continue immediately
(existing behaviour)
- On successful accept, reset `backoff_duration` to `ACCEPT_BACKOFF_INITIAL`

**Rate-limit error logging**:
- Track a `last_log_timestamp` for accept errors
- Suppress duplicate log entries if the previous log occurred within a short
window (e.g., 1 second)
- Log a count of suppressed errors when the window expires
===============================================================================

Task: 2

Introduce a connection-limiting semaphore into `src/server/legacy/mod.rs` to cap
concurrent connections and prevent unbounded task spawning. The goal is to bound
file-descriptor usage under load.

**Add a constant** at module level:
- `MAX_CONNECTIONS: usize` (e.g., 512)

**Extend `ServerResources`**:
- Add field `connection_semaphore: Arc<Semaphore>`
- Initialise it with `MAX_CONNECTIONS` permits in `accept_connections`

**Modify `accept_connections`**:
- After a successful `listener.accept()`, call
`connection_semaphore.clone().acquire_owned().await` to obtain an
`OwnedSemaphorePermit`
- Pass the acquired permit into `spawn_client_handler` (update the call site
accordingly)

**Update `spawn_client_handler`**:
- Add `permit: OwnedSemaphorePermit` as a parameter
- Move the permit into the spawned async block; it will be dropped automatically
when the handler completes, releasing the semaphore slot
- No explicit `drop` call is required

**Expose for test support**:
- Under `#[cfg(feature = "test-support")]`, make the `connection_semaphore`
field on `ServerResources` `pub`
- Update the `AcceptContext` fixture in `tests/integration/server_legacy.rs` as
necessary
===============================================================================

Task: 3

Add an unauthenticated idle timeout to `src/server/legacy/mod.rs` to disconnect
clients that fail to authenticate within a deadline. The goal is to release file
descriptors promptly from stalled connections.

**Add a constant** at module level:
- `UNAUTHENTICATED_TIMEOUT: Duration` (e.g., 30 seconds)

**Modify `handle_client`**:
- At the start of the function, compute `auth_deadline = Instant::now() +
UNAUTHENTICATED_TIMEOUT`
- In the main `tokio::select!` loop, add a branch that fires on
`tokio::time::sleep_until(auth_deadline.into())` (or equivalent), guarded by `if
session.user_id.is_none()`
- When this branch fires, log a warning and `break` out of the loop to close the
connection
- The guard condition `session.user_id.is_none()` ensures the branch is skipped
automatically once authentication succeeds — no explicit reset is required

**Update tests**:
- Review `src/server/legacy/unit_tests.rs` and
`tests/integration/server_legacy.rs`
- Ensure tests that do not authenticate complete or disconnect before
`UNAUTHENTICATED_TIMEOUT` elapses
- Add a test case that verifies unauthenticated connections are closed after
`UNAUTHENTICATED_TIMEOUT`

💡 Iterate on the plan with: `@coderabbitai` <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!