fix(security): address NC-2227-01 through NC-2227-05 from security review - #2439
Merged
ericksoa merged 4 commits intoApr 24, 2026
Merged
Conversation
…view Follow-up to PR #2227 (mutable-by-default refactor). Fixes all five findings from the 2026-04-24 security and vulnerability review: - NC-2227-01 (Critical): Guard migrate_legacy_layout with sentinel, root-only, and ownership checks; never chown -R the config dir itself; reapply shields-up after migration if previously active - NC-2227-02 (High): Three-state shields model (mutable_default / locked / temporarily_unlocked) so fresh sandboxes report NOT CONFIGURED instead of falsely claiming Shields: UP - NC-2227-03 (High): Timer auto-restore now resolves the full agent config target including sensitive sidecar files before re-locking - NC-2227-04 (High): Remove tar -h from backup and restore; add pre-backup lstat audit rejecting symlinks and special files - NC-2227-05 (High): Lock high-risk state dirs (skills, hooks, cron, agents, extensions, plugins) during shields up; restore on down Signed-off-by: Andrew Erickson <aerickson@nvidia.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
… test The existing nemoclaw-start.test.ts asserts every `echo` line in the non-root block redirects to stderr. The migration sentinel write (`echo "..." > "$sentinel"`) is a file write, not a diagnostic, so use printf instead to avoid matching the echo-grep pattern.
Auto-formatting from shfmt (shell) and prettier (TypeScript) to satisfy the CI "files were modified by following hooks" check.
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
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
Follow-up to PR #2227 (
refactor(sandbox): default to mutable config, make shields opt-in). Addresses all five findings from the 2026-04-24 security and vulnerability review so #2227 can proceed to merge without blocking on these concerns.Fixes #2300 (partial — shared concern about mutable config attack surface)
Changes
NC-2227-01 (Critical): Legacy migration can undo
shields upmigrate_legacy_layout()inscripts/nemoclaw-start.shnow has three guards:.migration-completemarker prevents re-running on already-migrated sandboxessandboxuser (detects agent-planted triggers; legitimate legacy dirs are root-owned from the image build)Additionally: replaced blanket
chown -R sandbox:sandbox "$config_dir"with targeted subdirectory-only chown, preserving root ownership onopenclaw.json,.config-hash,.env. Reapplies shields-up ownership after migration if shields were previously active.NC-2227-02 (High): Fresh mutable sandboxes report
Shields: UPIntroduced a three-state model in
src/lib/shields.ts:mutable_default,locked,temporarily_unlocked. Fresh sandboxes (no state file) now reportNOT CONFIGURED (default mutable state)instead of falsely claiming lockdown is active.isShieldsDown()returns true for bothmutable_defaultandtemporarily_unlocked— onlylocked(after explicitshields up) returns false.NC-2227-03 (High): Auto-restore does not re-lock sensitive sidecar files
src/lib/shields-timer.tsnow callsresolveAgentConfig(sandboxName)to get the full lock target (including.config-hash,.env) before callinglockAgentConfig. Falls back to bareconfigPath/configDirfrom argv if resolution fails.NC-2227-04 (High): Backup dereferences untrusted symlinks
Removed
tar -hflag from both backup and restore insrc/lib/sandbox-state.ts. Added a pre-backupfind-based audit that rejects symlinks (-type l) and special files before creating the tar, preventing exfiltration of config contents via agent-planted symlinks.NC-2227-05 (High):
shields upleaves co-located state directories writableAdded
HIGH_RISK_STATE_DIRSconstant (skills,hooks,cron,agents,extensions,plugins).lockAgentConfig()now recursivelychown -R root:rootandchmod 755these directories during shields-up.unlockAgentConfig()restoressandbox:sandboxownership during shields-down.Type of Change
Verification
Tests added:
test/nemoclaw-start.test.ts— 6 tests for migration guards (sentinel, root-only, ownership, no blanket chown, shields reapplication, sentinel permissions)test/shields.test.ts— 10 tests covering three-state model derivation, tar flag regression, and state directory lockingNote: NC-2227-02 unit tests replicate
deriveShieldsModelogic inline (CJS module not directly importable in vitest). Full code path is exercised by E2E tests intest/e2e/test-shields-config.sh.Signed-off-by: Andrew Erickson aerickson@nvidia.com