fix(security): make .openclaw symlinks immutable after validation#1137
Conversation
📝 WalkthroughWalkthroughAfter validating symlinks under Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
|
I opened a small follow-up PR on top of this branch to tighten the hardening path and make it easier to review operationally: latenighthackathon#1 It adds two things on top of the current fix:
The intent is to keep the current defense-in-depth approach while making silent regressions and silent no-op hardening easier to catch. |
The symlink validation loop in nemoclaw-start.sh verifies that all symlinks in /sandbox/.openclaw/ point to their expected /sandbox/.openclaw-data/ targets, but this check runs only once at boot. After validation, the symlinks could theoretically be swapped before the gateway starts on the next line (TOCTOU). While DAC already prevents the sandbox user from modifying the root-owned /sandbox/.openclaw directory, this adds defense-in-depth by setting the immutable flag (chattr +i) on both the directory and its symlinks after validation passes. The immutable flag cannot be removed by the sandbox user, closing the TOCTOU window even if DAC or Landlock are bypassed. The fix degrades gracefully: if chattr is not available or the filesystem does not support immutable flags, the existing DAC protections remain in effect. Closes NVIDIA#1019 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
99ef53b to
5ab4386
Compare
) ## Summary The symlink validation loop in `nemoclaw-start.sh` verifies that all symlinks in `/sandbox/.openclaw/` point to their expected `/sandbox/.openclaw-data/` targets, but this check runs only once at boot. After validation completes (line 360), the gateway starts on the next line (line 366), leaving a TOCTOU window where symlinks could theoretically be swapped. This adds defense-in-depth by setting the immutable flag (`chattr +i`) on both the `/sandbox/.openclaw` directory and its validated symlinks immediately after the validation loop passes. ## Changes - Added `chattr +i` on `/sandbox/.openclaw` directory and all symlinks after the validation loop - Guarded behind `command -v chattr` check — degrades gracefully if not available - All `chattr` calls use `|| true` to avoid failures on filesystems that don't support immutable flags ## Security Analysis | Layer | Protection | Status | |-------|-----------|--------| | DAC (chmod/chown) | Directory owned by root:root, mode 755 | Already in place (Dockerfile) | | Landlock | `/sandbox/.openclaw` in read-only policy | `best_effort` — not enforced on older kernels | | **Immutable flag** | `chattr +i` prevents modification even by root | **Added by this PR** | The immutable flag cannot be removed by the sandbox user (requires `CAP_LINUX_IMMUTABLE`), closing the TOCTOU window even if DAC or Landlock are bypassed. ## Test Plan - [x] Full test suite: 650/653 passed (3 pre-existing failures in `service-env.test.js`) - [x] Verified graceful degradation when `chattr` is unavailable - [x] Confirmed `chattr` guard (`command -v`) prevents errors in minimal containers Closes #1019 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added additional sandbox hardening: verification of internal links followed by an optional immutability step to protect those links from runtime modification, improving integrity and reducing risk of unauthorized changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
…IDIA#1137) ## Summary The symlink validation loop in `nemoclaw-start.sh` verifies that all symlinks in `/sandbox/.openclaw/` point to their expected `/sandbox/.openclaw-data/` targets, but this check runs only once at boot. After validation completes (line 360), the gateway starts on the next line (line 366), leaving a TOCTOU window where symlinks could theoretically be swapped. This adds defense-in-depth by setting the immutable flag (`chattr +i`) on both the `/sandbox/.openclaw` directory and its validated symlinks immediately after the validation loop passes. ## Changes - Added `chattr +i` on `/sandbox/.openclaw` directory and all symlinks after the validation loop - Guarded behind `command -v chattr` check — degrades gracefully if not available - All `chattr` calls use `|| true` to avoid failures on filesystems that don't support immutable flags ## Security Analysis | Layer | Protection | Status | |-------|-----------|--------| | DAC (chmod/chown) | Directory owned by root:root, mode 755 | Already in place (Dockerfile) | | Landlock | `/sandbox/.openclaw` in read-only policy | `best_effort` — not enforced on older kernels | | **Immutable flag** | `chattr +i` prevents modification even by root | **Added by this PR** | The immutable flag cannot be removed by the sandbox user (requires `CAP_LINUX_IMMUTABLE`), closing the TOCTOU window even if DAC or Landlock are bypassed. ## Test Plan - [x] Full test suite: 650/653 passed (3 pre-existing failures in `service-env.test.js`) - [x] Verified graceful degradation when `chattr` is unavailable - [x] Confirmed `chattr` guard (`command -v`) prevents errors in minimal containers Closes NVIDIA#1019 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added additional sandbox hardening: verification of internal links followed by an optional immutability step to protect those links from runtime modification, improving integrity and reducing risk of unauthorized changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
## Summary This follow-up builds on #1137 and improves the observability around immutable symlink hardening without changing the underlying defense-in-depth approach. ## What Changed - factors `.openclaw` symlink validation into a reusable helper so both startup paths use the same validation logic - adds explicit security logging when immutable hardening succeeds, is partial, or is skipped because `chattr` is unavailable - extends the gateway-isolation E2E to fail if `chattr` is missing from the image, so the mitigation cannot silently disappear ## Why The original immutable-hardening fix is directionally strong, but the `chattr` path is intentionally best-effort and currently silent. That makes the mitigation harder to trust and harder to debug because: - a missing `chattr` binary looks the same as successful hardening - partial `chattr +i` failures are suppressed with no visibility - the image can regress and stop shipping `chattr` without CI catching it These changes make the mitigation easier to audit while staying compatible with the current layered hardening model. ## Validation - `bash -n scripts/nemoclaw-start.sh` - `bash -n test/e2e-gateway-isolation.sh` - `git diff --check` - not run: `test/e2e-gateway-isolation.sh` (`docker` is not installed in this environment) ## Relationship To #1137 This is a repost of the follow-up originally opened as `latenighthackathon#1`, now targeted at `NVIDIA/NemoClaw` as requested. ## Note This replaces `#1467`, which GitHub auto-closed because the repository's contributor open-PR limit was hit at the time. Signed-off-by: 13ernkastel <LennonCMJ@live.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced startup process validation to ensure system integrity and correct configuration * Improved security hardening mechanisms with comprehensive logging and graceful fallback handling when system features are unavailable * **Tests** * Updated end-to-end integration tests to verify system hardening capabilities and feature availability <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
## Summary This follow-up builds on NVIDIA#1137 and improves the observability around immutable symlink hardening without changing the underlying defense-in-depth approach. ## What Changed - factors `.openclaw` symlink validation into a reusable helper so both startup paths use the same validation logic - adds explicit security logging when immutable hardening succeeds, is partial, or is skipped because `chattr` is unavailable - extends the gateway-isolation E2E to fail if `chattr` is missing from the image, so the mitigation cannot silently disappear ## Why The original immutable-hardening fix is directionally strong, but the `chattr` path is intentionally best-effort and currently silent. That makes the mitigation harder to trust and harder to debug because: - a missing `chattr` binary looks the same as successful hardening - partial `chattr +i` failures are suppressed with no visibility - the image can regress and stop shipping `chattr` without CI catching it These changes make the mitigation easier to audit while staying compatible with the current layered hardening model. ## Validation - `bash -n scripts/nemoclaw-start.sh` - `bash -n test/e2e-gateway-isolation.sh` - `git diff --check` - not run: `test/e2e-gateway-isolation.sh` (`docker` is not installed in this environment) ## Relationship To NVIDIA#1137 This is a repost of the follow-up originally opened as `latenighthackathon#1`, now targeted at `NVIDIA/NemoClaw` as requested. ## Note This replaces `NVIDIA#1467`, which GitHub auto-closed because the repository's contributor open-PR limit was hit at the time. Signed-off-by: 13ernkastel <LennonCMJ@live.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced startup process validation to ensure system integrity and correct configuration * Improved security hardening mechanisms with comprehensive logging and graceful fallback handling when system features are unavailable * **Tests** * Updated end-to-end integration tests to verify system hardening capabilities and feature availability <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
…IDIA#1137) ## Summary The symlink validation loop in `nemoclaw-start.sh` verifies that all symlinks in `/sandbox/.openclaw/` point to their expected `/sandbox/.openclaw-data/` targets, but this check runs only once at boot. After validation completes (line 360), the gateway starts on the next line (line 366), leaving a TOCTOU window where symlinks could theoretically be swapped. This adds defense-in-depth by setting the immutable flag (`chattr +i`) on both the `/sandbox/.openclaw` directory and its validated symlinks immediately after the validation loop passes. ## Changes - Added `chattr +i` on `/sandbox/.openclaw` directory and all symlinks after the validation loop - Guarded behind `command -v chattr` check — degrades gracefully if not available - All `chattr` calls use `|| true` to avoid failures on filesystems that don't support immutable flags ## Security Analysis | Layer | Protection | Status | |-------|-----------|--------| | DAC (chmod/chown) | Directory owned by root:root, mode 755 | Already in place (Dockerfile) | | Landlock | `/sandbox/.openclaw` in read-only policy | `best_effort` — not enforced on older kernels | | **Immutable flag** | `chattr +i` prevents modification even by root | **Added by this PR** | The immutable flag cannot be removed by the sandbox user (requires `CAP_LINUX_IMMUTABLE`), closing the TOCTOU window even if DAC or Landlock are bypassed. ## Test Plan - [x] Full test suite: 650/653 passed (3 pre-existing failures in `service-env.test.js`) - [x] Verified graceful degradation when `chattr` is unavailable - [x] Confirmed `chattr` guard (`command -v`) prevents errors in minimal containers Closes NVIDIA#1019 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added additional sandbox hardening: verification of internal links followed by an optional immutability step to protect those links from runtime modification, improving integrity and reducing risk of unauthorized changes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
## Summary This follow-up builds on NVIDIA#1137 and improves the observability around immutable symlink hardening without changing the underlying defense-in-depth approach. ## What Changed - factors `.openclaw` symlink validation into a reusable helper so both startup paths use the same validation logic - adds explicit security logging when immutable hardening succeeds, is partial, or is skipped because `chattr` is unavailable - extends the gateway-isolation E2E to fail if `chattr` is missing from the image, so the mitigation cannot silently disappear ## Why The original immutable-hardening fix is directionally strong, but the `chattr` path is intentionally best-effort and currently silent. That makes the mitigation harder to trust and harder to debug because: - a missing `chattr` binary looks the same as successful hardening - partial `chattr +i` failures are suppressed with no visibility - the image can regress and stop shipping `chattr` without CI catching it These changes make the mitigation easier to audit while staying compatible with the current layered hardening model. ## Validation - `bash -n scripts/nemoclaw-start.sh` - `bash -n test/e2e-gateway-isolation.sh` - `git diff --check` - not run: `test/e2e-gateway-isolation.sh` (`docker` is not installed in this environment) ## Relationship To NVIDIA#1137 This is a repost of the follow-up originally opened as `latenighthackathon#1`, now targeted at `NVIDIA/NemoClaw` as requested. ## Note This replaces `NVIDIA#1467`, which GitHub auto-closed because the repository's contributor open-PR limit was hit at the time. Signed-off-by: 13ernkastel <LennonCMJ@live.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced startup process validation to ensure system integrity and correct configuration * Improved security hardening mechanisms with comprehensive logging and graceful fallback handling when system features are unavailable * **Tests** * Updated end-to-end integration tests to verify system hardening capabilities and feature availability <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Summary
The symlink validation loop in
nemoclaw-start.shverifies that all symlinks in/sandbox/.openclaw/point to their expected/sandbox/.openclaw-data/targets, but this check runs only once at boot. After validation completes (line 360), the gateway starts on the next line (line 366), leaving a TOCTOU window where symlinks could theoretically be swapped.This adds defense-in-depth by setting the immutable flag (
chattr +i) on both the/sandbox/.openclawdirectory and its validated symlinks immediately after the validation loop passes.Changes
chattr +ion/sandbox/.openclawdirectory and all symlinks after the validation loopcommand -v chattrcheck — degrades gracefully if not availablechattrcalls use|| trueto avoid failures on filesystems that don't support immutable flagsSecurity Analysis
/sandbox/.openclawin read-only policybest_effort— not enforced on older kernelschattr +iprevents modification even by rootThe immutable flag cannot be removed by the sandbox user (requires
CAP_LINUX_IMMUTABLE), closing the TOCTOU window even if DAC or Landlock are bypassed.Test Plan
service-env.test.js)chattris unavailablechattrguard (command -v) prevents errors in minimal containersCloses #1019
Summary by CodeRabbit
Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com