fix: harden sandbox image — remove gcc/netcat, add process limits, drop capabilities#830
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRuntime image now purges build toolchain packages and network probing utilities, runs apt autoremove and clears apt caches. A startup script was updated to set process ulimits early. New documentation describes the sandbox hardening, ulimit guidance, and recommended runtime capability settings. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 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 |
There was a problem hiding this comment.
Pull request overview
Hardens the NemoClaw sandbox runtime by reducing available tooling, adding process-count limits to mitigate fork-bombs, and documenting capability-dropping guidance for container runtimes (fixes #807, #808, #809; addresses #797).
Changes:
- Adds an
nproc(max processes) limit viaulimitin the startup script and container entrypoint. - Removes/purges compiler toolchain and netcat variants from the runtime image.
- Adds deployment documentation for sandbox hardening (capabilities,
ulimits, read-only FS, etc.).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/nemoclaw-start.sh | Applies ulimit -u 512 during startup to reduce fork-bomb risk. |
| docs/deployment/sandbox-hardening.md | Documents image hardening steps and secure runtime flags (cap-drop, ulimits, etc.). |
| Dockerfile | Purges risky tooling from runtime and enforces process limits in ENTRYPOINT. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/nemoclaw-start.sh
Outdated
| set -euo pipefail | ||
|
|
||
| # Harden: limit process count to prevent fork bombs (ref: #809) | ||
| ulimit -u 512 2>/dev/null || true |
There was a problem hiding this comment.
ulimit -u 512 sets the soft limit by default; an untrusted process can often raise its soft limit back up to the inherited hard limit (potentially unlimited), bypassing the fork-bomb mitigation. Consider setting the hard limit as well (and then the soft limit) so the cap can’t be increased within the container.
| ulimit -u 512 2>/dev/null || true | |
| ulimit -H -u 512 2>/dev/null || true | |
| ulimit -S -u 512 2>/dev/null || true |
There was a problem hiding this comment.
Fixed — now setting both hard and soft limits.
Dockerfile
Outdated
|
|
||
| ENTRYPOINT ["/bin/bash"] | ||
| # Enforce process limits to mitigate fork bombs (#809) | ||
| ENTRYPOINT ["/bin/bash", "-c", "ulimit -u 512 2>/dev/null; exec \"$@\"", "--"] |
There was a problem hiding this comment.
Same issue as the startup script: ulimit -u 512 typically only constrains the soft limit, which can be raised up to the hard limit by the running process. To make this mitigation effective, set the hard limit too (and then set soft to match) inside the ENTRYPOINT wrapper.
| ENTRYPOINT ["/bin/bash", "-c", "ulimit -u 512 2>/dev/null; exec \"$@\"", "--"] | |
| ENTRYPOINT ["/bin/bash", "-c", "ulimit -Hu 512 2>/dev/null; ulimit -Su 512 2>/dev/null; exec \"$@\"", "--"] |
There was a problem hiding this comment.
Fixed in the same commit.
docs/deployment/sandbox-hardening.md
Outdated
| docker run --rm \ | ||
| --cap-drop=ALL \ | ||
| --cap-add=NET_BIND_SERVICE \ | ||
| --ulimit nproc=512:512 \ | ||
| nemoclaw-sandbox |
There was a problem hiding this comment.
The example re-adds NET_BIND_SERVICE, but the documented/default UI port in this repo context is 18789 (>1024), which doesn’t require binding privileged ports. Recommending an extra capability when it may not be needed weakens the hardening guidance—either remove --cap-add=NET_BIND_SERVICE from the baseline example or annotate it as optional/only required when binding to ports <1024.
There was a problem hiding this comment.
Removed — port 18789 doesn't need it.
![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
🧹 Nitpick comments (4)
docs/deployment/sandbox-hardening.md (4)
38-57: Add an introductory sentence under this H3 section before the code block.
### Docker Compose Examplecurrently starts with YAML immediately.As per coding guidelines, "Sections use H2 and H3, each starting with an introductory sentence."
🤖 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 38 - 57, The "Docker Compose Example" H3 section currently jumps straight into the YAML block; add a brief introductory sentence immediately under the "### Docker Compose Example" header (before the fenced code block) that explains what the example shows (e.g., that it demonstrates a hardened Docker Compose service definition for the nemoclaw-sandbox service), so update the section containing the heading "Docker Compose Example" to insert that one-line intro above the YAML code block.
59-62: Replace the blockquote note with a MyST admonition.Use
:::{note}instead of> **Note:** .... Also, the bold emphasis here is an LLM-pattern style issue. LLM pattern detected.As per coding guidelines, "Use MyST admonitions (:::{tip}, :::{note}, :::{warning}) for callouts, not bold text or blockquotes." and "Unnecessary bold on routine instructions ... should be flagged. LLM pattern detected."
🤖 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 59 - 62, Replace the blockquote that starts with "> **Note:** The `Dockerfile` itself cannot enforce `--cap-drop` ..." with a MyST admonition using the :::{note} ... ::: syntax; remove the bold markup and rewrite the same sentence inside the admonition so it uses MyST callout formatting (use :::{note} before the text and ::: after) instead of the LLM-style "> **Note:**" blockquote.
64-69: Add a finalNext Stepssection with related links.The page currently ends at
Referencesand is missing the required closing navigation section.As per coding guidelines, "A 'Next Steps' section at the bottom links to related pages."
🤖 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 64 - 69, Add a final "Next Steps" section after the existing "References" heading that provides navigation to related documentation; create a level-2 heading "Next Steps" and include a short bulleted list of links to relevant pages (e.g., deployment hardening, sandbox configuration, troubleshooting or related issues) using the same markdown link style as the rest of the file so the page ends with the required closing navigation section.
3-24: Use one sentence per source line in prose paragraphs.Several sentences are wrapped across multiple source lines, which makes diffs noisier in docs edits.
As per coding guidelines, "One sentence per line in source (makes diffs readable)."
🤖 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 3 - 24, The prose in docs/deployment/sandbox-hardening.md currently wraps multiple sentences per source line (notably in the "Removed Unnecessary Tools" and "Process Limits" paragraphs and lines mentioning ENTRYPOINT, `ulimit -u 512`, and `nemoclaw-start.sh`), which makes diffs noisy; rewrite the paragraphs so each sentence occupies its own source line—split long lines into separate lines at sentence boundaries, preserving existing wording and references (e.g., `gcc`, `g++`, `make`, `netcat`, `builder`, `ENTRYPOINT`, `--ulimit nproc=512:512`) to comply with the "one sentence per line" guideline.
🤖 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 23-25: The shell `|| true` at the end of the long `&&` chain makes
the whole chain succeed even if earlier commands (like `apt-get update` or
`apt-get install`) fail; restrict the error masking to only the remove step by
grouping that command and its redirection and `|| true` so it doesn't apply to
previous commands. Locate the `apt-get remove --purge -y gcc ...
netcat-traditional ncat 2>/dev/null || true` fragment in the Dockerfile and
change it to a grouped form (e.g. wrap the remove command in parentheses or
braces with its `2>/dev/null || true`) so only that remove command is tolerant
of failure while the earlier `apt-get update`/`apt-get install` steps still
cause the build to fail on error.
In `@docs/deployment/sandbox-hardening.md`:
- Around line 30-36: The code block showing the docker run example is using a
```bash fence and lacks prompt prefixes; change the fence to use the `console`
language tag and add a leading `$ ` prompt to each CLI line (the docker
invocation lines including `docker run --rm \`, `--cap-drop=ALL \`,
`--cap-add=NET_BIND_SERVICE \`, `--ulimit nproc=512:512 \`, and
`nemoclaw-sandbox`) so the snippet follows the CLI guideline of `console` with
`$` prompts.
- Around line 1-2: Add canonical frontmatter and an SPDX license header to this
new docs page: wrap the existing top-level heading "# Sandbox Image Hardening"
with YAML frontmatter that includes title, description, keywords, topics, tags,
content type, difficulty, audience, and status fields, and then insert the SPDX
license header immediately after the closing frontmatter marker; ensure the SPDX
header uses the project's standard identifier (e.g., "SPDX-License-Identifier:
<LICENSE>") and that the frontmatter values accurately describe the "Sandbox
Image Hardening" page.
---
Nitpick comments:
In `@docs/deployment/sandbox-hardening.md`:
- Around line 38-57: The "Docker Compose Example" H3 section currently jumps
straight into the YAML block; add a brief introductory sentence immediately
under the "### Docker Compose Example" header (before the fenced code block)
that explains what the example shows (e.g., that it demonstrates a hardened
Docker Compose service definition for the nemoclaw-sandbox service), so update
the section containing the heading "Docker Compose Example" to insert that
one-line intro above the YAML code block.
- Around line 59-62: Replace the blockquote that starts with "> **Note:** The
`Dockerfile` itself cannot enforce `--cap-drop` ..." with a MyST admonition
using the :::{note} ... ::: syntax; remove the bold markup and rewrite the same
sentence inside the admonition so it uses MyST callout formatting (use :::{note}
before the text and ::: after) instead of the LLM-style "> **Note:**"
blockquote.
- Around line 64-69: Add a final "Next Steps" section after the existing
"References" heading that provides navigation to related documentation; create a
level-2 heading "Next Steps" and include a short bulleted list of links to
relevant pages (e.g., deployment hardening, sandbox configuration,
troubleshooting or related issues) using the same markdown link style as the
rest of the file so the page ends with the required closing navigation section.
- Around line 3-24: The prose in docs/deployment/sandbox-hardening.md currently
wraps multiple sentences per source line (notably in the "Removed Unnecessary
Tools" and "Process Limits" paragraphs and lines mentioning ENTRYPOINT, `ulimit
-u 512`, and `nemoclaw-start.sh`), which makes diffs noisy; rewrite the
paragraphs so each sentence occupies its own source line—split long lines into
separate lines at sentence boundaries, preserving existing wording and
references (e.g., `gcc`, `g++`, `make`, `netcat`, `builder`, `ENTRYPOINT`,
`--ulimit nproc=512:512`) to comply with the "one sentence per line" guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34b06a27-121c-4a29-82a4-fba458cc626d
📒 Files selected for processing (3)
Dockerfiledocs/deployment/sandbox-hardening.mdscripts/nemoclaw-start.sh
|
|
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
172-174: ENTRYPOINT pattern is correct; note&&chaining behavior.The Docker
ENTRYPOINT/CMDpattern with"--"as$0placeholder is valid. The;beforeexecensures the container always starts.One consideration: using
&&between the twoulimitcommands means if the hard limit fails, the soft limit won't be attempted. This is likely intentional (soft limit alone offers no protection since it can be raised), but if you want both to be attempted independently, consider using;instead:-ENTRYPOINT ["/bin/bash", "-c", "ulimit -Hu 512 2>/dev/null && ulimit -Su 512 2>/dev/null; exec \"$@\"", "--"] +ENTRYPOINT ["/bin/bash", "-c", "ulimit -Hu 512 2>/dev/null; ulimit -Su 512 2>/dev/null; exec \"$@\"", "--"]Additionally, the silent suppression via
2>/dev/nullmeans operators won't know if process limits fail to apply. For production hardening, consider logging a warning or using a health check to verify limits are in effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 172 - 174, The ENTRYPOINT uses `ulimit -Hu 512 && ulimit -Su 512 2>/dev/null` so the soft limit is skipped if the hard limit fails and stderr is suppressed; update the `ENTRYPOINT` invocation (the `ENTRYPOINT`, `CMD`, `ulimit`, `exec`, and `"--"` sequence) to run both `ulimit` calls independently (use `;` between them) and stop silencing failures (remove or redirect `2>/dev/null`) or add a visible warning/logging mechanism so operators can see if limits failed to apply.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 172-174: The ENTRYPOINT uses `ulimit -Hu 512 && ulimit -Su 512
2>/dev/null` so the soft limit is skipped if the hard limit fails and stderr is
suppressed; update the `ENTRYPOINT` invocation (the `ENTRYPOINT`, `CMD`,
`ulimit`, `exec`, and `"--"` sequence) to run both `ulimit` calls independently
(use `;` between them) and stop silencing failures (remove or redirect
`2>/dev/null`) or add a visible warning/logging mechanism so operators can see
if limits failed to apply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8aef0119-f91a-42a3-8c49-38e99d3835b8
📒 Files selected for processing (2)
Dockerfiledocs/deployment/sandbox-hardening.md
✅ Files skipped from review due to trivial changes (1)
- docs/deployment/sandbox-hardening.md
|
This does not address #797 as the writer claims |
|
You're right — updated the PR description. #797 needs a runtime-level fix (compose/docker flags), not just docs. Removed it from the Fixes list. |
BenediktSchackenberg
force-pushed
the
fix/sandbox-image-hardening
branch
from
856adc5 to
5b4c659
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
189-193:⚠️ Potential issue | 🔴 CriticalConfig hash file is no longer generated, breaking startup and e2e tests.
The build-time
.config-hashgeneration was removed, butscripts/nemoclaw-start.shstill callsverify_config_integrity()which expects/sandbox/.openclaw/.config-hashto exist. In root mode (line 209), this is called without error handling, causing the container to refuse startup with "[SECURITY] Config hash file missing".Additionally, the e2e test at
test/e2e-gateway-isolation.sh:98-102explicitly validates that the hash file exists and matchesopenclaw.json.Either restore the hash generation or update
verify_config_integrity()and the e2e tests to handle the missing file.Proposed fix — restore config hash generation
Add after line 193 (after locking openclaw.json permissions):
&& chmod 444 /sandbox/.openclaw/openclaw.json + +# Pin config hash for runtime integrity verification (ref: nemoclaw-start.sh) +RUN cd /sandbox/.openclaw \ + && sha256sum openclaw.json > .config-hash \ + && chmod 444 .config-hash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 189 - 193, The container startup fails because the build step that generated /sandbox/.openclaw/.config-hash was removed but scripts/nemoclaw-start.sh still calls verify_config_integrity() (which expects .config-hash) and e2e tests in test/e2e-gateway-isolation.sh assert the file exists; restore the missing .config-hash generation in the Dockerfile immediately after locking openclaw.json (after the chmod lines) by computing a stable hash (e.g., SHA256) of /sandbox/.openclaw/openclaw.json and writing it to /sandbox/.openclaw/.config-hash with appropriate ownership/permissions, or alternatively update verify_config_integrity() and the e2e test to tolerate a missing .config-hash by implementing fallback behavior — reference verify_config_integrity(), scripts/nemoclaw-start.sh, .config-hash, openclaw.json, and test/e2e-gateway-isolation.sh when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile`:
- Around line 189-193: The container startup fails because the build step that
generated /sandbox/.openclaw/.config-hash was removed but
scripts/nemoclaw-start.sh still calls verify_config_integrity() (which expects
.config-hash) and e2e tests in test/e2e-gateway-isolation.sh assert the file
exists; restore the missing .config-hash generation in the Dockerfile
immediately after locking openclaw.json (after the chmod lines) by computing a
stable hash (e.g., SHA256) of /sandbox/.openclaw/openclaw.json and writing it to
/sandbox/.openclaw/.config-hash with appropriate ownership/permissions, or
alternatively update verify_config_integrity() and the e2e test to tolerate a
missing .config-hash by implementing fallback behavior — reference
verify_config_integrity(), scripts/nemoclaw-start.sh, .config-hash,
openclaw.json, and test/e2e-gateway-isolation.sh when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d421c9f-e655-45b6-8b79-63c21ceadd5f
📒 Files selected for processing (3)
Dockerfiledocs/deployment/sandbox-hardening.mdscripts/nemoclaw-start.sh
✅ Files skipped from review due to trivial changes (1)
- docs/deployment/sandbox-hardening.md
|
@BenediktSchackenberg sorry I only saw your comment on #917 a second after merging the PR. I'd still like to get these changes in, would you mind resolving the conflicts, please? |
…es, add process limits - Remove gcc, g++, make, netcat from runtime image to reduce attack surface - Add ulimit -u 512 in ENTRYPOINT and start script to prevent fork bombs - Add deployment doc with Docker Compose example for dropping capabilities - Document security hardening measures Fixes NVIDIA#807, NVIDIA#808, NVIDIA#809. Addresses NVIDIA#797.
…SERVICE - Set both hard and soft ulimit in nemoclaw-start.sh and Dockerfile ENTRYPOINT to prevent agent from raising soft limit back up - Restore CMD ["/bin/bash"] so 'docker run -it' still drops into a shell - Remove --cap-add=NET_BIND_SERVICE from docs example since port 18789 > 1024
Address CodeRabbit review comments: - Dockerfile: wrap apt-get remove in subshell so || true only applies to the remove command, not the entire RUN chain - docs/sandbox-hardening.md: add YAML frontmatter and SPDX header - docs/sandbox-hardening.md: use console fence with $ prompt for CLI examples
BenediktSchackenberg
force-pushed
the
fix/sandbox-image-hardening
branch
from
5b4c659 to
76dc5dc
Compare
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 18-20: The ulimit calls in scripts/nemoclaw-start.sh currently
suppress errors (the `2>/dev/null || true`), so replace that silent suppression
with explicit failure and verification: run `ulimit -Hu 512` and `ulimit -Su
512` without redirection, check their exit status and/or read back `ulimit -Hu`
and `ulimit -Su` to ensure they report >=512, and if either call fails or
returns a lower value print a clear error to stderr and exit non‑zero so the
ENTRYPOINT fails loudly; update the script around the `ulimit -Hu` and `ulimit
-Su` lines to perform these checks and exit on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 171b136c-99a4-4537-8fd3-73b2e420e144
📒 Files selected for processing (3)
Dockerfiledocs/deployment/sandbox-hardening.mdscripts/nemoclaw-start.sh
✅ Files skipped from review due to trivial changes (1)
- docs/deployment/sandbox-hardening.md
Remove silent error suppression (2>/dev/null || true) so the entrypoint exits non-zero if ulimit fails, preventing the container from running without fork-bomb protection.
BenediktSchackenberg
force-pushed
the
fix/sandbox-image-hardening
branch
from
1f7f1c5 to
d102f50
Compare
Fixes Sphinx build error: 'document isn't included in any toctree'
cv
force-pushed
the
fix/sandbox-image-hardening
branch
from
d0a13b3 to
c2d0c5f
Compare
…test The gateway isolation E2E test sourced the first 21 lines of nemoclaw-start to capture the PATH export. After #830 added ulimit lines, the PATH export moved to line 30 and the test broke. Replace head -21 with sed -n "1,/^export PATH=/p" so the test dynamically finds the PATH export regardless of line number.
The gateway isolation test previously sourced partial lines of the entrypoint script to check PATH. This broke whenever the preamble changed (e.g., #830 added ulimit lines). The grep fix on main is also fragile since it sources a single line without preamble context. Replace with a static file content check: verify the entrypoint contains exactly one PATH export with the expected locked-down value. This tests the security property directly without depending on line numbers or partial script execution.
Hardens the sandbox image: removes gcc/g++/make/netcat from runtime, adds ulimit -u 512 for fork bomb protection, adds docs for cap-drop=ALL. Fixes #807, #808, #809.
Note: #797 (capability dropping) is documented in the hardening guide but not enforced at container level — that needs a runtime/compose-level fix.
AI-assisted: Built with Claude, reviewed by human.
Summary by CodeRabbit
Documentation
Chores