fix(security): drop cap_fowner and tighten sandbox policy - #1796
Conversation
Reorder chmod before chown on gateway/auto-pair log files so root still owns them when permissions are set, eliminating the need for cap_fowner. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Strip github, clawhub, openclaw_api, and discord network policies from the base allowlist for local-only operation. Presets can re-add them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update from pinned 2026.3.11 to ^2026.4.9 range to pick up recent fixes while allowing compatible patch updates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Track 9 transitive dependency vulns (Lark/Discord) in openclaw that are mitigated by the deny-by-default sandbox network policy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request introduces security hardening measures to the OpenClaw system. It adds documentation of upstream vulnerabilities, removes optional network policies for external services (clawhub, openclaw_api, discord), removes the file owner capability from the startup script, and implements symlink-safe log file initialization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 387-394: The current touch/chmod/chown on predictable /tmp paths
(/tmp/gateway.log and /tmp/auto-pair.log) is vulnerable to symlink/path
hijacking; replace the create-then-change sequence with a secure atomic
create-and-install: create a secure temp file with mktemp in /tmp, atomically mv
(or mv -T) that temp file to /tmp/gateway.log, then run chmod 600 and chown
gateway:gateway; repeat the same pattern for /tmp/auto-pair.log and chown
sandbox:sandbox. Update the commands in nemoclaw-start.sh where the
touch/chmod/chown for gateway and auto-pair are performed to use the mktemp ->
mv -> chmod -> chown sequence so the file cannot be pre-created as a symlink.
In `@SECURITY_NOTES.md`:
- Around line 1-3: Add a single-line SPDX license header to the top of
SECURITY_NOTES.md (as required for all .md files) by inserting
"SPDX-License-Identifier: <REPO_LICENSE>" on the first line of the file (replace
<REPO_LICENSE> with the repository's official SPDX token, e.g., MIT or
Apache-2.0), ensuring there is no other content or blank lines before it so the
header is recognized by tooling; this fixes the missing SPDX header violation
for SECURITY_NOTES.md.
- Around line 1-3: The three lines in SECURITY_NOTES.md are intended as Markdown
headings and must use correct product-name casing; update "openclaw@2026.4.11"
to "OpenClaw@2026.4.11" (and ensure any occurrences of "NemoClaw" and
"OpenShell" use that exact casing), keep the leading "# " heading markers, and
add a blank line after each heading so the file conforms to MD heading structure
and avoids MD022 warnings (also verify related phrases like "Lark/Discord deps",
"deny-by-default network policy in allspark sandbox", and "axios/tar dep bumps"
remain as heading content with corrected casing if needed).
🪄 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: 98487ae3-7c89-428d-8697-57f173a1e3d2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
SECURITY_NOTES.mdnemoclaw-blueprint/policies/openclaw-sandbox.yamlpackage.jsonscripts/nemoclaw-start.sh
💤 Files with no reviewable changes (1)
- nemoclaw-blueprint/policies/openclaw-sandbox.yaml
| # 9 upstream vulns in openclaw@2026.4.11 (Lark/Discord deps) | ||
| # Blocked by deny-by-default network policy in allspark sandbox | ||
| # Revisit when openclaw ships axios/tar dep bumps |
There was a problem hiding this comment.
Add SPDX header to this Markdown file.
This file currently has no SPDX header, which violates repo licensing requirements.
Suggested patch
+<!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. -->
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+
# 9 upstream vulns in openclaw@2026.4.11 (Lark/Discord deps)
# Blocked by deny-by-default network policy in allspark sandbox
# Revisit when openclaw ships axios/tar dep bumpsAs per coding guidelines, "**/*.{js,ts,tsx,jsx,sh,md}: Include SPDX license header in every source file".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 9 upstream vulns in openclaw@2026.4.11 (Lark/Discord deps) | |
| # Blocked by deny-by-default network policy in allspark sandbox | |
| # Revisit when openclaw ships axios/tar dep bumps | |
| <!-- SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> | |
| <!-- SPDX-License-Identifier: Apache-2.0 --> | |
| # 9 upstream vulns in openclaw@2026.4.11 (Lark/Discord deps) | |
| # Blocked by deny-by-default network policy in allspark sandbox | |
| # Revisit when openclaw ships axios/tar dep bumps |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY_NOTES.md` around lines 1 - 3, Add a single-line SPDX license header
to the top of SECURITY_NOTES.md (as required for all .md files) by inserting
"SPDX-License-Identifier: <REPO_LICENSE>" on the first line of the file (replace
<REPO_LICENSE> with the repository's official SPDX token, e.g., MIT or
Apache-2.0), ensuring there is no other content or blank lines before it so the
header is recognized by tooling; this fixes the missing SPDX header violation
for SECURITY_NOTES.md.
Fix Markdown structure and product-name casing (OpenClaw).
These lines are headings (not comments), trigger MD022 warnings, and use openclaw instead of OpenClaw.
Suggested patch
-# 9 upstream vulns in openclaw@2026.4.11 (Lark/Discord deps)
-# Blocked by deny-by-default network policy in allspark sandbox
-# Revisit when openclaw ships axios/tar dep bumps
+OpenClaw security snapshot:
+
+- 9 upstream vulnerabilities in OpenClaw@2026.4.11 (Lark/Discord dependencies).
+- Currently mitigated by the deny-by-default network policy in the allspark sandbox.
+- Revisit when OpenClaw ships axios/tar dependency bumps.As per coding guidelines, "**/*.md: NemoClaw, OpenClaw, and OpenShell must use correct casing."
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
[warning] 2-2: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@SECURITY_NOTES.md` around lines 1 - 3, The three lines in SECURITY_NOTES.md
are intended as Markdown headings and must use correct product-name casing;
update "openclaw@2026.4.11" to "OpenClaw@2026.4.11" (and ensure any occurrences
of "NemoClaw" and "OpenShell" use that exact casing), keep the leading "# "
heading markers, and add a blank line after each heading so the file conforms to
MD heading structure and avoids MD022 warnings (also verify related phrases like
"Lark/Discord deps", "deny-by-default network policy in allspark sandbox", and
"axios/tar dep bumps" remain as heading content with corrected casing if
needed).
Reject symlink targets before creating gateway.log and auto-pair.log, then use mktemp+mv to avoid TOCTOU races during log initialization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reformat as proper markdown with headings, blank lines, and correct OpenClaw casing per CodeRabbit review feedback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/nemoclaw-start.sh (2)
386-406: Solid symlink hijacking mitigation with mktemp+mv pattern.The implementation correctly addresses the TOCTOU concern: checking for symlinks upfront, clearing existing files, then using
mktempwith secure permissions applied before the atomicmv. Sincemvreplaces the target path rather than following symlinks, the race window is closed.One edge case to consider:
rm -fsilently ignores directories. If an attacker pre-creates/tmp/gateway.logas a directory, therm -fwould no-op andmvwould move the file into that directory. This is DoS-only (gateway fails to start) rather than privilege escalation, so it may not warrant additional complexity.,
🛡️ Optional: reject directories as well
for _log_path in /tmp/gateway.log /tmp/auto-pair.log; do - if [ -L "$_log_path" ]; then - echo "[SECURITY] $_log_path is a symlink — refusing to start (possible symlink hijack)" >&2 + if [ -L "$_log_path" ] || [ -d "$_log_path" ]; then + echo "[SECURITY] $_log_path is a symlink or directory — refusing to start" >&2 exit 1 fi rm -f "$_log_path" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 386 - 406, The loop that protects /tmp/gateway.log and /tmp/auto-pair.log should also reject pre-existing directories to avoid mv placing the temp file into a directory; update the check around _log_path (the for-loop and checks referencing _log_path and the mktemp/mv sequence) to treat a directory as an immediate fatal error (similar to the existing symlink check) so the script exits if _log_path is a directory before rm/mktemp/mv proceed.
356-361: Consider consistent symlink protection in non-root fallback path.The root path (lines 390-406) now has symlink-safe log creation, but the non-root fallback here still uses
touch+chmod. Since privilege separation is disabled in this mode, the risk is lower — nochownoccurs, so no ownership-override attack is possible. However, for defense-in-depth consistency, the same pattern could be applied.♻️ Optional: apply similar protection
- touch /tmp/gateway.log - chmod 600 /tmp/gateway.log - - # Separate log for auto-pair in non-root mode as well. - touch /tmp/auto-pair.log - chmod 600 /tmp/auto-pair.log + # Apply symlink protection even in non-root mode for consistency + for _log_path in /tmp/gateway.log /tmp/auto-pair.log; do + if [ -L "$_log_path" ]; then + echo "[SECURITY] $_log_path is a symlink — refusing to start" >&2 + exit 1 + fi + rm -f "$_log_path" + _tmp_log="$(mktemp "${_log_path}.XXXXXX")" + chmod 600 "$_tmp_log" + mv "$_tmp_log" "$_log_path" + done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 356 - 361, Replace the non-root fallback's simple touch+chmod for /tmp/gateway.log and /tmp/auto-pair.log with the same symlink-safe log creation routine used in the root path: instead of touch + chmod, create the file via a temporary file (mktemp), verify/avoid symlinks, atomically move/install it into place (or use an atomic open with O_CREAT|O_EXCL), then set permissions to 600; update the operations that currently call touch /tmp/gateway.log and chmod 600 /tmp/gateway.log (and the analogous touch/chmod for /tmp/auto-pair.log) to use that symlink-safe sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 386-406: The loop that protects /tmp/gateway.log and
/tmp/auto-pair.log should also reject pre-existing directories to avoid mv
placing the temp file into a directory; update the check around _log_path (the
for-loop and checks referencing _log_path and the mktemp/mv sequence) to treat a
directory as an immediate fatal error (similar to the existing symlink check) so
the script exits if _log_path is a directory before rm/mktemp/mv proceed.
- Around line 356-361: Replace the non-root fallback's simple touch+chmod for
/tmp/gateway.log and /tmp/auto-pair.log with the same symlink-safe log creation
routine used in the root path: instead of touch + chmod, create the file via a
temporary file (mktemp), verify/avoid symlinks, atomically move/install it into
place (or use an atomic open with O_CREAT|O_EXCL), then set permissions to 600;
update the operations that currently call touch /tmp/gateway.log and chmod 600
/tmp/gateway.log (and the analogous touch/chmod for /tmp/auto-pair.log) to use
that symlink-safe sequence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e15e031b-486b-49e9-b65d-853c449eaf3e
📒 Files selected for processing (2)
SECURITY_NOTES.mdscripts/nemoclaw-start.sh
✅ Files skipped from review due to trivial changes (1)
- SECURITY_NOTES.md
|
Please provide more information summarizing this PR, and related issues, thanks. |
|
Friendly AI-generated maintainer note: Thanks for the hardening work here. I took a pass through the current diff and I can't move it through the fast path yet. Please address these blockers:
Once those are in place, I'm happy to re-run the gate check. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 954-962: The move operations for temporary log files (_gw_log and
_ap_log) use plain mv which can accidentally place the temp file inside a
directory if the destination path is recreated; replace those calls with mv -T
when renaming the temp files to the final paths (/tmp/gateway.log and
/tmp/auto-pair.log) so the rename always replaces the destination pathname
atomically and avoids a TOCTOU window.
🪄 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 Plus
Run ID: 0f8e1a55-4a2e-4c88-8157-5e791c499e8f
📒 Files selected for processing (3)
SECURITY_NOTES.mdnemoclaw-blueprint/policies/openclaw-sandbox.yamlscripts/nemoclaw-start.sh
✅ Files skipped from review due to trivial changes (1)
- SECURITY_NOTES.md
| _gw_log="$(mktemp /tmp/gateway.log.XXXXXX)" | ||
| chmod 600 "$_gw_log" | ||
| chown gateway:gateway "$_gw_log" | ||
| mv "$_gw_log" /tmp/gateway.log | ||
|
|
||
| # Separate log for auto-pair so sandbox user can write to it | ||
| touch /tmp/auto-pair.log | ||
| chown sandbox:sandbox /tmp/auto-pair.log | ||
| chmod 600 /tmp/auto-pair.log | ||
| _ap_log="$(mktemp /tmp/auto-pair.log.XXXXXX)" | ||
| chmod 600 "$_ap_log" | ||
| chown sandbox:sandbox "$_ap_log" | ||
| mv "$_ap_log" /tmp/auto-pair.log |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
touch "$tmpdir/src"
mkdir "$tmpdir/dst"
mv "$tmpdir/src" "$tmpdir/dst"
test -f "$tmpdir/dst/src"
touch "$tmpdir/src2"
if mv -T "$tmpdir/src2" "$tmpdir/dst" 2>/dev/null; then
echo "unexpected: mv -T replaced a directory"
exit 1
fi
echo "plain mv treats an existing directory as a directory; mv -T rejects it"Repository: NVIDIA/NemoClaw
Length of output: 129
🏁 Script executed:
# Check if file exists and read the relevant section
fd -t f "nemoclaw-start.sh" scripts/Repository: NVIDIA/NemoClaw
Length of output: 84
🏁 Script executed:
# Read lines around 954-962 to verify the snippet and context
sed -n '950,965p' scripts/nemoclaw-start.sh | cat -nRepository: NVIDIA/NemoClaw
Length of output: 613
🏁 Script executed:
# Check for shebang and SPDX header at the top of the file
head -20 scripts/nemoclaw-start.sh | cat -nRepository: NVIDIA/NemoClaw
Length of output: 1433
Use mv -T for the final log-path replace.
Lines 957 and 962 use plain mv. If the destination is recreated as a directory between the rm -f step and the rename, mv will move the temp file into that directory instead of replacing the pathname, which weakens the TOCTOU hardening here.
Suggested fix
_gw_log="$(mktemp /tmp/gateway.log.XXXXXX)"
chmod 600 "$_gw_log"
chown gateway:gateway "$_gw_log"
-mv "$_gw_log" /tmp/gateway.log
+mv -T "$_gw_log" /tmp/gateway.log
@@
_ap_log="$(mktemp /tmp/auto-pair.log.XXXXXX)"
chmod 600 "$_ap_log"
chown sandbox:sandbox "$_ap_log"
-mv "$_ap_log" /tmp/auto-pair.log
+mv -T "$_ap_log" /tmp/auto-pair.log📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _gw_log="$(mktemp /tmp/gateway.log.XXXXXX)" | |
| chmod 600 "$_gw_log" | |
| chown gateway:gateway "$_gw_log" | |
| mv "$_gw_log" /tmp/gateway.log | |
| # Separate log for auto-pair so sandbox user can write to it | |
| touch /tmp/auto-pair.log | |
| chown sandbox:sandbox /tmp/auto-pair.log | |
| chmod 600 /tmp/auto-pair.log | |
| _ap_log="$(mktemp /tmp/auto-pair.log.XXXXXX)" | |
| chmod 600 "$_ap_log" | |
| chown sandbox:sandbox "$_ap_log" | |
| mv "$_ap_log" /tmp/auto-pair.log | |
| _gw_log="$(mktemp /tmp/gateway.log.XXXXXX)" | |
| chmod 600 "$_gw_log" | |
| chown gateway:gateway "$_gw_log" | |
| mv -T "$_gw_log" /tmp/gateway.log | |
| _ap_log="$(mktemp /tmp/auto-pair.log.XXXXXX)" | |
| chmod 600 "$_ap_log" | |
| chown sandbox:sandbox "$_ap_log" | |
| mv -T "$_ap_log" /tmp/auto-pair.log |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/nemoclaw-start.sh` around lines 954 - 962, The move operations for
temporary log files (_gw_log and _ap_log) use plain mv which can accidentally
place the temp file inside a directory if the destination path is recreated;
replace those calls with mv -T when renaming the temp files to the final paths
(/tmp/gateway.log and /tmp/auto-pair.log) so the rename always replaces the
destination pathname atomically and avoids a TOCTOU window.
|
Thanks for this. Since #1705 has merged and changed the base sandbox policy, could you please rebase this PR on current main and clarify the remaining security changes this PR is intended to carry forward? In particular, it would help to separate what is still needed beyond the messaging-policy cleanup from #1705. |
|
Closing due to inactivity. This PR has been waiting on author clarification since the needs-info request, and it now also needs to be rebased against current main before review can continue. Feel free to reopen once the branch is updated and the remaining security changes are clarified, or open a fresh PR with the current intended diff. Thanks for contributing! |
wscurran
added
needs: info
4 participants
Summary
Related Issue
Changes
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
nemoclaw-contributor-update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/nemoclaw-contributor-update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: Your Name your-email@example.com
Summary by CodeRabbit
Documentation
Security