Repository navigation
No policy validation to prevent corrupted or malicious policies #33
Description
Sandbox policies are loaded from the server via GetSandboxPolicy or from local files. The policy drives filesystem allowlists (Landlock), network allowlists (OPA), and process identity (run_as_user, run_as_group). There is no validation that the policy content is safe or well-formed beyond basic proto/type conversion. Corrupted or maliciously crafted policy could therefore cause security issues such as
- Privilege escalation
- Overly permissive network policy
- Undefined behaviour
Source code:
- The server returns policy as-is in crates/navigator-server/src/grpc.rs in get_sandbox_policy (lines 513-540): it fetches the sandbox, extracts spec.policy, and returns it. There is no validation of the policy fields (e.g. no check that run_as_user is not root, no validation of network_policies or filesystem paths).
- In crates/navigator-sandbox/src/policy.rs, SandboxPolicy::try_from(proto) (lines 95-121) and the From impls for FilesystemPolicy, ProcessPolicy, etc. (lines 123-158) convert proto to Rust types but do not validate safety (e.g. no rejection of root, no path normalization or constraints).
- File-based loading in crates/navigator-sandbox/src/lib.rs (lines 283-311) builds policy from OPA engine config or from gRPC; the YAML/rego file content is not validated for malicious or unsafe policy content before use.
- Filesystem paths from proto are used directly in policy.rs (e.g. lines 125-127): proto.read_only.into_iter().map(PathBuf::from).collect() with no check for path traversal or sensitive directories.
Originally by @igurel on 2026-02-20T06:06:48.189-08:00
Activity
johntmyers commentedon Mar 5, 2026
Security Review
Determination: Legitimate concern
Summary
The sandbox policy pipeline lacks semantic validation at creation time: filesystem paths are not normalized or constrained,
run_as_useraccepts arbitrary values (includingroot), and there are no bounds checks on policy fields. While existing mitigations (Landlock, container namespaces, immutable-after-creation enforcement) limit exploitability, this represents a defense-in-depth gap that could be exploited by a compromised or misconfigured policy source.Severity Assessment
read_write: ["/"]) or run as root inside the sandbox. Impact is bounded by container isolation and namespace restrictions.crates/navigator-sandbox/src/policy.rs—TryFrom<ProtoSandboxPolicy>andFromimpls perform type conversion with no semantic validationcrates/navigator-server/src/grpc.rs—create_sandbox(line 72) andget_sandbox_policy(line 557) accept/return policy without validating content safetycrates/navigator-sandbox/src/sandbox/linux/landlock.rs—apply()uses paths verbatim from policycrates/navigator-policy/src/lib.rs—parse_sandbox_policy()deserializes YAML with no safety constraintsAttack Scenario
run_as_user: "root"andread_write: ["/"], or uses path traversal patterns in filesystem pathsRemediation Plan
create_sandboxanddiscover_policy_from_disk_or_default):run_as_user: "root"(or UID 0)canonicalize()or reject relative/traversal paths (..), reject overly broad paths like/FilesystemPolicy::from(ProtoFilesystemPolicy)to canonicalize paths before they reach Landlockrun_as_user/run_as_group— only permit known sandbox-safe usersAdditional Notes
validate_static_fields_unchangedandvalidate_network_mode_unchangedfunctions show good precedent for server-side validation — the gap is that initial creation has no equivalent safety checks.restrictive_default_policy()is well-designed (sandbox user, limited paths, network blocked). The risk is primarily for custom/operator-supplied policies that bypass these defaults.johntmyers commentedon Mar 5, 2026
Implementation Plan
Issue type:
featComplexity: Medium
Confidence: High
Summary
Add a policy validation layer to
navigator-policythat rejects or warns on unsafe policy content — specifically root process identity, path traversal in filesystem paths, overly broad paths (e.g./), and unreasonable field sizes. Invoke this validation from all policy entry points:parse_sandbox_policy,load_sandbox_policy,create_sandbox,update_sandbox_policy, anddiscover_policy_from_disk_or_default.Scope
crates/navigator-policy/src/lib.rs: Add avalidate_sandbox_policy(policy: &SandboxPolicy) -> Result<Vec<PolicyWarning>>function that checks process identity (reject UID 0 / "root"), filesystem path safety (reject relative paths,..components, overly broad paths like/), and field size limits (max path count, max string length). Call it fromparse_sandbox_policyandload_sandbox_policy.crates/navigator-sandbox/src/policy.rs: Add validation in theTryFrom<ProtoSandboxPolicy>impl — normalize filesystem paths and reject unsafe values during conversion from proto to Rust types.crates/navigator-server/src/grpc.rs: Add avalidate_policy_safetycall increate_sandbox(before persisting) and inupdate_sandbox_policy(before storing the new revision), returningStatus::invalid_argumentfor hard violations. This mirrors the existingvalidate_static_fields_unchanged/validate_network_mode_unchangedpattern.crates/navigator-sandbox/src/lib.rs: Add a validation call indiscover_policy_from_disk_or_defaultto log warnings (and fall back to restrictive default) if the disk-loaded policy contains unsafe content.e2e/python/test_policy_validation.py: Add e2e tests that validate server-side rejection of unsafe policies via the gRPC API.Implementation Steps
navigator-policy: Add aPolicyViolationenum (with variants likeRootProcessIdentity,PathTraversal,OverlyBroadPath,RelativePath,FieldTooLong) and avalidate_sandbox_policy(&SandboxPolicy) -> Result<(), Vec<PolicyViolation>>function that checks: (a)run_as_user/run_as_groupis not "root" or "0", (b) filesystem paths are absolute, contain no..components, and are not overly broad (/), (c) path count and string length limits.parse_sandbox_policyandload_sandbox_policy: After parsing YAML into proto, callvalidate_sandbox_policyand return an error if hard violations are found.navigator-sandboxpolicy conversion: InFilesystemPolicy::from(ProtoFilesystemPolicy), normalize paths (collapse redundant separators, reject..components) before storing them. This hardens the Landlock apply path.create_sandbox: Before persisting the sandbox, ifspec.policyisSome, callvalidate_sandbox_policyand returnStatus::invalid_argumenton violation. This is the primary defense for the gRPC API surface.update_sandbox_policy: After existingvalidate_static_fields_unchanged/validate_network_mode_unchangedchecks, callvalidate_sandbox_policyon the new policy.discover_policy_from_disk_or_default: Afterparse_sandbox_policysucceeds, validate the result. Log warnings and fall back to restrictive default on hard violations.navigator-policy: Test that root user is rejected, path traversal is rejected, relative paths are rejected, overly broad paths are rejected, valid policies pass, and the restrictive default policy passes validation.navigator-server: Test thatcreate_sandboxandupdate_sandbox_policyreject policies with unsafe content (root user, bad paths).e2e/python/test_policy_validation.py: Using the existing e2e fixtures (sandbox,sandbox_client) fromconftest.py, add tests that exercise the server-side validation end-to-end:test_create_sandbox_rejects_root_user— create a sandbox withrun_as_user: "root"and assert gRPCINVALID_ARGUMENT.test_create_sandbox_rejects_path_traversal— use..in filesystem paths and assert rejection.test_create_sandbox_rejects_overly_broad_paths— useread_write: ["/"]and assert rejection.test_update_policy_rejects_unsafe_content— create a sandbox with a valid policy, then callUpdateSandboxPolicywith unsafe content and assertINVALID_ARGUMENT.Test Plan
crates/navigator-policy/src/lib.rs— tests for thevalidate_sandbox_policyfunction covering all violation types and edge cases.crates/navigator-server/src/grpc.rs— tests for server-side rejection of unsafe policies (following the existingvalidate_static_fields_*test pattern).e2e/python/test_policy_validation.py— tests that exercise the full gRPC path forcreate_sandboxandupdate_sandbox_policyrejection of unsafe policies. These follow the existing e2e patterns intest_sandbox_policy.pyand use thesandbox/sandbox_clientconftest fixtures.Risks & Open Questions
run_as_user: rootor broad paths like/will start failing. Consider whether to make root rejection a hard error or a logged warning. The plan defaults to hard error forcreate_sandboxand logged warning + fallback for disk-loaded policies.canonicalize()requires the path to exist on the server, which it won't (paths are for the sandbox filesystem). Use lexical normalization (collapse//, reject..) instead./as read-write (read-only/may be acceptable in some configurations). Open to feedback.Documentation Impact
Revision 2 — added e2e tests (step 9) for server-side policy validation per feedback; updated scope, test plan
Revision 1 — initial plan
johntmyers commentedon Mar 5, 2026
We could add a couple of e2e tests to validate server-side changes and rejection of policy updates that are violating.
johntmyers commentedon Mar 5, 2026
Agreed. The existing e2e test infrastructure in
e2e/python/test_sandbox_policy.pyalready has comprehensive tests for policy behavior (L4, L7, SSRF, live policy updates), and theconftest.pyfixtures providesandboxandsandbox_clienthelpers that make this straightforward.The
test_live_policy_update_and_logstest already callsUpdateSandboxPolicyvia the gRPC stub, so we have a clear pattern for testing server-side rejection. Specifically, I'll add:test_create_sandbox_rejects_root_user— Callssandbox()withrun_as_user: "root"in the policy and asserts the gRPC error isINVALID_ARGUMENT.test_create_sandbox_rejects_path_traversal— Uses..components in filesystem paths and asserts rejection.test_update_policy_rejects_unsafe_content— Creates a sandbox with a valid policy, then callsUpdateSandboxPolicywith a policy containingrun_as_user: "root"or overly broad paths, and assertsINVALID_ARGUMENT.test_create_sandbox_rejects_overly_broad_paths— Usesread_write: ["/"]and asserts rejection.These will live in a new test file
e2e/python/test_policy_validation.pyto keep them separate from the existing network/proxy policy tests. I'll update the plan to reflect this.feat(policy): add validation layer to reject unsafe sandbox policies
johntmyers commentedon Mar 5, 2026
Implementation PR opened: #135
Changes:
navigator-policy: AddedPolicyViolationenum,validate_sandbox_policy(),normalize_path(), constants for limits, and 13 unit testsnavigator-sandbox: Path normalization inFilesystemPolicy::from(), validation indiscover_policy_from_path()with fallback to restrictive default, 1 unit testnavigator-server:validate_policy_safety()helper called increate_sandboxandupdate_sandbox_policy(returnsINVALID_ARGUMENT), 4 unit testse2e/python/test_policy_validation.py: 4 e2e tests covering root user rejection, path traversal rejection, overly broad paths, and update policy rejectionAll local pre-commit checks pass (rust:check, rust:lint, rust:format:check, test:rust, python:format:check, python:proto, helm:lint).
feat(policy): add validation layer to reject unsafe sandbox policies (#…
feat(policy): add validation layer to reject unsafe sandbox policies (#…
feat: openshell improvements (NVIDIA#33)
Merge pull request NVIDIA#33 from opendatahub-io/disable-telemetry