fix(sandbox): restrict /sandbox to read-only via Landlock (#804)#1121
Open
prekshivyas wants to merge 4 commits intoNVIDIA:mainfrom
Open
fix(sandbox): restrict /sandbox to read-only via Landlock (#804)#1121prekshivyas wants to merge 4 commits intoNVIDIA:mainfrom
prekshivyas wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
…stem policy (NVIDIA#804) Tighten the Landlock filesystem policy so agents cannot write arbitrary files in the /sandbox home directory. Only explicitly declared paths remain writable (/sandbox/.openclaw-data, /sandbox/.nemoclaw, /tmp). - Set include_workdir to false (verified against OpenShell landlock.rs: when true, WORKDIR is added to read_write, overriding read_only) - Move /sandbox from read_write to read_only in the policy - Add /sandbox/.nemoclaw to read_write for plugin state/config writes - DAC-protect blueprints with root ownership (defense-in-depth) - Pre-create .bashrc/.profile at build time (read-only home prevents runtime writes); source proxy config from writable proxy-env.sh - Redirect tool dotfiles (npm, git, pip, bash, claude, node) to /tmp via env vars in both the entrypoint and the sourced proxy-env.sh so interactive connect sessions also get the redirects Closes NVIDIA#804
Contributor
📝 WalkthroughWalkthroughRead-only home-directory hardening: image build-time creates/sanitizes shell init and blueprint paths; runtime entrypoint writes proxy and cache redirects to Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
prekshivyas
changed the title
…proach The entrypoint no longer writes proxy config directly to ~/.bashrc (read-only home). Tests now verify that proxy-env.sh is written to the writable data dir and that .bashrc sourcing works correctly.
The sed-extracted block contains the path in comments before the variable assignment. replace() only swaps the first occurrence (the comment), leaving the actual _PROXY_ENV_FILE assignment pointing at /sandbox/.openclaw-data/ which doesn't exist in CI.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/deployment/sandbox-hardening.md (2)
85-86: Keep each sentence on its own source line in this intro.The first sentence is split across two source lines, and the second shares the same line as the end of the first. Please give each sentence its own line. As per coding guidelines, "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/sandbox-hardening.md` around lines 85 - 86, The intro currently has two sentences on the same source line; split them so each sentence is on its own line: ensure "The sandbox Landlock policy restricts `/sandbox` (the agent's home directory) to read-only access." is one line and "Only explicitly declared directories are writable:" is the following line, updating the text in the same paragraph (no other changes).
103-105: Rewrite this in active voice and keep one sentence per line.
are pre-createdis passive, and the sentence is wrapped across multiple source lines. As per coding guidelines, "Active voice required. Flag passive constructions." and "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/sandbox-hardening.md` around lines 103 - 105, Rewrite the two-line passive sentence into active voice and ensure each sentence sits on its own source line: change "Shell init files (`.bashrc`, `.profile`) are pre-created at image build time and source runtime proxy configuration from the writable `/sandbox/.openclaw-data/proxy-env.sh`." into two active-voice sentences such as "The image build process pre-creates shell init files `.bashrc` and `.profile`." and "These files source runtime proxy configuration from `/sandbox/.openclaw-data/proxy-env.sh`." Place each sentence on its own line in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 151-159: The Dockerfile currently only makes
/sandbox/.nemoclaw/blueprints root-owned; instead ensure the parent is locked:
in the RUN that touches /sandbox/.nemoclaw, set ownership and permissions on the
parent directory (chown root:root /sandbox/.nemoclaw && chmod 755
/sandbox/.nemoclaw) before adjusting the blueprints subtree, then create the
runtime dirs (/sandbox/.nemoclaw/state and /sandbox/.nemoclaw/migration) and
chown those to sandbox:sandbox so only those are writable; update the existing
RUN that uses chown/chmod/mkdir to apply root ownership and 755 permissions to
/sandbox/.nemoclaw itself (and keep /sandbox/.nemoclaw/blueprints root:root) and
then chown only the state and migration dirs to sandbox.
In `@scripts/nemoclaw-start.sh`:
- Around line 248-273: The proxy env file is written into a sandbox-writable
directory (_PROXY_ENV_FILE="/sandbox/.openclaw-data/proxy-env.sh") which allows
a sandbox user to replace it with malicious shell code; instead write the proxy
env to a non-user-writable, root-owned location (for example create and use a
system-owned directory like /etc/openclaw or /var/lib/openclaw and set
ownership/mode) and update whatever startup/profile sourcing to point at that
path; ensure the write is done atomically and safely (create a temporary file in
the root-owned dir, set owner to root, chmod 0644, then rename into place) and
avoid following attacker symlinks (use safe file creation APIs or the install
command rather than plain cat > "$_PROXY_ENV_FILE"); also remove or stop
auto-sourcing any file from the sandbox-writable tree so agent-controlled files
cannot be executed at session startup.
---
Nitpick comments:
In `@docs/deployment/sandbox-hardening.md`:
- Around line 85-86: The intro currently has two sentences on the same source
line; split them so each sentence is on its own line: ensure "The sandbox
Landlock policy restricts `/sandbox` (the agent's home directory) to read-only
access." is one line and "Only explicitly declared directories are writable:" is
the following line, updating the text in the same paragraph (no other changes).
- Around line 103-105: Rewrite the two-line passive sentence into active voice
and ensure each sentence sits on its own source line: change "Shell init files
(`.bashrc`, `.profile`) are pre-created at image build time and source runtime
proxy configuration from the writable `/sandbox/.openclaw-data/proxy-env.sh`."
into two active-voice sentences such as "The image build process pre-creates
shell init files `.bashrc` and `.profile`." and "These files source runtime
proxy configuration from `/sandbox/.openclaw-data/proxy-env.sh`." Place each
sentence on its own line in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c35344c-d7f9-4676-92dd-982615431c01
📒 Files selected for processing (5)
DockerfileDockerfile.basedocs/deployment/sandbox-hardening.mdnemoclaw-blueprint/policies/openclaw-sandbox.yamlscripts/nemoclaw-start.sh
Comment on lines
+151
to
+159
| # DAC-protect blueprints: /sandbox/.nemoclaw is Landlock read_write (for plugin | ||
| # state/config), but blueprints are immutable at runtime. Root ownership prevents | ||
| # the agent from modifying them even though the directory is writable. The state/ | ||
| # subdirectory stays sandbox-owned for runtime writes. | ||
| # Ref: https://github.com/NVIDIA/NemoClaw/issues/804 | ||
| RUN chown -R root:root /sandbox/.nemoclaw/blueprints \ | ||
| && chmod -R 755 /sandbox/.nemoclaw/blueprints \ | ||
| && mkdir -p /sandbox/.nemoclaw/state /sandbox/.nemoclaw/migration \ | ||
| && chown sandbox:sandbox /sandbox/.nemoclaw/state /sandbox/.nemoclaw/migration |
Contributor
There was a problem hiding this comment.
Lock the .nemoclaw parent, not just blueprints.
This still leaves /sandbox/.nemoclaw sandbox-owned/writable from the base image while Landlock grants write access to the whole subtree. That means the agent can rename or replace the root-owned blueprints directory from its writable parent, so the DAC protection claimed here is bypassable. Make the parent immutable too, then reopen only the runtime subdirectories that actually need writes.
🔒 Proposed fix
-RUN chown -R root:root /sandbox/.nemoclaw/blueprints \
- && chmod -R 755 /sandbox/.nemoclaw/blueprints \
- && mkdir -p /sandbox/.nemoclaw/state /sandbox/.nemoclaw/migration \
- && chown sandbox:sandbox /sandbox/.nemoclaw/state /sandbox/.nemoclaw/migration
+RUN chown root:root /sandbox/.nemoclaw \
+ && chmod 755 /sandbox/.nemoclaw \
+ && chown -R root:root /sandbox/.nemoclaw/blueprints \
+ && chmod -R 755 /sandbox/.nemoclaw/blueprints \
+ && mkdir -p /sandbox/.nemoclaw/state /sandbox/.nemoclaw/migration \
+ && chown sandbox:sandbox /sandbox/.nemoclaw/state /sandbox/.nemoclaw/migration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 151 - 159, The Dockerfile currently only makes
/sandbox/.nemoclaw/blueprints root-owned; instead ensure the parent is locked:
in the RUN that touches /sandbox/.nemoclaw, set ownership and permissions on the
parent directory (chown root:root /sandbox/.nemoclaw && chmod 755
/sandbox/.nemoclaw) before adjusting the blueprints subtree, then create the
runtime dirs (/sandbox/.nemoclaw/state and /sandbox/.nemoclaw/migration) and
chown those to sandbox:sandbox so only those are writable; update the existing
RUN that uses chown/chmod/mkdir to apply root ownership and 755 permissions to
/sandbox/.nemoclaw itself (and keep /sandbox/.nemoclaw/blueprints root:root) and
then chown only the state and migration dirs to sandbox.
Comment on lines
+248
to
+273
| # The /sandbox home directory is Landlock read-only (#804), so we write the proxy | ||
| # config to /sandbox/.openclaw-data/proxy-env.sh (writable). The pre-built | ||
| # .bashrc and .profile source this file automatically. | ||
| # | ||
| # Both uppercase and lowercase variants are required: Node.js undici prefers | ||
| # lowercase (no_proxy) over uppercase (NO_PROXY) when both are set. | ||
| # curl/wget use uppercase. gRPC C-core uses lowercase. | ||
| # | ||
| # Also write to ~/.profile for login-shell paths (e.g. `sandbox create -- cmd` | ||
| # which spawns `bash -lc`). | ||
| # | ||
| # Idempotency: begin/end markers delimit the block so it can be replaced | ||
| # on restart if NEMOCLAW_PROXY_HOST/PORT change, without duplicating. | ||
| _PROXY_MARKER_BEGIN="# nemoclaw-proxy-config begin" | ||
| _PROXY_MARKER_END="# nemoclaw-proxy-config end" | ||
| _PROXY_SNIPPET="${_PROXY_MARKER_BEGIN} | ||
| export HTTP_PROXY=\"$_PROXY_URL\" | ||
| export HTTPS_PROXY=\"$_PROXY_URL\" | ||
| export NO_PROXY=\"$_NO_PROXY_VAL\" | ||
| export http_proxy=\"$_PROXY_URL\" | ||
| export https_proxy=\"$_PROXY_URL\" | ||
| export no_proxy=\"$_NO_PROXY_VAL\" | ||
| ${_PROXY_MARKER_END}" | ||
|
|
||
| if [ "$(id -u)" -eq 0 ]; then | ||
| _SANDBOX_HOME=$(getent passwd sandbox 2>/dev/null | cut -d: -f6) | ||
| _SANDBOX_HOME="${_SANDBOX_HOME:-/sandbox}" | ||
| else | ||
| _SANDBOX_HOME="${HOME:-/sandbox}" | ||
| fi | ||
|
|
||
| _write_proxy_snippet() { | ||
| local target="$1" | ||
| if [ -f "$target" ] && grep -qF "$_PROXY_MARKER_BEGIN" "$target" 2>/dev/null; then | ||
| local tmp | ||
| tmp="$(mktemp)" | ||
| awk -v b="$_PROXY_MARKER_BEGIN" -v e="$_PROXY_MARKER_END" \ | ||
| '$0==b{s=1;next} $0==e{s=0;next} !s' "$target" >"$tmp" | ||
| printf '%s\n' "$_PROXY_SNIPPET" >>"$tmp" | ||
| cat "$tmp" >"$target" | ||
| rm -f "$tmp" | ||
| return 0 | ||
| fi | ||
| printf '\n%s\n' "$_PROXY_SNIPPET" >>"$target" | ||
| } | ||
|
|
||
| if [ -w "$_SANDBOX_HOME" ]; then | ||
| _write_proxy_snippet "${_SANDBOX_HOME}/.bashrc" | ||
| _write_proxy_snippet "${_SANDBOX_HOME}/.profile" | ||
| fi | ||
| _PROXY_ENV_FILE="/sandbox/.openclaw-data/proxy-env.sh" | ||
| cat > "$_PROXY_ENV_FILE" <<PROXYEOF | ||
| # Proxy configuration (overrides narrow OpenShell defaults on connect) | ||
| export HTTP_PROXY="$_PROXY_URL" | ||
| export HTTPS_PROXY="$_PROXY_URL" | ||
| export NO_PROXY="$_NO_PROXY_VAL" | ||
| export http_proxy="$_PROXY_URL" | ||
| export https_proxy="$_PROXY_URL" | ||
| export no_proxy="$_NO_PROXY_VAL" | ||
| # Tool cache redirects — /sandbox is Landlock read-only (#804) | ||
| export npm_config_cache="/tmp/.npm-cache" | ||
| export XDG_CACHE_HOME="/tmp/.cache" | ||
| export NODE_REPL_HISTORY="/tmp/.node_repl_history" | ||
| export HISTFILE="/tmp/.bash_history" | ||
| export GIT_CONFIG_GLOBAL="/tmp/.gitconfig" | ||
| export PYTHONUSERBASE="/tmp/.local" | ||
| export CLAUDE_CONFIG_DIR="/tmp/.claude" | ||
| PROXYEOF | ||
| chmod 644 "$_PROXY_ENV_FILE" |
Contributor
There was a problem hiding this comment.
Don't auto-source proxy-env.sh from a sandbox-writable directory.
/sandbox/.openclaw-data is sandbox-owned/writable, so the agent can replace proxy-env.sh with arbitrary shell code that every future openshell sandbox connect session sources via .bashrc. On top of that, the root startup cat > "$_PROXY_ENV_FILE" follows attacker-planted symlinks in that directory and can clobber another writable target on the next boot. Move this file to a non-user-writable location, or inject the environment without sourcing a script from a sandbox-writable tree.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/nemoclaw-start.sh` around lines 248 - 273, The proxy env file is
written into a sandbox-writable directory
(_PROXY_ENV_FILE="/sandbox/.openclaw-data/proxy-env.sh") which allows a sandbox
user to replace it with malicious shell code; instead write the proxy env to a
non-user-writable, root-owned location (for example create and use a
system-owned directory like /etc/openclaw or /var/lib/openclaw and set
ownership/mode) and update whatever startup/profile sourcing to point at that
path; ensure the write is done atomically and safely (create a temporary file in
the root-owned dir, set owner to root, chmod 0644, then rename into place) and
avoid following attacker symlinks (use safe file creation APIs or the install
command rather than plain cat > "$_PROXY_ENV_FILE"); also remove or stop
auto-sourcing any file from the sandbox-writable tree so agent-controlled files
cannot be executed at session startup.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/service-env.test.js (1)
187-225: Add guard for empty sed extraction to improve debuggability.Unlike the
extractProxyVarshelper (lines 110-115) which throws a descriptive error when the sed extraction fails, this test would fail with a confusingENOENTerror at line 211 if the script structure changes andpersistBlockis empty.🛠️ Proposed fix to add consistency with existing pattern
const persistBlock = execFileSync( "sed", ["-n", "/^_PROXY_URL=/,/^chmod 644/p", scriptPath], { encoding: "utf-8" } ); + if (!persistBlock.trim()) { + throw new Error( + "Failed to extract proxy persistence block from scripts/nemoclaw-start.sh — " + + "the _PROXY_URL..chmod block may have been moved or renamed" + ); + } const wrapper = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/service-env.test.js` around lines 187 - 225, The test "entrypoint writes proxy-env.sh to writable data dir" extracts a persistBlock via sed but doesn't guard against an empty result, causing a confusing ENOENT later; add the same defensive check used by extractProxyVars (throw a descriptive error when the sed extraction returns an empty string) before writing/executing tmpFile so failures in script structure are reported clearly—specifically check the persistBlock variable after the execFileSync sed call in this test and throw or assert with a helpful message if it's empty (refer to persistBlock and the extractProxyVars pattern for the exact guard behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/service-env.test.js`:
- Around line 187-225: The test "entrypoint writes proxy-env.sh to writable data
dir" extracts a persistBlock via sed but doesn't guard against an empty result,
causing a confusing ENOENT later; add the same defensive check used by
extractProxyVars (throw a descriptive error when the sed extraction returns an
empty string) before writing/executing tmpFile so failures in script structure
are reported clearly—specifically check the persistBlock variable after the
execFileSync sed call in this test and throw or assert with a helpful message if
it's empty (refer to persistBlock and the extractProxyVars pattern for the exact
guard behavior).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21f50e8b-a606-4eeb-881c-034528b7744f
📒 Files selected for processing (1)
test/service-env.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restricts the
/sandboxhome directory to Landlock read-only, preventing agents from creating arbitrary files or modifying their runtime environment. Only explicitly declared paths remain writable.Key changes:
include_workdir: falsein the filesystem policy — verified against OpenShell'slandlock.rsthatinclude_workdir: trueadds WORKDIR toread_write, which would override ourread_onlyentry (Landlock grants the union of all matching rules)/sandboxfromread_writetoread_only/sandbox/.openclaw-data(agent state) and/sandbox/.nemoclaw(plugin state) asread_write/sandbox/.nemoclaw/blueprints/with root ownership (defense-in-depth since the directory is Landlockread_write).bashrc/.profileat image build time — they source proxy config from writable/sandbox/.openclaw-data/proxy-env.sh/tmpvia env vars in both the entrypoint and the sourcedproxy-env.sh(soopenshell sandbox connectsessions also get the redirects)Writable surface after this change:
/sandbox/sandbox/.openclaw/sandbox/.openclaw-data/sandbox/.nemoclaw/tmpRelated Issue
Closes #804
Changes
nemoclaw-blueprint/policies/openclaw-sandbox.yamlinclude_workdir: false,/sandbox→ read_only,/sandbox/.nemoclaw→ read_writeDockerfileDockerfile.base.bashrc/.profilewith proxy-env sourcingscripts/nemoclaw-start.sh/tmpdocs/deployment/sandbox-hardening.mdTesting
nemoclaw onboardcompletes successfully (sandbox creation with new policy)openshell sandbox connect→ interactive shell works, proxy env vars are set/sandbox/.openclaw-data/workspace)/sandbox/(e.g.,touch /sandbox/testfails)openclaw gateway runstarts correctly (reads from read-only.openclaw/)/sandbox/.nemoclaw/state/)npm testpasses (no policy tests assert specific read_write paths)Summary by CodeRabbit
Security & Hardening
Environment Configuration
Documentation
Tests