Unbounded WatchSandbox sandbox_id #27

Activity

1. pimlock

   added this to the march-mvp milestone on Feb 27, 2026
2. pimlock

   added

   topic:securitySecurity issuesSecurity issues

   on Feb 27, 2026
3. johntmyers commented on Mar 5, 2026

   

   johntmyers

   on Mar 5, 2026

   Collaborator

   More actions

   🔒 security-review-agent

   Security Review

   Determination: Legitimate concern

   Summary

   The WatchSandbox and PushSandboxLogs gRPC handlers create unbounded per-sandbox-id entries in three in-memory broadcast bus HashMaps (TracingLogBus, SandboxWatchBus, PlatformEventBus) without validating that the sandbox exists (or, for WatchSandbox, before the existence check runs). No cleanup/eviction mechanism exists for any of these maps, allowing unbounded memory growth.

   Severity Assessment

   • Impact: Medium — memory exhaustion / denial of service on the navigator-server process
   • Exploitability: Low-to-medium — the server enforces mTLS (tls.rs:42-47), so only clients presenting a valid certificate signed by the server CA can connect. This limits the attack surface to compromised sandboxes or internal cluster components. A compromised sandbox with valid mTLS credentials could trivially exploit this.
   • Affected components:
     • crates/navigator-server/src/grpc.rs — watch_sandbox (line 175): subscribes to all 3 buses at lines 206, 211, 218 before the existence check at line 227, creating entries for non-existent sandbox IDs
     • crates/navigator-server/src/grpc.rs — push_sandbox_logs (line 1137): calls publish_external at line 1159 with unchecked sandbox_id, creating both channel and tail buffer entries
     • crates/navigator-server/src/tracing_bus.rs — TracingLogBus::sender_for (line 62): or_insert_with creates a broadcast::channel(1024) per new ID; publish (line 107) creates a VecDeque tail entry per new ID (capacity 2000 events)
     • crates/navigator-server/src/sandbox_watch.rs — SandboxWatchBus::sender_for (line 38): or_insert_with creates a broadcast::channel(128) per new ID
     • crates/navigator-server/src/tracing_bus.rs — PlatformEventBus::sender_for (line 204): or_insert_with creates a broadcast::channel(1024) per new ID

   Attack Scenario

   1. Attacker compromises a sandbox pod (or obtains valid mTLS client credentials through another vector)
   2. Attacker sends thousands of WatchSandbox requests with randomly generated sandbox IDs — each creates entries in 3 separate HashMaps (3 broadcast channels per fake ID)
   3. Attacker also sends PushSandboxLogs streams with random sandbox IDs, which additionally creates tail buffer VecDeques (each capable of holding up to 2000 SandboxStreamEvent entries)
   4. Over time, the server's memory grows without bound, eventually causing OOM or degraded performance for all users

   Remediation Plan

   1. Validate sandbox existence before subscribing — in watch_sandbox, move the store lookup (get_message) before the bus subscribe() calls. Return not_found early without creating bus entries. Similarly, validate sandbox_id in push_sandbox_logs before calling publish_external.
   2. Add cleanup/eviction for bus entries — implement a mechanism to remove bus entries for sandbox IDs that are deleted or no longer exist. Options include:
      • Hook sandbox deletion to call a remove(sandbox_id) method on all 3 buses
      • Implement a periodic GC sweep that checks entries against the store and removes orphans
      • Use a bounded LRU-style map with TTL eviction
   3. Cap the number of distinct sandbox IDs per bus — add a hard upper bound on the number of entries in each HashMap to prevent unbounded growth even if cleanup has edge-case gaps
   4. Rate-limit subscribe/watch calls — add per-client rate limiting on the WatchSandbox endpoint to limit how quickly new entries can be created

   Additional Notes

   • The mTLS requirement (tls.rs:24-25 comment: "The server always enforces mTLS") significantly reduces exploitability — this is not reachable from the public internet. The primary attack vector is a compromised sandbox or a malicious internal component.
   • The push_sandbox_logs endpoint has a partial mitigation: it caps lines per batch to 100 (line 1153), but this doesn't prevent creating new map entries for arbitrary sandbox IDs.
   • The get_sandbox_logs endpoint (tail() at line 78) is safe — it uses HashMap::get() which doesn't insert.
   • Fix ci: add GitHub Actions CI workflow with lint, test, and image build #1 (validate before subscribe) is the highest-priority remediation as it directly addresses the root cause in watch_sandbox. The current code structure subscribes at lines 206-224 then checks existence at line 227 — reordering these eliminates the issue for non-existent IDs.
4. johntmyers

   added

   state:review-readyReady for human reviewReady for human review

   on Mar 5, 2026
5. johntmyers commented on Mar 5, 2026

   

   johntmyers

   on Mar 5, 2026

   Last edited by johntmyers

   Collaborator

   More actions

   🏗️ build-plan

   Implementation Plan

   Issue type: fix
   Complexity: Medium
   Confidence: High

   Summary

   WatchSandbox subscribes to three in-memory buses (SandboxWatchBus, TracingLogBus, PlatformEventBus) before verifying the sandbox exists, creating unbounded map entries for arbitrary IDs. Additionally, push_sandbox_logs publishes to the tracing bus without validating sandbox existence. None of the three buses have any cleanup/removal mechanism, so entries accumulate indefinitely even for legitimately deleted sandboxes. This fix reorders watch_sandbox to validate before subscribing, adds validation to push_sandbox_logs, and adds remove methods to all three buses wired into the sandbox deletion path.

   Scope

   • crates/navigator-server/src/grpc.rs: Reorder watch_sandbox (lines 202-260) so the store lookup (get_message) happens before the bus subscribe() calls. Add sandbox existence validation to push_sandbox_logs (lines 1137-1164) on the first batch of the stream before calling publish_external.
   • crates/navigator-server/src/tracing_bus.rs: Add remove(sandbox_id) method to TracingLogBus that removes entries from both per_id and tails HashMaps. Add remove(sandbox_id) method to PlatformEventBus that removes entries from its inner HashMap.
   • crates/navigator-server/src/sandbox_watch.rs: Add remove(sandbox_id) method to SandboxWatchBus that removes entries from its inner HashMap.
   • crates/navigator-server/src/sandbox/mod.rs: Wire bus cleanup into handle_deleted (line 362) so all three buses are cleaned up when a sandbox is deleted from Kubernetes.

   Implementation Steps

   1. Add remove method to TracingLogBus — Add pub fn remove(&self, sandbox_id: &str) that removes the entry from both per_id and tails under a single lock acquisition. This drops the broadcast sender (closing any active receivers) and frees the tail buffer.
   2. Add remove method to PlatformEventBus — Add pub(crate) fn remove(&self, sandbox_id: &str) that removes the entry from the inner HashMap.
   3. Add remove method to SandboxWatchBus — Add pub fn remove(&self, sandbox_id: &str) that removes the entry from the inner HashMap.
   4. Wire cleanup into handle_deleted — In crates/navigator-server/src/sandbox/mod.rs, pass TracingLogBus to handle_deleted and call remove on all three buses (tracing_log_bus, tracing_log_bus.platform_event_bus, sandbox_watch_bus) after deletion. Update the calling signature accordingly.
   5. Wire cleanup into delete_sandbox gRPC handler — In grpc.rs, after successful Kubernetes deletion, call remove on all three buses to clean up entries for the deleted sandbox ID.
   6. Reorder watch_sandbox to validate before subscribing — Move the state.store.get_message::<Sandbox>(&sandbox_id) lookup above the bus subscription calls. If the sandbox does not exist, return not_found immediately without creating any bus entries. Restructure the spawned task so the snapshot read happens first, then subscriptions happen only for existing sandboxes. Note: care must be taken to avoid a race where a notification fires between the snapshot read and the subscribe—the existing code's comment (line 203) explains this concern. The fix should read the snapshot, subscribe, then re-read if needed (or accept the minor race for non-existent IDs, which is the actual fix target).
   7. Add sandbox validation to push_sandbox_logs — Validate the sandbox_id once on the first batch of the client-streaming RPC, not per-batch. push_sandbox_logs is a long-lived stream where sandbox_id is constant across all batches (the sandbox client sets it at connection time, flushing every 500ms or 50 lines). A single store check at stream open is sufficient. If the sandbox is deleted mid-stream, the bus remove() from step 4/5 drops the broadcast sender, causing subsequent publish_external calls to silently discard via the existing let _ = tx.send(...) pattern — no cache or eviction needed.
   8. Add unit tests for bus remove methods — Add #[cfg(test)] mod tests in tracing_bus.rs and sandbox_watch.rs to verify: (a) remove cleans up entries from all internal maps, (b) subscribe after remove creates a fresh channel, (c) tail returns empty after remove.
   9. Add unit test for watch_sandbox validation ordering — Add a test (or extend existing integration tests) that confirms watch_sandbox with a non-existent sandbox ID returns not_found and does not leave entries in any bus.

   Test Plan

   • Unit tests: tracing_bus.rs — test TracingLogBus::remove and PlatformEventBus::remove clean up internal maps. sandbox_watch.rs — test SandboxWatchBus::remove cleans up internal map. Test that publish after remove re-creates entries correctly.
   • Integration tests: Extend existing integration test patterns in crates/navigator-server/tests/ to verify watch_sandbox with invalid sandbox ID returns not_found without leaking bus entries (may require a test helper to inspect bus size).
   • E2E tests: N/A — mTLS requirement makes this difficult to test end-to-end; the unit and integration tests provide sufficient coverage.

   Risks & Open Questions

   • Race condition in watch_sandbox reordering: The existing code intentionally subscribes before the snapshot read to avoid missing notifications (comment on line 203). Reordering to validate-first introduces a small window where a sandbox could be created between the validation check and the subscribe. Mitigation: perform a snapshot read to validate, then subscribe, then re-read the snapshot for the actual data. This preserves correctness for existing sandboxes while preventing entries for non-existent ones.
   • Broadcast receiver drop behavior: When remove drops the broadcast Sender, existing subscribers will receive a RecvError::Closed. The existing broadcast_to_status helper already translates this to Status::cancelled, so active streams will terminate gracefully. For push_sandbox_logs, the let _ = tx.send(...) pattern silently discards, so stale streams naturally drain without error.

   Documentation Impact

   • None expected — these are internal implementation changes. The gRPC API behavior is unchanged for valid sandbox IDs.

   ---

   Revision 2 — Updated step 7: validate sandbox_id once at stream open instead of per-batch; removed per-batch store lookup latency concern from risks since it no longer applies
   Revision 1 — initial plan
6. drew

   assigned

   johntmyers

   on Mar 5, 2026
7. johntmyers commented on Mar 5, 2026

   

   johntmyers

   on Mar 5, 2026

   Collaborator

   More actions

   "store lookup adds latency" - any risk of this growing too much? can we evict and have to occasionally re-hit the store in the event we don't see logs for a while?
8. johntmyers commented on Mar 5, 2026

   

   johntmyers

   on Mar 5, 2026

   Collaborator

   More actions

   🏗️ build-from-issue-agent

   "store lookup adds latency" - any risk of this growing too much? can we evict and have to occasionally re-hit the store in the event we don't see logs for a while?

   Good call — the plan as written implied a store lookup per batch, which is unnecessarily expensive. Looking at the actual push_sandbox_logs implementation (grpc.rs:1137-1164), it's a long-lived client-streaming RPC where sandbox_id is constant across every batch in the stream (the sandbox sets it once at connection time). The batches flush every 500ms or 50 lines (log_push.rs:160,180), so per-batch validation would be a lot of wasted work.

   The better approach: validate sandbox_id once at the start of the stream (on the first batch received), then trust it for the remainder of that stream. If the sandbox gets deleted mid-stream, the bus remove() cleanup will drop the broadcast sender, which causes the publish to silently no-op (the let _ = tx.send(...) pattern at tracing_bus.rs:109 already discards send errors). So there's a natural self-healing path — once the sandbox is deleted and the bus entry is cleaned up, subsequent publishes on the stale stream just discard into the void until the stream ends.

   This means we don't need a cache or eviction strategy for push_sandbox_logs at all — a single store check at stream open is sufficient. I'll update the plan to reflect this.
9. johntmyers

   added

   state:agent-readyApproved for agent implementationApproved for agent implementation

   state:in-progressWork is currently in progressWork is currently in progress

   on Mar 5, 2026
10. johntmyers

    added a commit that references this issue on Mar 5, 2026

    fix(server): prevent unbounded bus entry growth for sandbox IDs

    ...

    This issue will close once commit 119c3c6 is merged into the 'main' branch.

    119c3c6
11. johntmyers commented on Mar 5, 2026

    

    johntmyers

    on Mar 5, 2026

    Collaborator

    More actions

    🏗️ build-from-issue-agent

    Implementation Complete

    PR: #138

    What was built

    Added remove() methods to all three in-memory buses (TracingLogBus, PlatformEventBus, SandboxWatchBus) and wired cleanup into sandbox deletion paths. Reordered WatchSandbox to validate before subscribing, and added sandbox existence validation to PushSandboxLogs at stream open.

    Tests

    • Unit: 10 tests added (7 for tracing_bus, 3 for sandbox_watch)

    Docs updated

    • architecture/gateway.md — added cleanup and validation notes

    The issue will auto-close when the PR is merged.
12. johntmyers

    added

    state:pr-openedPR has been opened for this issuePR has been opened for this issue

    and removed

    state:in-progressWork is currently in progressWork is currently in progress

    state:review-readyReady for human reviewReady for human review

    on Mar 5, 2026
13. johntmyers

    closed this as completedin #138on Mar 5, 2026
14. kirit93

    added a commit that references this issue on Mar 9, 2026

    fix(server): prevent unbounded bus entry growth for sandbox IDs (#138)

    ...

    This issue will close once commit e8e81a3 is merged into the 'main' branch.

    e8e81a3
15. drew

    added a commit that references this issue on Mar 16, 2026

    fix(server): prevent unbounded bus entry growth for sandbox IDs (#138)

    ...

    This issue will close once commit ae9e766 is merged into the 'main' branch.

    ae9e766
16. factory-octavian

    added a commit that references this issue on Mar 31, 2026

    chore: add cloudpickle to sandbox base image (NVIDIA#27)

    5c8b189
17. maxamillion

    added a commit that references this issue on Apr 9, 2026

    Merge pull request NVIDIA#27 from LobsterTrap/fix/podman-socket-error…

    ...

    5c543af