Unbounded list limit #26

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 list RPCs (ListSandboxes, ListProviders, ListInferenceRoutes, ListPolicyRevisions) accept a client-provided limit parameter with no upper bound. When limit is non-zero, the raw value is passed directly to SQL LIMIT clauses, allowing a client to request up to 4,294,967,295 rows in a single call, potentially causing memory exhaustion and denial of service.

   Severity Assessment

   • Impact: Medium — an attacker can cause server memory pressure and potential OOM, degrading or crashing the service for all users.
   • Exploitability: Trivial — requires only a single gRPC call with limit set to a large value. No authentication bypass is needed; any client with gRPC access can trigger this. There is no rate limiting or auth middleware on these endpoints.
   • Affected components:
     • crates/navigator-server/src/grpc.rs — list_sandboxes (line 405), list_providers (line 519), list_policies (line 995), list_provider_records (line 1830)
     • crates/navigator-server/src/inference.rs — list_inference_routes (line 168)
     • crates/navigator-server/src/persistence/sqlite.rs — list() (line 153)
     • crates/navigator-server/src/persistence/postgres.rs — list() (line 130)

   Attack Scenario

   1. Attacker sends a gRPC ListSandboxes (or any list RPC) request with limit = 4294967295.
   2. The server passes this value directly as the SQL LIMIT, causing the database to attempt to return all matching rows.
   3. The server loads all results into memory (fetch_all), serializes them into a gRPC response, and attempts to send the massive payload.
   4. This causes excessive memory allocation on the server, potentially leading to OOM kills and service disruption for all clients.

   Remediation Plan

   1. Define a server-side maximum limit constant (e.g., const MAX_PAGE_SIZE: u32 = 1000;) and clamp the client-provided limit using limit.min(MAX_PAGE_SIZE) in all list handlers.
   2. Apply this cap consistently across all four affected list RPCs and the list_provider_records helper function.
   3. Consider adding the maximum allowed page size to the API documentation / proto comments so clients are aware of the constraint.

   Additional Notes

   • The Vec::with_capacity(records.len()) calls are safe since they use the actual result count, not the raw limit — the primary concern is the unbounded SQL query and response size.
   • While the actual number of records in the database may be small today, this is a defense-in-depth issue: as the system grows or if the store is shared, the risk increases.
   • There is currently no rate limiting or authentication middleware on these gRPC endpoints, which makes exploitation even more straightforward.
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

   Collaborator

   More actions

   🏗️ build-plan

   Implementation Plan

   Issue type: fix
   Complexity: Low
   Confidence: High

   Summary

   All list RPCs pass the client-provided limit directly to SQL queries with no upper bound, allowing a single request to load an unbounded number of rows and cause memory exhaustion. The fix introduces a shared MAX_PAGE_SIZE constant and clamps the limit in every list handler before it reaches the persistence layer.

   Scope

   • crates/navigator-server/src/grpc.rs: Clamp limit to MAX_PAGE_SIZE in list_sandboxes (line 405), list_providers (line 519), list_sandbox_policies (line 995), and list_provider_records (line 1830). Define the MAX_PAGE_SIZE constant and a helper function clamp_limit(raw: u32, default: u32) -> u32 at module level.
   • crates/navigator-server/src/inference.rs: Clamp limit to MAX_PAGE_SIZE in list_inference_routes (line 168). Import the constant/helper from grpc or a shared location.

   Implementation Steps

   1. Add a pub(crate) const MAX_PAGE_SIZE: u32 = 1000; and a pub(crate) fn clamp_limit(raw: u32, default: u32, max: u32) -> u32 helper in crates/navigator-server/src/grpc.rs (or a shared module). The helper returns default when raw == 0, otherwise raw.min(max).
   2. Replace the inline if request.limit == 0 { 100 } else { request.limit } pattern in list_sandboxes (line 405-408) with clamp_limit(request.limit, 100, MAX_PAGE_SIZE).
   3. Replace the same pattern in list_providers (line 519-523) with clamp_limit(request.limit, 100, MAX_PAGE_SIZE).
   4. Replace the same pattern in list_sandbox_policies (line 995) with clamp_limit(req.limit, 50, MAX_PAGE_SIZE) (preserving the existing default of 50).
   5. Replace the same pattern in list_inference_routes in inference.rs (line 168-171) with clamp_limit(request.limit, 100, MAX_PAGE_SIZE), importing the constant and helper.
   6. Add unit tests for clamp_limit: verify 0 returns default, a value within range passes through, and a value exceeding MAX_PAGE_SIZE is capped.
   7. Run mise run pre-commit to verify everything compiles and passes.

   Test Plan

   • Unit tests: Add tests for clamp_limit in the existing mod tests block in crates/navigator-server/src/grpc.rs — cover zero input (returns default), normal input (pass-through), and over-limit input (clamped to max).
   • Integration tests: The existing sqlite_list_paging test in crates/navigator-server/src/persistence/tests.rs already validates list paging at the store level; no new integration tests needed since the clamp is applied at the handler layer above the store.
   • E2E tests: N/A

   Risks & Open Questions

   • The MAX_PAGE_SIZE of 1000 is a reasonable default but could be made configurable via server config in the future. For this fix, a compile-time constant is sufficient.
   • No proto/API changes are needed — the server silently clamps the limit. Clients requesting more than 1000 will simply receive at most 1000 results per page. This is standard pagination behavior and not a breaking change.

   Documentation Impact

   • None expected. The proto comments for limit fields could optionally be updated to document the server-side maximum, but that is a follow-up concern.

   ---

   Revision 1 — initial plan
6. drew

   assigned

   johntmyers

   on Mar 5, 2026
7. johntmyers

   added

   state:agent-readyApproved for agent implementationApproved for agent implementation

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

   on Mar 6, 2026
8. johntmyers

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

   fix(server): clamp list RPC page limit to prevent unbounded queries

   ...

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

   3f4b06c
9. johntmyers commented on Mar 6, 2026

   

   johntmyers

   on Mar 6, 2026

   Collaborator

   More actions

   🏗️ build-from-issue-agent

   Implementation Complete

   PR: #140

   What was built

   Added a MAX_PAGE_SIZE constant (1000) and clamp_limit helper function to cap client-provided limit values in all list RPCs (ListSandboxes, ListProviders, ListSandboxPolicies, ListInferenceRoutes), preventing unbounded SQL queries and potential memory exhaustion.

   Tests

   • Unit: 3 tests added (clamp_limit_zero_returns_default, clamp_limit_within_range_passes_through, clamp_limit_exceeding_max_is_capped)

   Docs updated

   • None needed

   The issue will auto-close when the PR is merged.
10. 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 6, 2026
11. johntmyers

    closed this as completedin #140on Mar 6, 2026
12. johntmyers

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

    fix(server): clamp list RPC page limit to prevent unbounded queries (#…

    ...

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

    6bc4eb2
13. drew

    added 3 commits that reference this issue on Mar 16, 2026

    refactor(sandbox): consolidate policy data into YAML, remove rego dat…

    ...

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

    6cf0264

    fix(server): clamp list RPC page limit to prevent unbounded queries (#…

    ...

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

    5dd823a

    refactor(sandbox): consolidate policy data into YAML, remove rego dat…

    ...

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

    42eb16d
14. factory-octavian

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

    Revert "fix: restore cluster_pods network policy for nemoclaw and ope…

    ...

    764f9c9
15. maxamillion

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

    Merge pull request NVIDIA#26 from maxamillion/fix/flannel-nftables-en…

    ...

    73909fb
16. alexclewontin

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

    Merge pull request NVIDIA#26 from markatcanonical/doc/update-snap-readme

    ...

    fe73720